[v3 of PATCH 08/14] cp/tree.c: strip location wrappers in lvalue_kind
Jason Merrill
jason@redhat.com
Thu Dec 21 22:44:00 GMT 2017
OK.
On Thu, Dec 21, 2017 at 12:50 PM, David Malcolm <dmalcolm@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 <dmalcolm@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
>
More information about the Gcc-patches
mailing list