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

[David Malcolm <dmalcolm@redhat.com>]

On Fri, 2017-12-15 at 13:58 -0500, Jason Merrill wrote:
> On Fri, Dec 15, 2017 at 11:35 AM, David Malcolm <dmalcolm@redhat.com>
> wrote:
> > On Fri, 2017-12-15 at 10:01 -0500, Jason Merrill wrote:
> > > On Thu, Dec 14, 2017 at 2:25 PM, David Malcolm <dmalcolm@redhat.c
> > > om>
> > > wrote:
> > > > On Mon, 2017-12-11 at 21:10 -0500, Jason Merrill wrote:
> > > > > On 11/10/2017 04:45 PM, David Malcolm wrote:
> > > > > > The initial version of the patch kit added location wrapper
> > > > > > nodes
> > > > > > around constants and uses-of-declarations, along with some
> > > > > > other
> > > > > > places in the parser (typeid, alignof, sizeof, offsetof).
> > > > > > 
> > > > > > This version takes a much more minimal approach: it only
> > > > > > adds
> > > > > > location wrapper nodes around the arguments at callsites,
> > > > > > thus
> > > > > > not adding wrapper nodes around uses of constants and decls
> > > > > > in
> > > > > > other
> > > > > > locations.
> > > > > > 
> > > > > > It keeps them for the other places in the parser (typeid,
> > > > > > alignof,
> > > > > > sizeof, offsetof).
> > > > > > 
> > > > > > In addition, for now, each site that adds wrapper nodes is
> > > > > > guarded
> > > > > > with !processing_template_decl, suppressing the creation of
> > > > > > wrapper
> > > > > > nodes when processing template declarations.  This is to
> > > > > > simplify
> > > > > > the patch kit so that we don't have to support wrapper
> > > > > > nodes
> > > > > > during
> > > > > > template expansion.
> > > > > 
> > > > > Hmm, it should be easy to support them, since NON_LVALUE_EXPR
> > > > > and
> > > > > VIEW_CONVERT_EXPR don't otherwise appear in template trees.
> > > > > 
> > > > > Jason
> > > > 
> > > > I don't know if it's "easy"; it's at least non-trivial.
> > > > 
> > > > I attempted to support them in the obvious way by adding the
> > > > two
> > > > codes
> > > > to the switch statement tsubst_copy, reusing the case used by
> > > > NOP_EXPR
> > > > and others, but ran into a issue when dealing with template
> > > > parameter
> > > > packs.
> > > > Attached is the reproducer I've been testing with (minimized
> > > > using
> > > > "delta" from a stdlib reproducer); my code was failing with:
> > > > 
> > > > ../../src/cp-stdlib.ii: In instantiation of ‘struct
> > > > allocator_traits<allocator<char> >’:
> > > > ../../src/cp-stdlib.ii:31:8:   required from ‘struct
> > > > __alloc_traits<allocator<char>, char>’
> > > > ../../src/cp-stdlib.ii:43:75:   required from ‘class
> > > > basic_string<char, allocator<char> >’
> > > > ../../src/cp-stdlib.ii:47:58:   required from here
> > > > ../../src/cp-stdlib.ii:27:55: sorry, unimplemented: use of
> > > > ‘type_pack_expansion’ in template
> > > >      -> decltype(_S_construct(__a, __p,
> > > > forward<_Args>(__args)...))  {   }
> > > >                                                        ^~~~~~
> > > > 
> > > > The issue is that normally "__args" would be a PARM_DECL of
> > > > type
> > > > TYPE_PACK_EXPANSION, and that's handled by tsubst_decl, but on
> > > > adding a
> > > > wrapper node we now have a VIEW_CONVERT_EXPR of the same type
> > > > i.e.
> > > > TYPE_PACK_EXPANSION wrapping the PARM_DECL.
> > > > 
> > > > When tsubst traverses the tree, the VIEW_CONVERT_EXPR is
> > > > reached
> > > > first,
> > > > and it attempts to substitute the type TYPE_PACK_EXPANSION,
> > > > which
> > > > leads
> > > > to the "sorry".
> > > > 
> > > > If I understand things right, during substitution, only
> > > > tsubst_decl
> > > > on
> > > > PARM_DECL can handle nodes with type with code
> > > > TYPE_PACK_EXPANSION.
> > > > 
> > > > The simplest approach seems to be to not create wrapper nodes
> > > > for
> > > > decls
> > > > of type TYPE_PACK_EXPANSION, and that seems to fix the issue.
> > > 
> > > That does seem simplest.
> > > 
> > > > Alternatively I can handle TYPE_PACK_EXPANSION for
> > > > VIEW_CONVERT_EXPR in
> > > > tsubst by remapping the type to that of what they wrap after
> > > > substitution; doing so also fixes the issue.
> > > 
> > > This will be more correct.  For the wrappers you don't need all
> > > the
> > > handling that we currently have for NOP_EXPR and such; since we
> > > know
> > > they don't change the type, we can substitute what they wrap, and
> > > then
> > > rewrap the result.
> > 
> > (nods; I have this working)
> > 
> > I've been debugging the other issues that I ran into when removing
> > the
> > "!processing_template_decl" filter on making wrapper nodes (ICEs
> > and
> > other errors on valid code).  They turn out to relate to wrappers
> > around decls of type TEMPLATE_TYPE_PARM; having these wrappers
> > leads to
> > such VIEW_CONVERT_EXPRs turning up in unexpected places.
> 
> Hmm, that's odd.  What kind of decls?  A variable which happens to
> have a template parameter for a type shouldn't be a problem.

That one turned out to be a bug in my changes to tsubst_copy, which
I've fixed.

But I'm still running into various other issues when attempting to
enable the wrappers for when processing_template_decl is true.  Having
spent a fair chunk of the week on it, I'm not finding it "easy to
support them" (though that may be one, of course...).

I'm working on addressing the other issues you raised; I hope to
post some updated patches when I get things bootstrapping again.

Thanks
Dave


> > I could try to track all those places down, but it seems much
> > simpler
> > to just add an exclusion to adding wrapper nodes around decls of
> > type
> > TEMPLATE_TYPE_PARM.  On doing that my smoketests with the C++
> > stdlib
> > work again.  Does that sound reasonable?
> 
> Jason