From patchwork Wed Aug 7 07:52:36 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adam Butcher X-Patchwork-Id: 265387 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "localhost", Issuer "www.qmailtoaster.com" (not verified)) by ozlabs.org (Postfix) with ESMTPS id 9620C2C015D for ; Wed, 7 Aug 2013 17:53:45 +1000 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id:in-reply-to:references; q=dns; s= default; b=hKybzfBg2/0X83ZLUwexGA57rfUYI2qhIxdTdY2oFFsp+mVYhEyR1 YjXE7zgUYoxA9vZbJ/4WDboqsKr7ws2aaQ/UYTVkUs8N/BCcqUNGeSJqSJmk/Obr 8m6XV2v+YL2LSVDyFY8bY59qdb+liGiypMTlpPcY8pCVeKyecGK9TA= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id:in-reply-to:references; s= default; bh=PT5+sjzobKGMjCVGTkT0grgIgJM=; b=UUJWbobRjnGTlhJgy1Bi KO+6DrkdnVEA4IbtZEhFR1+M0tG/f+wnEXUJL8/j40BcO5B6jmMx5+x+rHTAWTmL rXRAO4ym7XaOP4FwQzIGuwkaHq04CwV7RdP5Za6q/PrymJqeTZEU15wsnGOxoEEh olmynIJ7UkKKH4J/j6yfwLs= Received: (qmail 13013 invoked by alias); 7 Aug 2013 07:53:38 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 13001 invoked by uid 89); 7 Aug 2013 07:53:37 -0000 X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, KHOP_THREADED, RCVD_IN_DNSWL_LOW, RDNS_NONE, SPF_PASS autolearn=ham version=3.3.1 Received: from Unknown (HELO mail-wi0-f177.google.com) (209.85.212.177) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 07 Aug 2013 07:53:07 +0000 Received: by mail-wi0-f177.google.com with SMTP id hq12so1345292wib.4 for ; Wed, 07 Aug 2013 00:52:58 -0700 (PDT) X-Received: by 10.194.110.39 with SMTP id hx7mr1417300wjb.4.1375861978821; Wed, 07 Aug 2013 00:52:58 -0700 (PDT) Received: from localhost.localdomain (munkyhouse.force9.co.uk. [84.92.244.81]) by mx.google.com with ESMTPSA id nb12sm7219834wic.3.2013.08.07.00.52.57 for (version=TLSv1.1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 07 Aug 2013 00:52:58 -0700 (PDT) From: Adam Butcher To: Jason Merrill Cc: gcc-patches@gcc.gnu.org, Gabriel Dos Reis , Andrew Sutton , Adam Butcher Subject: Re: [FIXED] Generic lambda symbol table bug Date: Wed, 7 Aug 2013 08:52:36 +0100 Message-Id: <1375861956-11593-1-git-send-email-adam@jessamine.co.uk> In-Reply-To: <52001874.5050105@redhat.com> References: <52001874.5050105@redhat.com> 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 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] (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),