[FIXED] Generic lambda symbol table bug

Adam Butcher adam@jessamine.co.uk
Wed Aug 7 07:53:00 GMT 2013


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(-)

diff --git a/gcc/cp/lambda.c b/gcc/cp/lambda.c
index c90a27c..5fdc551 100644
--- a/gcc/cp/lambda.c
+++ b/gcc/cp/lambda.c
@@ -915,7 +915,9 @@ maybe_add_lambda_conv_op (tree type)
   finish_compound_stmt (compound_stmt);
   finish_function_body (body);
 
-  expand_or_defer_fn (finish_function (2));
+  fn = finish_function (/*inline*/2);
+  if (!generic_lambda_p)
+    expand_or_defer_fn (fn);
 
   /* Generate the body of the conversion op.  */
 
@@ -931,7 +933,9 @@ maybe_add_lambda_conv_op (tree type)
   finish_compound_stmt (compound_stmt);
   finish_function_body (body);
 
-  expand_or_defer_fn (finish_function (2));
+  fn = finish_function (/*inline*/2);
+  if (!generic_lambda_p)
+    expand_or_defer_fn (fn);
 
   if (nested)
     pop_function_context ();
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 7cd197d..8107ff1 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -9135,7 +9135,11 @@ cp_parser_lambda_body (cp_parser* parser, tree lambda_expr)
     finish_lambda_scope ();
 
     /* Finish the function and generate code for it if necessary.  */
-    expand_or_defer_fn (finish_function (/*inline*/2));
+    tree fn = finish_function (/*inline*/2);
+
+    /* Only expand if the call op is not a template.  */
+    if (!DECL_TEMPLATE_INFO (fco))
+      expand_or_defer_fn (fn);
   }
 
   parser->local_variables_forbidden_p = local_variables_forbidden_p;
diff --git a/gcc/symtab.c b/gcc/symtab.c
index a23a905..d15881b 100644
--- a/gcc/symtab.c
+++ b/gcc/symtab.c
@@ -116,15 +116,6 @@ insert_to_assembler_name_hash (symtab_node node, bool with_clones)
 
       tree name = DECL_ASSEMBLER_NAME (node->symbol.decl);
 
-
-      // FIXME: how does this nullptr get here when declaring a C++
-      // FIXME: generic lambda and including iostream (or presumably
-      // FIXME: any other header with whatever property is triggering
-      // FIXME: this)!?
-      //
-      if (name == 0)
-	return;
-
       aslot = htab_find_slot_with_hash (assembler_name_hash, name,
 					decl_assembler_name_hash (name),
 					INSERT);
@@ -165,15 +156,6 @@ unlink_from_assembler_name_hash (symtab_node node, bool with_clones)
       else
 	{
 	  tree name = DECL_ASSEMBLER_NAME (node->symbol.decl);
-
-	  // FIXME: how does this nullptr get here when declaring a C++
-	  // FIXME: generic lambda and including iostream (or presumably
-	  // FIXME: any other header with whatever property is triggering
-	  // FIXME: this)!?
-	  //
-	  if (name == 0)
-	    return;
-
           void **slot;
 	  slot = htab_find_slot_with_hash (assembler_name_hash, name,
 					   decl_assembler_name_hash (name),
-- 
1.8.3



More information about the Gcc-patches mailing list