* [PATCH] [Annotalysis] Fix internal compiler error on template methods
@ 2011-09-28 0:10 Delesley Hutchins
2011-09-29 16:19 ` Ollie Wild
0 siblings, 1 reply; 11+ messages in thread
From: Delesley Hutchins @ 2011-09-28 0:10 UTC (permalink / raw)
To: gcc-patches, Diego Novillo, Ollie Wild
[-- Attachment #1: Type: text/plain, Size: 785 bytes --]
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
[-- Attachment #2: gcc_parser_clones.diff --]
[-- Type: text/x-patch, Size: 1233 bytes --]
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);
}
}
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] [Annotalysis] Fix internal compiler error on template methods
2011-09-28 0:10 [PATCH] [Annotalysis] Fix internal compiler error on template methods Delesley Hutchins
@ 2011-09-29 16:19 ` Ollie Wild
[not found] ` <CAF6KqwWg5qUBom7XJrziQTKHrcXfmfqk2q--RZGM_RPZ9JNMfw@mail.gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Ollie Wild @ 2011-09-29 16:19 UTC (permalink / raw)
To: Delesley Hutchins; +Cc: gcc-patches, Diego Novillo
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] [Annotalysis] Fix internal compiler error on template methods
[not found] ` <CAF6KqwWg5qUBom7XJrziQTKHrcXfmfqk2q--RZGM_RPZ9JNMfw@mail.gmail.com>
@ 2011-09-29 17:22 ` Ollie Wild
2011-09-29 17:41 ` Diego Novillo
0 siblings, 1 reply; 11+ messages in thread
From: Ollie Wild @ 2011-09-29 17:22 UTC (permalink / raw)
To: Delesley Hutchins, Diego Novillo; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] [Annotalysis] Fix internal compiler error on template methods
2011-09-29 17:22 ` Ollie Wild
@ 2011-09-29 17:41 ` Diego Novillo
[not found] ` <CAF6KqwX4eCJNMTHUVU8YvgMi58yLSjH_AKFcqQzjdRFQjbTKRA@mail.gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Diego Novillo @ 2011-09-29 17:41 UTC (permalink / raw)
To: Ollie Wild; +Cc: Delesley Hutchins, gcc-patches
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] [Annotalysis] Fix internal compiler error on template methods
[not found] ` <CAF6KqwX4eCJNMTHUVU8YvgMi58yLSjH_AKFcqQzjdRFQjbTKRA@mail.gmail.com>
@ 2011-09-29 19:18 ` Delesley Hutchins
2011-09-29 20:36 ` Diego Novillo
0 siblings, 1 reply; 11+ messages in thread
From: Delesley Hutchins @ 2011-09-29 19:18 UTC (permalink / raw)
To: gcc-patches
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] [Annotalysis] Fix internal compiler error on template methods
2011-09-29 19:18 ` Delesley Hutchins
@ 2011-09-29 20:36 ` Diego Novillo
2011-09-29 21:22 ` Delesley Hutchins
0 siblings, 1 reply; 11+ messages in thread
From: Diego Novillo @ 2011-09-29 20:36 UTC (permalink / raw)
To: Delesley Hutchins, Ollie Wild; +Cc: gcc-patches
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] [Annotalysis] Fix internal compiler error on template methods
2011-09-29 20:36 ` Diego Novillo
@ 2011-09-29 21:22 ` Delesley Hutchins
2011-09-29 21:45 ` Diego Novillo
0 siblings, 1 reply; 11+ messages in thread
From: Delesley Hutchins @ 2011-09-29 21:22 UTC (permalink / raw)
To: Diego Novillo; +Cc: Ollie Wild, gcc-patches
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.
>
--
DeLesley Hutchins | Software Engineer | delesley@google.com | 505-206-0315
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] [Annotalysis] Fix internal compiler error on template methods
2011-09-29 21:22 ` Delesley Hutchins
@ 2011-09-29 21:45 ` Diego Novillo
2011-09-30 0:40 ` Delesley Hutchins
0 siblings, 1 reply; 11+ messages in thread
From: Diego Novillo @ 2011-09-29 21:45 UTC (permalink / raw)
To: Delesley Hutchins; +Cc: Ollie Wild, gcc-patches
On 11-09-29 15:19 , Delesley Hutchins wrote:
> 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.
Yes, there is no case in which a TEMPLATE_DECL will match
DECL_CLONED_FUNCTION_P. I agree with you that the logic in
decl_cloned_function_p seems like it may allow that, but clones of
functions are created as FUNCTION_DECLs.
In looking at the code again, I think we could even simplify it a bit
more using FOR_EACH_CLONE. This does the same work as the if()/for()
combination in a more compact way.
Diego.
Index: cp/parser.c
===================================================================
--- cp/parser.c (revision 179065)
+++ cp/parser.c (working copy)
@@ -19279,6 +19279,7 @@ cp_parser_late_parsing_attribute_arg_lis
cp_token_cache *tokens = (cp_token_cache *) TREE_VALUE
(artificial_node);
tree ctype;
VEC(tree,gc) *vec;
+ tree clone;
gcc_assert (tokens);
gcc_assert (decl && decl != error_mark_node);
@@ -19322,16 +19323,9 @@ cp_parser_late_parsing_attribute_arg_lis
/* If decl has clones (when it is a ctor or a dtor), we need to
modify the clones' attributes as well. */
- if (TREE_CODE (decl) == FUNCTION_DECL
- && (DECL_CONSTRUCTOR_P (decl) || DECL_DESTRUCTOR_P (decl)))
- {
- tree clone;
- for (clone = TREE_CHAIN (decl); clone; clone = TREE_CHAIN
(clone))
- {
- if (DECL_CLONED_FUNCTION (clone) == decl)
- DECL_ATTRIBUTES (clone) = DECL_ATTRIBUTES (decl);
- }
- }
+ FOR_EACH_CLONE (clone, decl)
+ if (DECL_CLONED_FUNCTION (clone) == decl)
+ DECL_ATTRIBUTES (clone) = DECL_ATTRIBUTES (decl);
pop_nested_class ();
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] [Annotalysis] Fix internal compiler error on template methods
2011-09-29 21:45 ` Diego Novillo
@ 2011-09-30 0:40 ` Delesley Hutchins
2011-09-30 1:13 ` Diego Novillo
0 siblings, 1 reply; 11+ messages in thread
From: Delesley Hutchins @ 2011-09-30 0:40 UTC (permalink / raw)
To: Diego Novillo; +Cc: Ollie Wild, gcc-patches
That seems much cleaner! LGTM. Are you submitting the patch? I can
submit the test case.
-DeLesley
On Thu, Sep 29, 2011 at 1:50 PM, Diego Novillo <dnovillo@google.com> wrote:
> On 11-09-29 15:19 , Delesley Hutchins wrote:
>
>> 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.
>
> Yes, there is no case in which a TEMPLATE_DECL will match
> DECL_CLONED_FUNCTION_P. I agree with you that the logic in
> decl_cloned_function_p seems like it may allow that, but clones of functions
> are created as FUNCTION_DECLs.
>
> In looking at the code again, I think we could even simplify it a bit more
> using FOR_EACH_CLONE. This does the same work as the if()/for() combination
> in a more compact way.
>
>
> Diego.
>
> Index: cp/parser.c
> ===================================================================
> --- cp/parser.c (revision 179065)
> +++ cp/parser.c (working copy)
> @@ -19279,6 +19279,7 @@ cp_parser_late_parsing_attribute_arg_lis
> cp_token_cache *tokens = (cp_token_cache *) TREE_VALUE
> (artificial_node);
> tree ctype;
> VEC(tree,gc) *vec;
> + tree clone;
>
> gcc_assert (tokens);
> gcc_assert (decl && decl != error_mark_node);
> @@ -19322,16 +19323,9 @@ cp_parser_late_parsing_attribute_arg_lis
>
> /* If decl has clones (when it is a ctor or a dtor), we need to
> modify the clones' attributes as well. */
> - if (TREE_CODE (decl) == FUNCTION_DECL
> - && (DECL_CONSTRUCTOR_P (decl) || DECL_DESTRUCTOR_P (decl)))
> - {
> - tree clone;
> - for (clone = TREE_CHAIN (decl); clone; clone = TREE_CHAIN
> (clone))
> - {
> - if (DECL_CLONED_FUNCTION (clone) == decl)
> - DECL_ATTRIBUTES (clone) = DECL_ATTRIBUTES (decl);
> - }
> - }
> + FOR_EACH_CLONE (clone, decl)
> + if (DECL_CLONED_FUNCTION (clone) == decl)
> + DECL_ATTRIBUTES (clone) = DECL_ATTRIBUTES (decl);
>
> pop_nested_class ();
>
>
--
DeLesley Hutchins | Software Engineer | delesley@google.com | 505-206-0315
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] [Annotalysis] Fix internal compiler error on template methods
2011-09-30 0:40 ` Delesley Hutchins
@ 2011-09-30 1:13 ` Diego Novillo
2011-09-30 1:14 ` Delesley Hutchins
0 siblings, 1 reply; 11+ messages in thread
From: Diego Novillo @ 2011-09-30 1:13 UTC (permalink / raw)
To: Delesley Hutchins; +Cc: Ollie Wild, gcc-patches
On 11-09-29 17:24 , Delesley Hutchins wrote:
> That seems much cleaner! LGTM. Are you submitting the patch? I can
> submit the test case.
I tested it on an unclean tree, sorry. I'd rather leave the testing and
final submit to you.
Thanks. Diego.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] [Annotalysis] Fix internal compiler error on template methods
2011-09-30 1:13 ` Diego Novillo
@ 2011-09-30 1:14 ` Delesley Hutchins
0 siblings, 0 replies; 11+ messages in thread
From: Delesley Hutchins @ 2011-09-30 1:14 UTC (permalink / raw)
To: Diego Novillo; +Cc: Ollie Wild, gcc-patches
No problem. Will do.
-DeLesley
On Thu, Sep 29, 2011 at 2:44 PM, Diego Novillo <dnovillo@google.com> wrote:
> On 11-09-29 17:24 , Delesley Hutchins wrote:
>>
>> That seems much cleaner! LGTM. Are you submitting the patch? I can
>> submit the test case.
>
> I tested it on an unclean tree, sorry. I'd rather leave the testing and
> final submit to you.
>
>
> Thanks. Diego.
>
--
DeLesley Hutchins | Software Engineer | delesley@google.com | 505-206-0315
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2011-09-29 21:49 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-28 0:10 [PATCH] [Annotalysis] Fix internal compiler error on template methods Delesley Hutchins
2011-09-29 16:19 ` Ollie Wild
[not found] ` <CAF6KqwWg5qUBom7XJrziQTKHrcXfmfqk2q--RZGM_RPZ9JNMfw@mail.gmail.com>
2011-09-29 17:22 ` Ollie Wild
2011-09-29 17:41 ` Diego Novillo
[not found] ` <CAF6KqwX4eCJNMTHUVU8YvgMi58yLSjH_AKFcqQzjdRFQjbTKRA@mail.gmail.com>
2011-09-29 19:18 ` Delesley Hutchins
2011-09-29 20:36 ` Diego Novillo
2011-09-29 21:22 ` Delesley Hutchins
2011-09-29 21:45 ` Diego Novillo
2011-09-30 0:40 ` Delesley Hutchins
2011-09-30 1:13 ` Diego Novillo
2011-09-30 1:14 ` Delesley Hutchins
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).