| Message ID | CAF6KqwWKTXCK=059NuMT7Ge4aBT5aprK4LD5wTmYNrp=Cwj9-A@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
I think what you're looking for is: if (DECL_CLONED_FUNCTION_P (clone) && DECL_CLONED_FUNCTION (clone) == decl) That's a much cleaner implementation. Ollie On Tue, Sep 27, 2011 at 6:18 PM, Delesley Hutchins <delesley@google.com> wrote: > > This patch fixes a bug in the parser which cause an internal compiler > error when copying attributes from cloned methods. The bug occurs > when a class has both an annotated constructor and a template method. > > Bootstrapped and passed gcc regression testsuite on > x86_64-unknown-linux-gnu. Okay for google/gcc-4_6? > > -DeLesley > > cp/Changelog.google-4_6: > 2011-9-27 DeLesley Hutchins <delesley@google.com> > > * cp/parser.c (cp_parser_late_parsing_attribute_arg_lists) > fixed case where the potential clone is a template. > > testsuite/Changelog.google-4_6: > 2011-9-27 DeLesley Hutchins <delesley@google.com> > > * testsuite/g++.dg/thread-ann/thread_annot_lock-81.C > (new regression test) > > -- > DeLesley Hutchins | Software Engineer | delesley@google.com | 505-206-0315
On Thu, Sep 29, 2011 at 10:13 AM, Delesley Hutchins <delesley@google.com> wrote: > > Unfortunately, DECL_CLONED_FUNCTION_P is not actually a predicate that allows you to call DECL_CLONED_FUNCTION safely. Look at the definition of the macros; despite what the comments say, DECL_CLONED_FUNCTION_P may return true in cases where DECL_CLONED_FUNCTION will still crash. The correct fix is to fix the macros, but I have no understanding of what they are actually doing. :-( > -DeLesley Diego, can you comment? Ollie
On Thu, Sep 29, 2011 at 11:26, Ollie Wild <aaw@google.com> wrote: > On Thu, Sep 29, 2011 at 10:13 AM, Delesley Hutchins <delesley@google.com> wrote: >> >> Unfortunately, DECL_CLONED_FUNCTION_P is not actually a predicate that allows you >> to call DECL_CLONED_FUNCTION safely. Look at the definition of the macros; despite >> what the comments say, DECL_CLONED_FUNCTION_P may return true in cases where >> DECL_CLONED_FUNCTION will still crash. The correct fix is to fix the macros, but I >> have no understanding of what they are actually doing. :-( >> -DeLesley > > Diego, can you comment? Really? That's surprising. I would certainly expect DECL_CLONED_FUNCTION_P to be exactly the right predicate to guard DECL_CLONED_FUNCTION with. That's how it's used elsewhere. Delesley, can you give more details on when DECL_CLONED_FUNCTION_P fails? Changing that predicate with: if (DECL_CLONED_FUNCTION_P (clone) && DECL_CLONED_FUNCTION (clone) == decl) should be all you need. If that's not working, then send me the test case, cause I'll be confused :) Diego.
I don't have a test case, but look at the definitions of the two macros in cp/cp-tree.h. DECL_CLONED_FUNCTION_P calls decl_cloned_function_p, passing true as the second argument, while DECL_CLONED_FUNCTION makes the same call, but passes false. Now look at the definition of decl_cloned_function_p in cp/class.c. If the second argument is true, it will step into templates, and if it is false, it won't. Incidentally, the ICE occurs when DECL_CLONED_FUNCTION is applied to a template function, so this is not a hypothetical case. :-) IMHO, it doesn't make sense to me to have a predicate that can potentially succeed on templates, when the macro itself will fail. However, I don't understand how cloned functions work, so I may be missing something here -- perhaps a template function is never a clone, so the predicate still returns NULL in that case? But if so, then why step into templates at all? -DeLesley On Thu, Sep 29, 2011 at 8:42 AM, Diego Novillo <dnovillo@google.com> wrote: > > On Thu, Sep 29, 2011 at 11:26, Ollie Wild <aaw@google.com> wrote: > > On Thu, Sep 29, 2011 at 10:13 AM, Delesley Hutchins <delesley@google.com> wrote: > >> > >> Unfortunately, DECL_CLONED_FUNCTION_P is not actually a predicate that allows you > >> to call DECL_CLONED_FUNCTION safely. Look at the definition of the macros; despite > >> what the comments say, DECL_CLONED_FUNCTION_P may return true in cases where > >> DECL_CLONED_FUNCTION will still crash. The correct fix is to fix the macros, but I > >> have no understanding of what they are actually doing. :-( > >> -DeLesley > > > > Diego, can you comment? > > Really? That's surprising. I would certainly expect > DECL_CLONED_FUNCTION_P to be exactly the right predicate to guard > DECL_CLONED_FUNCTION with. That's how it's used elsewhere. > > Delesley, can you give more details on when DECL_CLONED_FUNCTION_P > fails? Changing that predicate with: > > if (DECL_CLONED_FUNCTION_P (clone) > && DECL_CLONED_FUNCTION (clone) == decl) > > should be all you need. If that's not working, then send me the test > case, cause I'll be confused :) > > > Diego.
On 11-09-29 13:21 , Delesley Hutchins wrote: > I don't have a test case, but look at the definitions of the two > macros in cp/cp-tree.h. DECL_CLONED_FUNCTION_P calls > decl_cloned_function_p, passing true as the second argument, while > DECL_CLONED_FUNCTION makes the same call, but passes false. Now look > at the definition of decl_cloned_function_p in cp/class.c. If the > second argument is true, it will step into templates, and if it is > false, it won't. Incidentally, the ICE occurs when > DECL_CLONED_FUNCTION is applied to a template function, so this is not > a hypothetical case. :-) But notice that STRIP_TEMPLATE is a NOP when DECL is not a TEMPLATE_DECL. So, I'm not sure where you saw it ICE. We are already using this idiom all over the parser, so it would be great if you could produce a test case for the failure you have in mind. Incidentally, I applied the variant of the patch Ollie and I suggested and the testcase works fine with it (while it fails without the patch, of course). Diego.
The ICE occurs when decl is a TEMPLATE_DECL; that's the corner case that causes a problem. The patch that you and Ollie suggest does stop the ICE for our particular example; I assume because the template in question is not a clone, and so the predicate fails further on. However, I'm not convinced that it will work in all cases. I wish I could come up with a test case, but like I said, I don't understand enough about clones to understand what's happening here. If you are confident that DECL_CLONED_FUNCTION_P is correct, then we can use that; I personally had no such confidence. -DeLesley On Thu, Sep 29, 2011 at 10:41 AM, Diego Novillo <dnovillo@google.com> wrote: > On 11-09-29 13:21 , Delesley Hutchins wrote: >> >> I don't have a test case, but look at the definitions of the two >> macros in cp/cp-tree.h. DECL_CLONED_FUNCTION_P calls >> decl_cloned_function_p, passing true as the second argument, while >> DECL_CLONED_FUNCTION makes the same call, but passes false. Now look >> at the definition of decl_cloned_function_p in cp/class.c. If the >> second argument is true, it will step into templates, and if it is >> false, it won't. Incidentally, the ICE occurs when >> DECL_CLONED_FUNCTION is applied to a template function, so this is not >> a hypothetical case. :-) > > But notice that STRIP_TEMPLATE is a NOP when DECL is not a TEMPLATE_DECL. > So, I'm not sure where you saw it ICE. We are already using this idiom all > over the parser, so it would be great if you could produce a test case for > the failure you have in mind. > > Incidentally, I applied the variant of the patch Ollie and I suggested and > the testcase works fine with it (while it fails without the patch, of > course). > > > Diego. >
Index: testsuite/g++.dg/thread-ann/thread_annot_lock-81.C =================================================================== --- testsuite/g++.dg/thread-ann/thread_annot_lock-81.C (revision 0) +++ testsuite/g++.dg/thread-ann/thread_annot_lock-81.C (revision 0) @@ -0,0 +1,17 @@ +// Test template methods in the presence of cloned constructors. +// Regression test for bugfix. +// { dg-do compile } +// { dg-options "-Wthread-safety -O" } + +#include "thread_annot_common.h" + +Mutex mu_; +Mutex mu2_; + +class Foo { + Foo() LOCKS_EXCLUDED(mu_) { } + + template <class T> + void bar(T* t) LOCKS_EXCLUDED(mu2_) { } +}; + Index: cp/parser.c =================================================================== --- cp/parser.c (revision 179283) +++ cp/parser.c (working copy) @@ -19328,7 +19328,8 @@ cp_parser_late_parsing_attribute_arg_lists (cp_par tree clone; for (clone = TREE_CHAIN (decl); clone; clone = TREE_CHAIN (clone)) { - if (DECL_CLONED_FUNCTION (clone) == decl) + tree* clonefun = &DECL_CLONED_FUNCTION (clone); + if (clonefun && *clonefun == decl) DECL_ATTRIBUTES (clone) = DECL_ATTRIBUTES (decl); } }
This patch fixes a bug in the parser which cause an internal compiler error when copying attributes from cloned methods. The bug occurs when a class has both an annotated constructor and a template method. Bootstrapped and passed gcc regression testsuite on x86_64-unknown-linux-gnu. Okay for google/gcc-4_6? -DeLesley cp/Changelog.google-4_6: 2011-9-27 DeLesley Hutchins <delesley@google.com> * cp/parser.c (cp_parser_late_parsing_attribute_arg_lists) fixed case where the potential clone is a template. testsuite/Changelog.google-4_6: 2011-9-27 DeLesley Hutchins <delesley@google.com> * testsuite/g++.dg/thread-ann/thread_annot_lock-81.C (new regression test)