On Fri, 2018-01-05 at 17:20 -0500, David Malcolm wrote:
> 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.
>
> (I wondered if it would be possible to add a new flag to the various
> fold* calls to request folding in templates, but given that the API
> is
> partially shared with C doing so doesn't seem to make sense)
>
> > > + 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.
>
> OK. I'm testing an updated version which does so.
Doing so uncovered an issue which I'm not sure how to resolve: it's
possible for a decl to change type during parsing, after location
wrappers may have been created, which changes location_wrapper_p on
those wrappers from true to false.
Here's the most minimal reproducer I've generated so far:
1 template<typename _CharT>
2 struct basic_string {
3 static const _CharT _S_terminal;
4 static void assign(const _CharT& __c2);
5 void _M_set_length_and_sharable() {
6 assign(_S_terminal);
7 }
8 };
9
10 template<typename _CharT>
11 const _CharT basic_string<_CharT>::_S_terminal = _CharT();
12
13 void getline(basic_string<char>& __str) {
14 __str._M_set_length_and_sharable();
15 }
Asserting that the only VIEW_CONVERT_EXPR or NON_LVALUE_EXPR seen in
tsubst_copy and tsubst_copy_and_build are location_wrapper_p leads to
an ICE on the above code.
What's happening is as follows. First, in the call:
6 assign(_S_terminal);
^~~~~~~~~~~
the VAR_DECL "_S_terminal" gains a VIEW_CONVERT_EXPR location wrapper
node to express the underline shown above.
Later, during parsing of this init-declarator:
10 template<typename _CharT>
11 const _CharT basic_string<_CharT>::_S_terminal = _CharT();
^~~~~~~~~~~
...cp_parser_init_declarator calls start_decl, which calls
duplicate_decls, merging the "_S_terminal" seen here:
1 template<typename _CharT>
2 struct basic_string {
3 static const _CharT _S_terminal;
^~~~~~~~~~~
with that seen here:
10 template<typename _CharT>
11 const _CharT basic_string<_CharT>::_S_terminal = _CharT();
^~~~~~~~~~~
Both "_S_terminal" VAR_DECLs have a "_CharT" TEMPLATE_TYPE_PARM, but
these types are different tree nodes.
Hence the type of the first VAR_DECL changes in duplicate_decls here:
2152 TREE_TYPE (newdecl) = TREE_TYPE (olddecl) = newtype;
...changing type to the TEMPLATE_TYPE_PARM of the second VAR_DECL.
At this point, the location wrapper node at the callsite still has the
*old* type, and hence location_wrapper_p (wrapper) changes from true to
false, as its type no longer matches that of the decl it's wrapping.
Hence my rewritten code in tsubst_copy_and_build fails the assertion
here:
18306 case VIEW_CONVERT_EXPR:
18307 case NON_LVALUE_EXPR:
18308 gcc_assert (location_wrapper_p (t));
18309 RETURN (RECUR (TREE_OPERAND (t, 0)));
Assuming I'm correctly understanding the above, I'm not sure what the
best solution is.
Some ideas:
* accept that this is a rare case, and either:
* don't assert, or
* also allow nodes here that are "nearly" location wrappers (i.e. of
type TEMPLATE_TYPE_PARM)
* some kind of fixup of location wrapper node types when changing the
type of a decl (ugh)
* don't add location wrappers if processing a template
* introduce a new tree node for location wrappers (gah)
* something I haven't thought of
Thoughts? Thanks
Dave
[...snipped discussion of the other up-thread issue, relating
build_non_dependent_expr...]
