OK.
On Thu, Dec 21, 2017 at 12:50 PM, David Malcolm <dmalc...@redhat.com> wrote: > On Wed, 2017-12-20 at 23:56 -0500, Jason Merrill wrote: >> On Wed, Dec 20, 2017 at 5:14 PM, David Malcolm <dmalc...@redhat.com> >> wrote: >> > On Mon, 2017-12-11 at 18:39 -0500, Jason Merrill wrote: >> > > On 11/10/2017 04:45 PM, David Malcolm wrote: >> > > > Without this, then lvalue_p returns false for decls, and hence >> > > > e.g. uses of them for references fail. >> > > > >> > > > Stripping location wrappers in lvalue_kind restores the correct >> > > > behavior of lvalue_p etc. >> > > > >> > > > gcc/cp/ChangeLog: >> > > > * tree.c (lvalue_kind): Strip any location wrapper. >> > > >> > > Rather, lvalue_kind should learn to handle VIEW_CONVERT_EXPR. >> > This patch does so, using: >> > >> > case NON_LVALUE_EXPR: >> > case VIEW_CONVERT_EXPR: >> > if (location_wrapper_p (ref)) >> > return lvalue_kind (TREE_OPERAND (ref, 0)); >> > >> > As well as the VIEW_CONVERT_EXPR, lvalue_kind needs to handle >> > NON_LVALUE_EXPR, otherwise a location-wrapped string literal >> > hits this clause in the "default" case: >> > >> > if (CLASS_TYPE_P (TREE_TYPE (ref)) >> > || TREE_CODE (TREE_TYPE (ref)) == ARRAY_TYPE) >> > return clk_class; >> > >> > when it should have hit this one (after removing the >> > location wrapper): >> > >> > case STRING_CST: >> > case COMPOUND_LITERAL_EXPR: >> > return clk_ordinary; >> >> Ah, the issue is that string literals should use VIEW_CONVERT_EXPR >> rather than NON_LVALUE_EXPR, since they are lvalues. With that >> change, we shouldn't need to handle NON_LVALUE_EXPR specifically. >> >> Jason > > Thanks. Here's an updated version of the patch which tweaks > maybe_wrap_with_location (from patch 02/14 [1]) for STRING_CST to do so, > also adding some more test coverage to selftest::test_location_wrappers > for the STRING_CST case. With that, it only adds the VIEW_CONVERT_EXPR > case to lvalue_kind. > > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu, as > part of the kit. > Manually tested with "make s-selftest-c++" (since we don't run > the selftests for cc1plus by default). > > OK for trunk once the rest of the kit is approved? > > [1] https://gcc.gnu.org/ml/gcc-patches/2017-11/msg02611.html > > gcc/cp/ChangeLog: > * cp-lang.c (selftest::run_cp_tests): Call > selftest::cp_tree_c_tests. > * cp-tree.h (selftest::run_cp_tests): Move decl to bottom of file. > (selftest::cp_tree_c_tests): New decl. > * tree.c: Include "selftest.h". > (lvalue_kind): Handle VIEW_CONVERT_EXPR location wrapper nodes. > (selftest::test_lvalue_kind): New function. > (selftest::cp_tree_c_tests): New function. > > gcc/ChangeLog: > * tree.c (maybe_wrap_with_location): Use VIEW_CONVERT_EXPR for > STRING_CST. > (selftest::test_location_wrappers): Add coverage for STRING_CST. > * tree.h (location_wrapper_p): Update comment to reflect change to > STRING_CST. > --- > gcc/cp/cp-lang.c | 1 + > gcc/cp/cp-tree.h | 16 ++++++++------ > gcc/cp/tree.c | 66 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > gcc/tree.c | 19 +++++++++++++--- > gcc/tree.h | 4 ++-- > 5 files changed, 95 insertions(+), 11 deletions(-) > > diff --git a/gcc/cp/cp-lang.c b/gcc/cp/cp-lang.c > index 805319a..9bb4ce7 100644 > --- a/gcc/cp/cp-lang.c > +++ b/gcc/cp/cp-lang.c > @@ -247,6 +247,7 @@ run_cp_tests (void) > c_family_tests (); > > /* Additional C++-specific tests. */ > + cp_tree_c_tests (); > } > > } // namespace selftest > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index 9879e16..bb405f2 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -7415,12 +7415,6 @@ extern tree cp_ubsan_maybe_instrument_downcast > (location_t, tree, tree, tree); > extern tree cp_ubsan_maybe_instrument_cast_to_vbase (location_t, tree, tree); > extern void cp_ubsan_maybe_initialize_vtbl_ptrs (tree); > > -#if CHECKING_P > -namespace selftest { > - extern void run_cp_tests (void); > -} // namespace selftest > -#endif /* #if CHECKING_P */ > - > /* Inline bodies. */ > > inline tree > @@ -7451,6 +7445,16 @@ named_decl_hash::equal (const value_type existing, > compare_type candidate) > return candidate == name; > } > > +#if CHECKING_P > +namespace selftest { > + extern void run_cp_tests (void); > + > + /* Declarations for specific families of tests within cp, > + by source file, in alphabetical order. */ > + extern void cp_tree_c_tests (void); > +} // namespace selftest > +#endif /* #if CHECKING_P */ > + > /* -- end of C++ */ > > #endif /* ! GCC_CP_TREE_H */ > diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c > index 0ae2eff..6900bc7 100644 > --- a/gcc/cp/tree.c > +++ b/gcc/cp/tree.c > @@ -35,6 +35,7 @@ along with GCC; see the file COPYING3. If not see > #include "stringpool.h" > #include "attribs.h" > #include "flags.h" > +#include "selftest.h" > > static tree bot_manip (tree *, int *, void *); > static tree bot_replace (tree *, int *, void *); > @@ -240,6 +241,11 @@ lvalue_kind (const_tree ref) > case NON_DEPENDENT_EXPR: > return lvalue_kind (TREE_OPERAND (ref, 0)); > > + case VIEW_CONVERT_EXPR: > + if (location_wrapper_p (ref)) > + return lvalue_kind (TREE_OPERAND (ref, 0)); > + /* Fallthrough. */ > + > default: > if (!TREE_TYPE (ref)) > return clk_none; > @@ -5339,4 +5345,64 @@ lang_check_failed (const char* file, int line, const > char* function) > } > #endif /* ENABLE_TREE_CHECKING */ > > +#if CHECKING_P > + > +namespace selftest { > + > +/* Verify that lvalue_kind () works, for various expressions, > + and that location wrappers don't affect the results. */ > + > +static void > +test_lvalue_kind () > +{ > + location_t loc = BUILTINS_LOCATION; > + > + /* Verify constants and parameters, without and with > + location wrappers. */ > + tree int_cst = build_int_cst (integer_type_node, 42); > + ASSERT_EQ (clk_none, lvalue_kind (integer_zero_node)); > + > + tree wrapped_int_cst = maybe_wrap_with_location (int_cst, loc); > + ASSERT_TRUE (location_wrapper_p (wrapped_int_cst)); > + ASSERT_EQ (clk_none, lvalue_kind (integer_zero_node)); > + > + tree string_lit = build_string (4, "foo"); > + TREE_TYPE (string_lit) = char_array_type_node; > + string_lit = fix_string_type (string_lit); > + ASSERT_EQ (clk_ordinary, lvalue_kind (string_lit)); > + > + tree wrapped_string_lit = maybe_wrap_with_location (string_lit, loc); > + ASSERT_TRUE (location_wrapper_p (wrapped_string_lit)); > + ASSERT_EQ (clk_ordinary, lvalue_kind (wrapped_string_lit)); > + > + tree parm = build_decl (UNKNOWN_LOCATION, PARM_DECL, > + get_identifier ("some_parm"), > + integer_type_node); > + ASSERT_EQ (clk_ordinary, lvalue_kind (parm)); > + > + tree wrapped_parm = maybe_wrap_with_location (parm, loc); > + ASSERT_TRUE (location_wrapper_p (wrapped_parm)); > + ASSERT_EQ (clk_ordinary, lvalue_kind (parm)); > + > + /* Verify that lvalue_kind of std::move on a parm isn't > + affected by location wrappers. */ > + tree rvalue_ref_of_parm = move (parm); > + ASSERT_EQ (clk_rvalueref, lvalue_kind (rvalue_ref_of_parm)); > + tree rvalue_ref_of_wrapped_parm = move (wrapped_parm); > + ASSERT_EQ (clk_rvalueref, lvalue_kind (rvalue_ref_of_wrapped_parm)); > +} > + > +/* Run all of the selftests within this file. */ > + > +void > +cp_tree_c_tests () > +{ > + test_lvalue_kind (); > +} > + > +} // namespace selftest > + > +#endif /* #if CHECKING_P */ > + > + > #include "gt-cp-tree.h" > diff --git a/gcc/tree.c b/gcc/tree.c > index b72f963..517f4a4 100644 > --- a/gcc/tree.c > +++ b/gcc/tree.c > @@ -13811,8 +13811,8 @@ set_source_range (tree expr, source_range src_range) > /* Return EXPR, potentially wrapped with a node expression LOC, > if !CAN_HAVE_LOCATION_P (expr). > > - NON_LVALUE_EXPR is used for wrapping constants. > - VIEW_CONVERT_EXPR is used for wrapping non-constants. > + NON_LVALUE_EXPR is used for wrapping constants, apart from STRING_CST. > + VIEW_CONVERT_EXPR is used for wrapping non-constants and STRING_CST. > > Wrapper nodes can be identified using location_wrapper_p. */ > > @@ -13834,7 +13834,7 @@ maybe_wrap_with_location (tree expr, location_t loc) > if (EXCEPTIONAL_CLASS_P (expr)) > return expr; > > - if (CONSTANT_CLASS_P (expr)) > + if (CONSTANT_CLASS_P (expr) && TREE_CODE (expr) != STRING_CST) > return build1_loc (loc, NON_LVALUE_EXPR, TREE_TYPE (expr), expr); > else > return build1_loc (loc, VIEW_CONVERT_EXPR, TREE_TYPE (expr), expr); > @@ -14238,6 +14238,18 @@ test_location_wrappers () > ASSERT_EQ (loc, EXPR_LOCATION (wrapped_int_cst)); > ASSERT_EQ (int_cst, tree_strip_any_location_wrapper (wrapped_int_cst)); > > + /* Wrapping a STRING_CST. */ > + tree string_cst = build_string (4, "foo"); > + ASSERT_FALSE (CAN_HAVE_LOCATION_P (string_cst)); > + ASSERT_FALSE (location_wrapper_p (string_cst)); > + > + tree wrapped_string_cst = maybe_wrap_with_location (string_cst, loc); > + ASSERT_TRUE (location_wrapper_p (wrapped_string_cst)); > + ASSERT_EQ (VIEW_CONVERT_EXPR, TREE_CODE (wrapped_string_cst)); > + ASSERT_EQ (loc, EXPR_LOCATION (wrapped_string_cst)); > + ASSERT_EQ (string_cst, tree_strip_any_location_wrapper > (wrapped_string_cst)); > + > + > /* Wrapping a variable. */ > tree int_var = build_decl (UNKNOWN_LOCATION, VAR_DECL, > get_identifier ("some_int_var"), > @@ -14258,6 +14270,7 @@ test_location_wrappers () > > /* Verify that STRIP_NOPS removes wrappers. */ > check_strip_nops (wrapped_int_cst, int_cst); > + check_strip_nops (wrapped_string_cst, string_cst); > check_strip_nops (wrapped_int_var, int_var); > > /* operand_equal_p should "see through" any location wrapper... */ > diff --git a/gcc/tree.h b/gcc/tree.h > index de085ca..7f4ea5e 100644 > --- a/gcc/tree.h > +++ b/gcc/tree.h > @@ -3679,8 +3679,8 @@ location_wrapper_p (const_tree exp) > /* A wrapper node has code NON_LVALUE_EXPR or VIEW_CONVERT_EXPR, and the > same type as its operand. > > - NON_LVALUE_EXPR is used for wrapping constants. > - VIEW_CONVERT_EXPR is used for wrapping non-constants. > + NON_LVALUE_EXPR is used for wrapping constants, apart from STRING_CST. > + VIEW_CONVERT_EXPR is used for wrapping non-constants and STRING_CST. > > A subtlety is that we may have to test whether we have the correct > TREE_CODE for the wrapped TREE_CODE. Otherwise, e.g. the C++ > expression: > -- > 1.8.5.3 >
