Re: [PATCH 02/14] Support for adding and stripping location_t wrapper nodes

[Richard Biener]

On Wed, Nov 15, 2017 at 4:33 PM, David Malcolm  wrote:
> On Wed, 2017-11-15 at 12:11 +0100, Richard Biener wrote:
>> On Wed, Nov 15, 2017 at 7:17 AM, Trevor Saunders > rg> wrote:
>> > On Fri, Nov 10, 2017 at 04:45:17PM -0500, David Malcolm wrote:
>> > > This patch provides a mechanism in tree.c for adding a wrapper
>> > > node
>> > > for expressing a location_t, for those nodes for which
>> > > !CAN_HAVE_LOCATION_P, along with a new method of cp_expr.
>> > >
>> > > It's called in later patches in the kit via that new method.
>> > >
>> > > In this version of the patch, I use NON_LVALUE_EXPR for wrapping
>> > > constants, and VIEW_CONVERT_EXPR for other nodes.
>> > >
>> > > I also turned off wrapper nodes for EXCEPTIONAL_CLASS_P, for the
>> > > sake
>> > > of keeping the patch kit more minimal.
>> > >
>> > > The patch also adds a STRIP_ANY_LOCATION_WRAPPER macro for
>> > > stripping
>> > > such nodes, used later on in the patch kit.
>> >
>> > I happened to start reading this series near the end and was rather
>> > confused by this macro since it changes variables in a rather
>> > unhygienic
>> > way.  Did you consider just defining a inline function to return
>> > the
>> > actual decl?  It seems like its not used that often so the slight
>> > extra
>> > syntax should be that big a deal compared to the explicitness.
>>
>> Existing practice  (STRIP_NOPS & friends).  I'm fine either way,
>> the patch looks good.
>>
>> Eventually you can simplify things by doing less checking in
>> location_wrapper_p, like only checking
>>
>> +inline bool location_wrapper_p (const_tree exp)
>> +{
>> +  if ((TREE_CODE (exp) == NON_LVALUE_EXPR
>> +   || (TREE_CODE (exp) == VIEW_CONVERT_EXPR
>> +  && (TREE_TYPE (exp)
>> + == TREE_TYPE (TREE_OPERAND (exp, 0)))
>> +return true;
>> +  return false;
>> +}
>>
>> and renaming to maybe_location_wrapper_p.  After all you can't really
>> distinguish location wrappers from non-location wrappers?  (and why
>> would you want to?)
>
> That's the implementation I originally tried.
>
> As noted in an earlier thread about this, the problem I ran into was
> (in g++.dg/conversion/reinterpret1.C):
>
>   // PR c++/15076
>
>   struct Y { Y(int &); };
>
>   int v;
>   Y y1(reinterpret_cast(v));  // { dg-error "" }
>
> where the "reinterpret_cast" has the same type as the VAR_DECL v,
> and hence the argument to y1 is a NON_LVALUE_EXPR around a VAR_DECL,
> where both have the same type, and hence location_wrapper_p () on the
> cast would return true.
>
> Compare with:
>
>   Y y1(v);
>
> where the argument "v" with a location wrapper is a VIEW_CONVERT_EXPR
> around a VAR_DECL.
>
> With the simpler conditions you suggested above, both are treated as
> location wrappers (leading to the dg-error in the test failing),
> whereas with the condition in the patch, only the latter is treated as
> a location wrapper, and an error is correctly emitted for the dg-error.
>
> Hope this sounds sane.  Maybe the function needs a more detailed
> comment explaining this?

Yes.  I guess the above would argue for a new tree code but I can
see that it is better to avoid that.

Thanks,
Richard.

> Thanks
> Dave
>
>
>> Thanks,
>> Richard.
>>
>> > Other than that the series seems reasonable, and I look forward to
>> > having wrappers in more places.  I seem to remember something I
>> > wanted
>> > to warn about they would make much easier.
>> >
>> > Thanks
>> >
>> > Trev
>> >

Re: [PATCH 02/14] Support for adding and stripping location_t wrapper nodes

[David Malcolm]

On Wed, 2017-11-15 at 12:11 +0100, Richard Biener wrote:
> On Wed, Nov 15, 2017 at 7:17 AM, Trevor Saunders  rg> wrote:
> > On Fri, Nov 10, 2017 at 04:45:17PM -0500, David Malcolm wrote:
> > > This patch provides a mechanism in tree.c for adding a wrapper
> > > node
> > > for expressing a location_t, for those nodes for which
> > > !CAN_HAVE_LOCATION_P, along with a new method of cp_expr.
> > > 
> > > It's called in later patches in the kit via that new method.
> > > 
> > > In this version of the patch, I use NON_LVALUE_EXPR for wrapping
> > > constants, and VIEW_CONVERT_EXPR for other nodes.
> > > 
> > > I also turned off wrapper nodes for EXCEPTIONAL_CLASS_P, for the
> > > sake
> > > of keeping the patch kit more minimal.
> > > 
> > > The patch also adds a STRIP_ANY_LOCATION_WRAPPER macro for
> > > stripping
> > > such nodes, used later on in the patch kit.
> > 
> > I happened to start reading this series near the end and was rather
> > confused by this macro since it changes variables in a rather
> > unhygienic
> > way.  Did you consider just defining a inline function to return
> > the
> > actual decl?  It seems like its not used that often so the slight
> > extra
> > syntax should be that big a deal compared to the explicitness.
> 
> Existing practice  (STRIP_NOPS & friends).  I'm fine either way,
> the patch looks good.
> 
> Eventually you can simplify things by doing less checking in
> location_wrapper_p, like only checking
> 
> +inline bool location_wrapper_p (const_tree exp)
> +{
> +  if ((TREE_CODE (exp) == NON_LVALUE_EXPR
> +   || (TREE_CODE (exp) == VIEW_CONVERT_EXPR
> +  && (TREE_TYPE (exp)
> + == TREE_TYPE (TREE_OPERAND (exp, 0)))
> +return true;
> +  return false;
> +}
> 
> and renaming to maybe_location_wrapper_p.  After all you can't really
> distinguish location wrappers from non-location wrappers?  (and why
> would you want to?)

That's the implementation I originally tried.

As noted in an earlier thread about this, the problem I ran into was
(in g++.dg/conversion/reinterpret1.C):

  // PR c++/15076

  struct Y { Y(int &); };

  int v;
  Y y1(reinterpret_cast(v));  // { dg-error "" }

where the "reinterpret_cast" has the same type as the VAR_DECL v,
and hence the argument to y1 is a NON_LVALUE_EXPR around a VAR_DECL,
where both have the same type, and hence location_wrapper_p () on the
cast would return true.

Compare with:

  Y y1(v);

where the argument "v" with a location wrapper is a VIEW_CONVERT_EXPR
around a VAR_DECL.

With the simpler conditions you suggested above, both are treated as
location wrappers (leading to the dg-error in the test failing),
whereas with the condition in the patch, only the latter is treated as
a location wrapper, and an error is correctly emitted for the dg-error.

Hope this sounds sane.  Maybe the function needs a more detailed
comment explaining this?

Thanks
Dave


> Thanks,
> Richard.
> 
> > Other than that the series seems reasonable, and I look forward to
> > having wrappers in more places.  I seem to remember something I
> > wanted
> > to warn about they would make much easier.
> > 
> > Thanks
> > 
> > Trev
> > 

Re: [PATCH 02/14] Support for adding and stripping location_t wrapper nodes

[Richard Biener]

On Wed, Nov 15, 2017 at 7:17 AM, Trevor Saunders  wrote:
> On Fri, Nov 10, 2017 at 04:45:17PM -0500, David Malcolm wrote:
>> This patch provides a mechanism in tree.c for adding a wrapper node
>> for expressing a location_t, for those nodes for which
>> !CAN_HAVE_LOCATION_P, along with a new method of cp_expr.
>>
>> It's called in later patches in the kit via that new method.
>>
>> In this version of the patch, I use NON_LVALUE_EXPR for wrapping
>> constants, and VIEW_CONVERT_EXPR for other nodes.
>>
>> I also turned off wrapper nodes for EXCEPTIONAL_CLASS_P, for the sake
>> of keeping the patch kit more minimal.
>>
>> The patch also adds a STRIP_ANY_LOCATION_WRAPPER macro for stripping
>> such nodes, used later on in the patch kit.
>
> I happened to start reading this series near the end and was rather
> confused by this macro since it changes variables in a rather unhygienic
> way.  Did you consider just defining a inline function to return the
> actual decl?  It seems like its not used that often so the slight extra
> syntax should be that big a deal compared to the explicitness.

Existing practice  (STRIP_NOPS & friends).  I'm fine either way,
the patch looks good.

Eventually you can simplify things by doing less checking in
location_wrapper_p, like only checking

+inline bool location_wrapper_p (const_tree exp)
+{
+  if ((TREE_CODE (exp) == NON_LVALUE_EXPR
+   || (TREE_CODE (exp) == VIEW_CONVERT_EXPR
+  && (TREE_TYPE (exp)
+ == TREE_TYPE (TREE_OPERAND (exp, 0)))
+return true;
+  return false;
+}

and renaming to maybe_location_wrapper_p.  After all you can't really
distinguish location wrappers from non-location wrappers?  (and why
would you want to?)

Thanks,
Richard.

> Other than that the series seems reasonable, and I look forward to
> having wrappers in more places.  I seem to remember something I wanted
> to warn about they would make much easier.
>
> Thanks
>
> Trev
>

Re: [PATCH 02/14] Support for adding and stripping location_t wrapper nodes

[Trevor Saunders]

On Fri, Nov 10, 2017 at 04:45:17PM -0500, David Malcolm wrote:
> This patch provides a mechanism in tree.c for adding a wrapper node
> for expressing a location_t, for those nodes for which
> !CAN_HAVE_LOCATION_P, along with a new method of cp_expr.
> 
> It's called in later patches in the kit via that new method.
> 
> In this version of the patch, I use NON_LVALUE_EXPR for wrapping
> constants, and VIEW_CONVERT_EXPR for other nodes.
> 
> I also turned off wrapper nodes for EXCEPTIONAL_CLASS_P, for the sake
> of keeping the patch kit more minimal.
> 
> The patch also adds a STRIP_ANY_LOCATION_WRAPPER macro for stripping
> such nodes, used later on in the patch kit.

I happened to start reading this series near the end and was rather
confused by this macro since it changes variables in a rather unhygienic
way.  Did you consider just defining a inline function to return the
actual decl?  It seems like its not used that often so the slight extra
syntax should be that big a deal compared to the explicitness.

Other than that the series seems reasonable, and I look forward to
having wrappers in more places.  I seem to remember something I wanted
to warn about they would make much easier.

Thanks

Trev

[PATCH 02/14] Support for adding and stripping location_t wrapper nodes

[David Malcolm]

This patch provides a mechanism in tree.c for adding a wrapper node
for expressing a location_t, for those nodes for which
!CAN_HAVE_LOCATION_P, along with a new method of cp_expr.

It's called in later patches in the kit via that new method.

In this version of the patch, I use NON_LVALUE_EXPR for wrapping
constants, and VIEW_CONVERT_EXPR for other nodes.

I also turned off wrapper nodes for EXCEPTIONAL_CLASS_P, for the sake
of keeping the patch kit more minimal.

The patch also adds a STRIP_ANY_LOCATION_WRAPPER macro for stripping
such nodes, used later on in the patch kit.

gcc/ChangeLog:
PR c++/43486
* tree.c (maybe_wrap_with_location): New function.
* tree.h (STRIP_ANY_LOCATION_WRAPPER): New macro.
(maybe_wrap_with_location): New decl.
(location_wrapper_p): New inline function.

gcc/cp/ChangeLog:
PR c++/43486
* cp-tree.h (cp_expr::maybe_add_location_wrapper): New method.
---
 gcc/cp/cp-tree.h |  6 ++
 gcc/tree.c   | 27 +++
 gcc/tree.h   | 26 ++
 3 files changed, 59 insertions(+)

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 874cbcb..726b6f5 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -93,6 +93,12 @@ public:
 set_location (make_location (m_loc, start, finish));
   }
 
+  cp_expr& maybe_add_location_wrapper ()
+  {
+m_value = maybe_wrap_with_location (m_value, m_loc);
+return *this;
+  }
+
  private:
   tree m_value;
   location_t m_loc;
diff --git a/gcc/tree.c b/gcc/tree.c
index 28e157f..50c818c 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -13747,6 +13747,33 @@ set_source_range (tree expr, source_range src_range)
   return adhoc;
 }
 
+/* Return EXPR, potentially wrapped with a LOCATION_WRAPPER_EXPR node
+   at LOC, if !CAN_HAVE_LOCATION_P (expr).  */
+
+tree
+maybe_wrap_with_location (tree expr, location_t loc)
+{
+  if (expr == NULL)
+return NULL;
+  if (loc == UNKNOWN_LOCATION)
+return expr;
+  if (CAN_HAVE_LOCATION_P (expr))
+return expr;
+  /* We should only be adding wrappers for constants and for decls,
+ or for some exceptional tree nodes (e.g. BASELINK in the C++ FE).  */
+  gcc_assert (CONSTANT_CLASS_P (expr)
+ || DECL_P (expr)
+ || EXCEPTIONAL_CLASS_P (expr));
+
+  if (EXCEPTIONAL_CLASS_P (expr))
+return expr;
+
+  if (CONSTANT_CLASS_P (expr))
+return build1_loc (loc, NON_LVALUE_EXPR, TREE_TYPE (expr), expr);
+  else
+return build1_loc (loc, VIEW_CONVERT_EXPR, TREE_TYPE (expr), expr);
+}
+
 /* Return the name of combined function FN, for debugging purposes.  */
 
 const char *
diff --git a/gcc/tree.h b/gcc/tree.h
index 277aa91..fb45a8d 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -483,6 +483,15 @@ extern void omp_clause_range_check_failed (const_tree, 
const char *, int,
 #define STRIP_USELESS_TYPE_CONVERSION(EXP) \
   (EXP) = tree_ssa_strip_useless_type_conversions (EXP)
 
+/* Remove any VIEW_CONVERT_EXPR or NON_LVALUE_EXPR that's purely
+   in use to provide a location_t.  */
+
+#define STRIP_ANY_LOCATION_WRAPPER(EXP) \
+  do { \
+if (location_wrapper_p (EXP))  \
+  (EXP) = TREE_OPERAND ((EXP), 0); \
+  } while (0)
+
 /* Nonzero if TYPE represents a vector type.  */
 
 #define VECTOR_TYPE_P(TYPE) (TREE_CODE (TYPE) == VECTOR_TYPE)
@@ -1147,6 +1156,8 @@ get_expr_source_range (tree expr)
 
 extern void protected_set_expr_location (tree, location_t);
 
+extern tree maybe_wrap_with_location (tree, location_t);
+
 /* In a TARGET_EXPR node.  */
 #define TARGET_EXPR_SLOT(NODE) TREE_OPERAND_CHECK_CODE (NODE, TARGET_EXPR, 0)
 #define TARGET_EXPR_INITIAL(NODE) TREE_OPERAND_CHECK_CODE (NODE, TARGET_EXPR, 
1)
@@ -3637,6 +3648,21 @@ id_equal (const char *str, const_tree id)
   return !strcmp (str, IDENTIFIER_POINTER (id));
 }
 
+/* Test if EXP is merely a wrapper node, added to express a location_t
+   on behalf of its child.  */
+
+inline bool location_wrapper_p (const_tree exp)
+{
+  if (((TREE_CODE (exp) == NON_LVALUE_EXPR
+   && CONSTANT_CLASS_P (TREE_OPERAND (exp, 0)))
+   || (TREE_CODE (exp) == VIEW_CONVERT_EXPR
+  && !CONSTANT_CLASS_P (TREE_OPERAND (exp, 0
+  && (TREE_TYPE (exp)
+ == TREE_TYPE (TREE_OPERAND (exp, 0
+return true;
+  return false;
+}
+
 #define error_mark_nodeglobal_trees[TI_ERROR_MARK]
 
 #define intQI_type_nodeglobal_trees[TI_INTQI_TYPE]
-- 
1.8.5.3