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
