[FIXED] Generic lambda symbol table bug

Hi Jason,

On Mon, 05 Aug 2013 17:26:12 -0400, Jason Merrill wrote:
> On 08/04/2013 07:45 PM, Adam Butcher wrote:
> > What should I do about the symtab nullptr issue?
> > (http://gcc.gnu.org/ml/gcc-patches/2013-07/msg00043.html)  Should I
> > leave the workaround in my patch set as a standalone commit to be
> > remedied later or should I try to squash it?  Or is the hack appropriate
> > for some reason?
>
> Let's fix it.
>
> >    0x810670 handle_alias_pairs
> >        ../../gcc/cgraphunit.c:1014
>
> This suggests that somehow the call operator template wound up in
> alias_pairs.  This is very mysterious; I really don't know why there
> would be any aliases in this testcase, much less one involving the
> operator().  The offending code should be pretty straightforward to find
> with a watchpoint on alias_pairs.
>
Turns out that it was the presence of any alias that was triggering the issue.
The call op itself was not in the 'alias_pairs' vector.  It happened because, if
aliases are present, 'handle_alias_pairs' causes an initialization of the
'assembler_name_hash' containing every known symbol.  And for some reason, which
I now understand, the uninstantiated call operator template result declaration
was being added to the symbol table (without an asmname).  The particular
aliases my original program encountered from <iostream> were the gthread ones.
The following program (no includes, no conversion ops, no implicit templates) is
sufficient and minimal to reproduce the issue.

   int dummy() {}
   static decltype(dummy) weak_dummy __attribute__ ((__weakref__("dummy")));

   int main()
   {
     int i;
     [i] <typename T> (T) {};
   }

The patch below fixes it.  But a cleaner way might be to extend the "processing
template declaration" state from lambda declarator all the way to the end of the
lambda body.  This would match with the scenario that occurs with a standard
in-class member function template definition.  To do that elegantly would
require a bit of refactoring of the lambda parser code.

The patch below does the job by avoiding the cause of the erroneous symbol table
entry, 'expand_or_defer_fn', explicitly.

Assuming you're okay with this fix, how do you want me to submit the final
patchset for this stuff?  There are two features here which I was intending to
submit in the following order as two patches:

  - Generic lambdas specified with explicit template parameter list.  This
    teaches GCC how to deal with lambda call op templates (and their conversion
    operators) but does not actually implement the 'generic lambda' spec in
    N3690 5.1.2.5. 

  - Generalized implicit function template support via 'auto params'; a step
    along the road to terse templates.  The necessary propagation into the
    lambda code has a side-effect of supporting 'generic lambdas' as proposed by
    the C++14 draft standard.

I'm not sure which dialect guards to put these features behind though.
Strictly:

  a) Generic lambdas created fully implicitly via 'auto params' should be
     accepted with -std=c++1y and -std=gnu++1y since it is actually spec'd by
     the draft.

  b) Generic lambdas created with an explicit template parameter list should be
     accepted with -std=gnu++1y only.

  c) Generalized implicit function templates should be accepted by -std=gnu++1y
     only.

Due to the generalized implementation of the feature guarded by (c), it may be
messy (or maybe costly) to implement (a).

Cheers,
Adam


----------------- fix for symtab issue ------------------

Don't add generic lambda call ops to the symbol table.

Typically, 'processing_template_decl' is high throughout an in-class member
function template definition, so 'expand_or_defer_fn' normally does nothing.
It is difficult, without considerable refactoring of the current lambda parser
implementation, to mimic the situation that arises with a conventional in-class
member template definition.  Hence, this patch prevents 'expand_or_defer_fn'
from being called for generic lambdas.

The bug was triggered by the presence of at least one alias (such as a weak ref)
causing the assembler name hash to be initialized with every known symbol.  The
call operator template should not be added to the symbol table; only
instantiations of it should.

---
 gcc/cp/lambda.c |  8 ++++++--
 gcc/cp/parser.c |  6 +++++-
 gcc/symtab.c    | 18 ------------------
 3 files changed, 11 insertions(+), 21 deletions(-)

On 08/07/2013 03:52 AM, Adam Butcher wrote:
> But a cleaner way might be to extend the "processing
> template declaration" state from lambda declarator all the way to the end of the
> lambda body.  This would match with the scenario that occurs with a standard
> in-class member function template definition.  To do that elegantly would
> require a bit of refactoring of the lambda parser code.

It isn't already set through the whole lambda body?  That seems 
necessary to support non-trivial lambda bodies; otherwise we won't be 
able to handle dependence properly.

> I'm not sure which dialect guards to put these features behind though.
> Strictly:
>
>   a) Generic lambdas created fully implicitly via 'auto params' should be
>      accepted with -std=c++1y and -std=gnu++1y since it is actually spec'd by
>      the draft.
>
>   b) Generic lambdas created with an explicit template parameter list should be
>      accepted with -std=gnu++1y only.
>
>   c) Generalized implicit function templates should be accepted by -std=gnu++1y
>      only.

This makes sense to me.  Or perhaps add (c) to the concepts lite flag, 
when there is one.

Jason

On 07.08.2013 16:59, Jason Merrill wrote:
> On 08/07/2013 03:52 AM, Adam Butcher wrote:
>> But a cleaner way might be to extend the "processing
>> template declaration" state from lambda declarator all the way to 
>> the end of the
>> lambda body.  This would match with the scenario that occurs with a 
>> standard
>> in-class member function template definition.  To do that elegantly 
>> would
>> require a bit of refactoring of the lambda parser code.
>
> It isn't already set through the whole lambda body?
>
No.  It is within cp_parser_lambda_declarator_opt.

> That seems
> necessary to support non-trivial lambda bodies; otherwise we won't be
> able to handle dependence properly.
>
Agreed.  Okay, I will produce three patches:

   1) Refactor existing monomorphic implementation to give a cleaner
      parser separation of concerns; extract the fco creation and
      provide begin and end for the lambda function operator.

   2) Add explicit lambda template support.

   3) Add implicit function template support.

>> I'm not sure which dialect guards to put these features behind 
>> though.
>> Strictly:
>>
>>   a) Generic lambdas created fully implicitly via 'auto params' 
>> should be
>>      accepted with -std=c++1y and -std=gnu++1y since it is actually 
>> spec'd by
>>      the draft.
>>
>>   b) Generic lambdas created with an explicit template parameter 
>> list should be
>>      accepted with -std=gnu++1y only.
>>
>>   c) Generalized implicit function templates should be accepted by 
>> -std=gnu++1y
>>      only.
>
> This makes sense to me.  Or perhaps add (c) to the concepts lite
> flag, when there is one.
>
Okay.  Will do as originally suggested.  I am intending to try out
constrained implicit function templates on the c++-concepts branch
at some point also (which might already have such a flag).

Cheers,
Adam

On 07.08.2013 20:56, Adam Butcher wrote:
> On 07.08.2013 16:59, Jason Merrill wrote:
>> On 08/07/2013 03:52 AM, Adam Butcher wrote:
>>> But a cleaner way might be to extend the "processing
>>> template declaration" state from lambda declarator all the way to 
>>> the end of the
>>> lambda body.  This would match with the scenario that occurs with a 
>>> standard
>>> in-class member function template definition.  To do that elegantly 
>>> would
>>> require a bit of refactoring of the lambda parser code.
>>
>> It isn't already set through the whole lambda body?
>>
> No.  It is within cp_parser_lambda_declarator_opt.
>
>> That seems
>> necessary to support non-trivial lambda bodies; otherwise we won't 
>> be
>> able to handle dependence properly.
>>
> Agreed.  Okay, I will produce three patches:
>
>   1) Refactor existing monomorphic implementation to give a cleaner
>      parser separation of concerns; extract the fco creation and
>      provide begin and end for the lambda function operator.
>
I did this---well not exactly as written---but I extended the template 
decl state through until the end of the lambda body.  And it works with 
my dependence test.  However, the previous version does too.  The 
following program works fine (conv ops an all) with the refactored 
version and without it.  I'm now questioning whether it is worth making 
any change in this area.  What do you think?  The test code is as 
follows and the resulting program correctly executes S::N::test() 4 
times as expected -- were you thinking of some other dependence case?

   #include <iostream>

   struct S
   {
      struct N
      {
         float test () { std::cout << "S::N::test() -> 7.f\n"; return 
7.f; }
      };
   };

   int main()
   {
      auto f = [] <typename T> (T const& s) {
         typename T::N x;
         return x.test ();
      };
      auto g = [] (auto const& s) {
         typename std::decay<decltype (s)>::type::N x;
         return x.test ();
      };

      S i;

      f(i);
      g(i);

      float (*pfn) (S const&) = f;

      pfn(i);

      pfn = g;

      return pfn(i);
   }

Error messages are reasonable also.  Omitting the necessary 'typename's 
gives:

    : need ‘typename’ before ‘T::N’ because ‘T’ is a dependent scope
    : need ‘typename’ before ‘typename std::decay<decltype 
(s)>::type::N’ because ‘typename std::decay<decltype (s)>::type’ is a 
dependent scope

Passing a local 'struct X {}' instead of 'S' gives:

   : In instantiation of ‘auto main()::<lambda(const T&)> const [with T 
= main()::X]’:
   :24:9:   required from here
   :14:23: error: no type named ‘N’ in ‘struct main()::X’
   ...

So all seems to be okay with both versions.  Any ideas why?

Cheers,
Adam

On 08/08/2013 06:28 PM, Adam Butcher wrote:
> So all seems to be okay with both versions.  Any ideas why?

Hmm, it sounds like processing_template_decl is being set after all, 
even without your change.

Jason

On 09.08.2013 03:01, Jason Merrill wrote:
> On 08/08/2013 06:28 PM, Adam Butcher wrote:
>> So all seems to be okay with both versions.  Any ideas why?
>
> Hmm, it sounds like processing_template_decl is being set after all,
> even without your change.
>
Yup.  Although the lambda template code I originally added scoped it to 
within the the 'cp_parser_lambda_declarator_opt' function, since the 
call op being declared is an in-class inline member, 
'start_preparsed_function' is entering 
'maybe_begin_member_template_processing'.   'finish_function' matches 
that with 'maybe_end_member_template_processing' prior to the function 
being passed to 'expand_or_defer_fn' (which then causes the erroneous 
symtab entry).

So I think my original fix might be okay here; omitting 
'expand_or_defer_fn' if 'generic_lambda_p'.

What do you think?  Shall I just include it as part of the first patch? 
If so that'd just be two patches, "teach GCC to deal with lambda 
templates" and "implicit function templates with auto".

Cheers,
Adam