[v2 of PATCH 03/14] C++: add location_t wrapper nodes during parsing (minimal impl)

[David Malcolm]

On Fri, 2018-01-05 at 15:29 -0500, Jason Merrill wrote:
> On 12/29/2017 12:06 PM, David Malcolm wrote:
> > One issue I ran into was that fold_for_warn doesn't eliminate
> > location wrappers when processing_template_decl, leading to
> > failures of the template-based cases in
> > g++.dg/warn/Wmemset-transposed-args-1.C.
> > 
> > This is due to the early bailout when processing_template_decl
> > within cp_fold:
> > 
> > 2078	  if (processing_template_decl
> > 2079	      || (EXPR_P (x) && (!TREE_TYPE (x) || TREE_TYPE
> > (x) == error_mark_node)))
> > 2080	    return x;
> > 
> > which dates back to the merger of the C++ delayed folding branch.
> > 
> > I've fixed that in this version of the patch by removing that
> > "processing_template_decl ||" condition from that cp_fold early
> > bailout.
> 
> Hmm, that makes me nervous.  We might want to fold in templates when 
> called from fold_for_warn, but not for other occurrences.  But I see 
> that we check processing_template_decl in cp_fully_fold and in the
> call 
> to cp_fold_function, so I guess this is fine.
> 
> > +    case VIEW_CONVERT_EXPR:
> > +    case NON_LVALUE_EXPR:
> >      case CAST_EXPR:
> >      case REINTERPRET_CAST_EXPR:
> >      case CONST_CAST_EXPR:
> > @@ -14937,6 +14940,15 @@ tsubst_copy (tree t, tree args,
> > tsubst_flags_t complain, tree in_decl)
> >      case CONVERT_EXPR:
> >      case NOP_EXPR:
> >        {
> > +	if (location_wrapper_p (t))
> > +	  {
> > +	    /* Handle location wrappers by substituting the
> > wrapped node
> > +	       first, *then* reusing the resulting type.  Doing
> > the type
> > +	       first ensures that we handle template parameters
> > and
> > +	       parameter pack expansions.  */
> > +	    tree op0 = tsubst_copy (TREE_OPERAND (t, 0), args,
> > complain, in_decl);
> > +	    return build1 (code, TREE_TYPE (op0), op0);
> > +	  }
> 
> I'd rather handle location wrappers separately, and abort if 
> VIEW_CONVERT_EXPR or NON_LVALUE_EXPR appear other than as wrappers.

Once I fixed the issue with location_wrapper_p with decls changing
type, it turns out that trunk is already passing VIEW_CONVERT_EXPR to
tsubst_copy_and_build for non-wrapper nodes (and from there to
tsubst_copy), where the default case currently handles them.  Adding an
assert turns this into an ICE.

g++.dg/delayedfold/builtin1.C is the only instance of it I found in our
test suite, where it's used here:

class RegionLock {
  template <unsigned long> void m_fn1();
  int spinlock;
} acquire_zero;
int acquire_one;
template <unsigned long> void RegionLock::m_fn1() {
  __atomic_compare_exchange(&spinlock, &acquire_zero, &acquire_one, false, 2, 2);
                                       ^~~~~~~~~~~~~
}

(gdb) call debug_tree (t)
 <view_convert_expr 0x7ffff1a15b40
    type <pointer_type 0x7ffff18d93f0
        type <integer_type 0x7ffff18c9690 unsigned int public unsigned
type_6 SI
            size <integer_cst 0x7ffff18cc0c0 constant 32>
            unit-size <integer_cst 0x7ffff18cc0d8 constant 4>
            align:32 warn_if_not_align:0 symtab:0 alias-set -1
canonical-type 0x7ffff18c9690 precision:32 min <integer_cst
0x7ffff18cc0f0 0> max <integer_cst 0x7ffff18cc0a8 4294967295>
            pointer_to_this <pointer_type 0x7ffff18d93f0>>
        unsigned type_6 DI
        size <integer_cst 0x7ffff18ace70 constant 64>
        unit-size <integer_cst 0x7ffff18ace88 constant 8>
        align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-
type 0x7ffff18d93f0>
   
    arg:0 <non_dependent_expr 0x7ffff1a15aa0
        type <pointer_type 0x7ffff1a18150 type <record_type
0x7ffff1a039d8 RegionLock>
            unsigned type_6 DI size <integer_cst 0x7ffff18ace70 64>
unit-size <integer_cst 0x7ffff18ace88 8>
            align:64 warn_if_not_align:0 symtab:0 alias-set -1
canonical-type 0x7ffff1a18150>
       
        arg:0 <addr_expr 0x7ffff1a159e0 type <pointer_type
0x7ffff1a18150>
            arg:0 <var_decl 0x7ffff7ffbd80 acquire_zero>
            ../../src/gcc/testsuite/g++.dg/delayedfold/builtin1.C:10:40
start: ../../src/gcc/testsuite/g++.dg/delayedfold/builtin1.C:10:40
finish: ../../src/gcc/testsuite/g++.dg/delayedfold/builtin1.C:10:52>>>

(This one is just for VIEW_CONVERT_EXPR; I don't yet know of any
existing places where NON_LVALUE_EXPR can be passed to tsubst_*).

Given that, is it OK to go with the approach in this (v2) patch? 
(presumably requiring the fix to location_wrapper_p to use a flag
rather than a matching type).


> > @@ -24998,6 +25018,8 @@ build_non_dependent_expr (tree expr)
> >        && !expanding_concept ())
> >      fold_non_dependent_expr (expr);
> >  
> > +  STRIP_ANY_LOCATION_WRAPPER (expr);
> 
> Why is this needed?

https://gcc.gnu.org/ml/gcc-patches/2018-01/msg00349.html

> Jason

Thanks
Dave