[v3,of,08/14] cp/tree.c: strip location wrappers in lvalue_kind

Message ID 1513878659-57155-1-git-send-email-dmalcolm@redhat.com
State New
Headers show
Series
  • Untitled series #261
Related show

Commit Message

David Malcolm Dec. 21, 2017, 5:50 p.m.
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&regrtested 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(-)

-- 
1.8.5.3

Comments

Jason Merrill Dec. 21, 2017, 10:44 p.m. | #1
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&regrtested 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

>

Patch

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: