| Message ID | AANLkTimKfTGX1VZ-UO5bhknCubqqgESu5WrS6z4F1Anz@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 22/12/2010 13:25, Kai Tietz wrote: > PR c++/15774 > * c-family/c-pretty-print.c: Add target.h include. > (pp_c_specifier_qualifier_list): Call > pp_c_attributes_calling_convention for (*). > (pp_c_attributes_calling_convention): New. > * c-family/c-pretty-print.h (pp_c_attributes_calling_convention): > New prototype. > * cp/error.c (dump_type_prefix): Call > pp_c_attributes_calling_convention for (*). > * config/i386/i386.c (ix86_attribute_affects_calling_convention): > New hook. > (TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION): Define. > * doc/tm.texi.in (TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION): Add. > * doc/tm.texi: Regenerated. You've omitted the diffs for target.def et. al. cheers, DaveK
On Wed, 22 Dec 2010, Kai Tietz wrote: > Hi, this patch fixes the bug described at > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15774 in bugzilla. Could you please explain more about how and why it fixes the bug? The bug described there is: The C++ frontend doesn't always call targetm.compare_type_attributes when comparing function decls in decl.c: decls_match. As a result conflicting decls are not diagnosed. But your patch doesn't change the compatibility testing at all; it just changes how things are printed. Is there code somewhere that generates strings for type names and compares those strings to determine compatibility? If so, where? If not, how does this patch fix the bug? In any case, the patch does not include testcases, and failure to diagnose invalid code, or bad diagnostics for such code, are exactly the sort of things that should be easy to test in the testsuite, so it's not OK without testcases or proper explanation of why they are hard, and should not have been approved without such testcases. For compatibility testing, targetm.comp_type_attributes is the right thing to use; no new hook should be needed. For printing, I don't see the need for a new hook either. Why is it appropriate to print only attributes that pass the new hook? Shouldn't you rather be printing all attributes, regardless of whether they affect the calling convention, since they are all part of the type in GNU terms? If it is appropriate to condition printing on that condition, it would seem that you should share code between the compatibility testing (ix86_comp_type_attributes) and your new hook rather than duplicating lists of attributes with type compatibility issues. Why does this patch not share code like that?
On Wed, Dec 22, 2010 at 10:31 AM, Joseph S. Myers <joseph@codesourcery.com> wrote: > On Wed, 22 Dec 2010, Kai Tietz wrote: > >> Hi, this patch fixes the bug described at >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15774 in bugzilla. > > Could you please explain more about how and why it fixes the bug? The bug > described there is: > > The C++ frontend doesn't always call targetm.compare_type_attributes > when comparing function decls in decl.c: decls_match. As a result > conflicting decls are not diagnosed. > > But your patch doesn't change the compatibility testing at all; it just > changes how things are printed. Is there code somewhere that generates > strings for type names and compares those strings to determine > compatibility? I don't think the patch actually implement compatibility checking. What it does is to print the conflicting attributes during diagnostic report. And that is something that needed to be fixed. Also, I am skeptical that we should print all attributes. I think the actual code generation stuff is orthogonal. -- Gaby
2010/12/22 Joseph S. Myers <joseph@codesourcery.com>: > On Wed, 22 Dec 2010, Kai Tietz wrote: > >> Hi, this patch fixes the bug described at >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15774 in bugzilla. > > Could you please explain more about how and why it fixes the bug? The bug > described there is: > > The C++ frontend doesn't always call targetm.compare_type_attributes > when comparing function decls in decl.c: decls_match. As a result > conflicting decls are not diagnosed. > > But your patch doesn't change the compatibility testing at all; it just > changes how things are printed. Is there code somewhere that generates > strings for type names and compares those strings to determine > compatibility? If so, where? If not, how does this patch fix the bug? > In any case, the patch does not include testcases, and failure to diagnose > invalid code, or bad diagnostics for such code, are exactly the sort of > things that should be easy to test in the testsuite, so it's not OK > without testcases or proper explanation of why they are hard, and should > not have been approved without such testcases. > > For compatibility testing, targetm.comp_type_attributes is the right thing > to use; no new hook should be needed. For printing, I don't see the need > for a new hook either. Why is it appropriate to print only attributes > that pass the new hook? Shouldn't you rather be printing all attributes, > regardless of whether they affect the calling convention, since they are > all part of the type in GNU terms? > > If it is appropriate to condition printing on that condition, it would > seem that you should share code between the compatibility testing > (ix86_comp_type_attributes) and your new hook rather than duplicating > lists of attributes with type compatibility issues. Why does this patch > not share code like that? > > -- > Joseph S. Myers > joseph@codesourcery.com > Well, a test is in PR, I can one explicit here, nevertheless we should have for this already enough tests in testsuite. For the rest of your reply, I can see a relation to the subject of the patch (and PR). Regards, Kai
On Wed, 22 Dec 2010, Kai Tietz wrote: > Well, a test is in PR, I can one explicit here, nevertheless we should > have for this already enough tests in testsuite. What existing test in the testsuite fails before the patch and passes after it, then? Among other things, the testsuite should verify that previous bugs do not reappear. That means that if you don't fix an existing failing test, you should add a new one, that fails without the patch applied but passes once it is applied. And there should be tests covering as much of the patch as possible; if you remove the new handling for any one of the attributes there, that should cause a test failure. > For the rest of your reply, I can see a relation to the subject of the > patch (and PR). Then please answer my points. How does your patch cause the results on a testcase to change from "no diagnostic" to "diagnostic"? (If it doesn't do so, please state the exact testcase, what the diagnostics are before the patch, what they are afterwards, and why the new version is superior.) Why do you need a new hook at all? Why are you duplicating code already present in the x86 back end that knows about what attributes are ABI attributes relevant to compatibility?
2010/12/22 Joseph S. Myers <joseph@codesourcery.com>: > On Wed, 22 Dec 2010, Kai Tietz wrote: > >> Well, a test is in PR, I can one explicit here, nevertheless we should >> have for this already enough tests in testsuite. > > What existing test in the testsuite fails before the patch and passes > after it, then? > > Among other things, the testsuite should verify that previous bugs do not > reappear. That means that if you don't fix an existing failing test, you > should add a new one, that fails without the patch applied but passes once > it is applied. And there should be tests covering as much of the patch as > possible; if you remove the new handling for any one of the attributes > there, that should cause a test failure. > >> For the rest of your reply, I can see a relation to the subject of the >> patch (and PR). > > Then please answer my points. How does your patch cause the results on a > testcase to change from "no diagnostic" to "diagnostic"? (If it doesn't > do so, please state the exact testcase, what the diagnostics are before > the patch, what they are afterwards, and why the new version is superior.) > Why do you need a new hook at all? Why are you duplicating code already > present in the x86 back end that knows about what attributes are ABI > attributes relevant to compatibility? > > -- > Joseph S. Myers > joseph@codesourcery.com > As this patch fixes a bug (as describe in the PR) in diagnostic output It doesn't change anything on the diagnostic logic of C, or C++. As we usually (and this is a good habit) not checking the type output in testsuite, I expect here no regression. Nevertheless I can add one explicit checking complete error/warning message for C. ABI attributes changing calling-convention. This makes asignments of function pointers declaring different calling conventions (like for i386 regparm, stdcall, fastcall, thiscall, cdecl, sysv_abi, ms_abi) incompatible. This is (or should be) diagnosed by FE, as it is already. But the display of such a warning/error lacks those attribute information, why check failed. See following example: t.c: int foo(void); int bar(int (__attribute__ ((regparm(3))) *arg)(void)); int doo(void) { return bar(foo); } and simply compile it by gcc -c t.c You will see that the warning message produced looks like that (without the patch): t.c: In function 'doo': t.c:5:3: warning: passing argument 1 of 'bar' from incompatible pointer type [enabled by default] t.c:2:5: note: expected 'int (*)(void)' but argument is of type 'int (*)(void)' The patch I sent simply makes displayed message understandable why 'int (*)(void)' isn't 'int (*)(void)' by displaying the callabi attributes, which are actual the cause for the warning. Regards, Kai
2010/12/22 Kai Tietz <ktietz70@googlemail.com>: > 2010/12/22 Joseph S. Myers <joseph@codesourcery.com>: >> On Wed, 22 Dec 2010, Kai Tietz wrote: >> >>> Well, a test is in PR, I can one explicit here, nevertheless we should >>> have for this already enough tests in testsuite. >> >> What existing test in the testsuite fails before the patch and passes >> after it, then? >> >> Among other things, the testsuite should verify that previous bugs do not >> reappear. That means that if you don't fix an existing failing test, you >> should add a new one, that fails without the patch applied but passes once >> it is applied. And there should be tests covering as much of the patch as >> possible; if you remove the new handling for any one of the attributes >> there, that should cause a test failure. >> >>> For the rest of your reply, I can see a relation to the subject of the >>> patch (and PR). >> >> Then please answer my points. How does your patch cause the results on a >> testcase to change from "no diagnostic" to "diagnostic"? (If it doesn't >> do so, please state the exact testcase, what the diagnostics are before >> the patch, what they are afterwards, and why the new version is superior.) >> Why do you need a new hook at all? Why are you duplicating code already >> present in the x86 back end that knows about what attributes are ABI >> attributes relevant to compatibility? >> >> -- >> Joseph S. Myers >> joseph@codesourcery.com >> > > As this patch fixes a bug (as describe in the PR) in diagnostic output > It doesn't change anything on the diagnostic logic of C, or C++. As > we usually (and this is a good habit) not checking the type output in > testsuite, I expect here no regression. Nevertheless I can add one > explicit checking complete error/warning message for C. > ABI attributes changing calling-convention. This makes asignments of > function pointers declaring different calling conventions (like for > i386 regparm, stdcall, fastcall, thiscall, cdecl, sysv_abi, ms_abi) > incompatible. This is (or should be) diagnosed by FE, as it is > already. But the display of such a warning/error lacks those attribute > information, why check failed. > > See following example: > t.c: > int foo(void); > int bar(int (__attribute__ ((regparm(3))) *arg)(void)); > int doo(void) > { > return bar(foo); > } > > and simply compile it by gcc -c t.c > > You will see that the warning message produced looks like that > (without the patch): > t.c: In function 'doo': > t.c:5:3: warning: passing argument 1 of 'bar' from incompatible > pointer type [enabled by default] > t.c:2:5: note: expected 'int (*)(void)' but argument is of type 'int (*)(void)' > > The patch I sent simply makes displayed message understandable why > 'int (*)(void)' isn't 'int (*)(void)' by displaying the callabi > attributes, which are actual the cause for the warning. > > Regards, > Kai > > -- > | (\_/) This is Bunny. Copy and paste > | (='.'=) Bunny into your signature to help > | (")_(") him gain world domination > PS: Use here 32-bit to get warnings displayed. Regards, Kai
On 12/22/2010 11:31 AM, Joseph S. Myers wrote: > For compatibility testing, targetm.comp_type_attributes is the right thing > to use; no new hook should be needed. For printing, I don't see the need > for a new hook either. Why is it appropriate to print only attributes > that pass the new hook? Shouldn't you rather be printing all attributes, > regardless of whether they affect the calling convention, since they are > all part of the type in GNU terms? I'm inclined to agree. Jason
On Wed, 22 Dec 2010, Kai Tietz wrote: > As this patch fixes a bug (as describe in the PR) in diagnostic output I see nothing in PR 15774 that is relevant to this patch or the bug you now describe. Have you given the wrong PR number? That PR is, again, "Conflicting function decls not diagnosed". It's not about bad or confusing diagnostics; it's about missing diagnostics. > t.c:2:5: note: expected 'int (*)(void)' but argument is of type 'int (*)(void)' That's a bug, but not 15774. Please try again, resubmitting the patch under an accurate subject with more attention to writing it up accurately; every hour you spend on writing up patches carefully and accurately saves much more time for hundreds of readers. You don't appear to have addressed my point that there should be one and only one point in the x86 back end that knows the list of ABI attributes; not duplicate lists in your new hook and in ix86_comp_type_attributes.
2010/12/22 Jason Merrill <jason@redhat.com>: > On 12/22/2010 11:31 AM, Joseph S. Myers wrote: >> >> For compatibility testing, targetm.comp_type_attributes is the right thing >> to use; no new hook should be needed. For printing, I don't see the need >> for a new hook either. Why is it appropriate to print only attributes >> that pass the new hook? Shouldn't you rather be printing all attributes, >> regardless of whether they affect the calling convention, since they are >> all part of the type in GNU terms? > > I'm inclined to agree. > > Jason > Well, I talked yesterday to Ian about that, and I thought about adding the attribute print even for tree-pretty-printer. Ian's said, that he would accept here the printing of calling-convention attributes, but he was strict against displaying all type attributes. So I added a this hook for checking this, as obviously the comp_type_attributes hook isn't usable for making this decission. Of course, if everybody agrees, I can change the patch so that it displays all attributes. Regards, Kai
2010/12/22 Kai Tietz <ktietz70@googlemail.com>: > 2010/12/22 Jason Merrill <jason@redhat.com>: >> On 12/22/2010 11:31 AM, Joseph S. Myers wrote: >>> >>> For compatibility testing, targetm.comp_type_attributes is the right thing >>> to use; no new hook should be needed. For printing, I don't see the need >>> for a new hook either. Why is it appropriate to print only attributes >>> that pass the new hook? Shouldn't you rather be printing all attributes, >>> regardless of whether they affect the calling convention, since they are >>> all part of the type in GNU terms? >> >> I'm inclined to agree. >> >> Jason >> > > Well, I talked yesterday to Ian about that, and I thought about adding > the attribute print even for tree-pretty-printer. Ian's said, that he > would accept here the printing of calling-convention attributes, but > he was strict against displaying all type attributes. So I added a > this hook for checking this, as obviously the comp_type_attributes > hook isn't usable for making this decission. Of course, if everybody > agrees, I can change the patch so that it displays all attributes. > > Regards, > Kai Well, I notice that I did a pasto and used wrong PR number. Sorry for that. It is http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12171 I meant.
2010/12/22 Kai Tietz <ktietz70@googlemail.com>: > 2010/12/22 Kai Tietz <ktietz70@googlemail.com>: >> 2010/12/22 Jason Merrill <jason@redhat.com>: >>> On 12/22/2010 11:31 AM, Joseph S. Myers wrote: >>>> >>>> For compatibility testing, targetm.comp_type_attributes is the right thing >>>> to use; no new hook should be needed. For printing, I don't see the need >>>> for a new hook either. Why is it appropriate to print only attributes >>>> that pass the new hook? Shouldn't you rather be printing all attributes, >>>> regardless of whether they affect the calling convention, since they are >>>> all part of the type in GNU terms? >>> >>> I'm inclined to agree. >>> >>> Jason >>> >> >> Well, I talked yesterday to Ian about that, and I thought about adding >> the attribute print even for tree-pretty-printer. Ian's said, that he >> would accept here the printing of calling-convention attributes, but >> he was strict against displaying all type attributes. So I added a >> this hook for checking this, as obviously the comp_type_attributes >> hook isn't usable for making this decission. Of course, if everybody >> agrees, I can change the patch so that it displays all attributes. >> >> Regards, >> Kai > > Well, I notice that I did a pasto and used wrong PR number. Sorry for > that. It is http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12171 I meant. > I am working on that, too. But this (PR/15774) problem is nothing for stage 3 phase AFAICS. It would need to handle the diagnostic of the comp_type_attributes-hook more strictly and add the missing places for diagnostics. Sorry, Kai
On Wed, Dec 22, 2010 at 12:03 PM, Kai Tietz <ktietz70@googlemail.com> wrote: > Well, I talked yesterday to Ian about that, and I thought about adding > the attribute print even for tree-pretty-printer. Ian's said, that he > would accept here the printing of calling-convention attributes, but > he was strict against displaying all type attributes. I strongly agree with that. -- Gaby
Index: gcc/gcc/c-family/c-pretty-print.c =================================================================== --- gcc.orig/gcc/c-family/c-pretty-print.c 2010-12-22 13:43:53.651439000 +0100 +++ gcc/gcc/c-family/c-pretty-print.c 2010-12-22 14:04:32.807689000 +0100 @@ -29,6 +29,7 @@ along with GCC; see the file COPYING3. #include "tree-pretty-print.h" #include "tree-iterator.h" #include "diagnostic.h" +#include "target.h" /* Translate if being used for diagnostics, but not for dump files or __PRETTY_FUNCTION. */ @@ -460,6 +461,7 @@ pp_c_specifier_qualifier_list (c_pretty_ { pp_c_whitespace (pp); pp_c_left_paren (pp); + pp_c_attributes_calling_convention (pp, TYPE_ATTRIBUTES (pointee)); } else if (!c_dialect_cxx ()) pp_c_whitespace (pp); @@ -790,6 +792,45 @@ pp_c_attributes (c_pretty_printer *pp, t pp_c_right_paren (pp); } +/* Pretty-print ATTRIBUTES using GNU C extension syntax for calling + convention affecting attributes. */ + +void +pp_c_attributes_calling_convention (c_pretty_printer *pp, tree a) +{ + int is_first = 1; + + if (a == NULL_TREE) + return; + + for (; a != NULL_TREE; a = TREE_CHAIN (a)) + { + if (!targetm.attribute_affects_calling_convention (TREE_PURPOSE (a))) + continue; + if (is_first) + { + pp_c_ws_string (pp, "__attribute__"); + pp_c_left_paren (pp); + pp_c_left_paren (pp); + is_first = 0; + } + else + { + pp_separate_with (pp, ','); + } + pp_tree_identifier (pp, TREE_PURPOSE (a)); + if (TREE_VALUE (a)) + pp_c_call_argument_list (pp, TREE_VALUE (a)); + } + + if (!is_first) + { + pp_c_right_paren (pp); + pp_c_right_paren (pp); + pp_c_whitespace (pp); + } +} + /* function-definition: declaration-specifiers declarator compound-statement */ Index: gcc/gcc/cp/error.c =================================================================== --- gcc.orig/gcc/cp/error.c 2010-12-22 13:43:53.681439000 +0100 +++ gcc/gcc/cp/error.c 2010-12-22 14:05:38.526439000 +0100 @@ -661,6 +661,8 @@ dump_type_prefix (tree t, int flags) { pp_cxx_whitespace (cxx_pp); pp_cxx_left_paren (cxx_pp); + pp_c_attributes_calling_convention (pp_c_base (cxx_pp), + TYPE_ATTRIBUTES (sub)); } if (TREE_CODE (t) == POINTER_TYPE) pp_character(cxx_pp, '*'); Index: gcc/gcc/c-family/c-pretty-print.h =================================================================== --- gcc.orig/gcc/c-family/c-pretty-print.h 2010-12-22 13:43:53.676439000 +0100 +++ gcc/gcc/c-family/c-pretty-print.h 2010-12-22 13:45:49.479564000 +0100 @@ -176,6 +176,7 @@ void pp_c_space_for_pointer_operator (c_ void pp_c_tree_decl_identifier (c_pretty_printer *, tree); void pp_c_function_definition (c_pretty_printer *, tree); void pp_c_attributes (c_pretty_printer *, tree); +void pp_c_attributes_calling_convention (c_pretty_printer *, tree); void pp_c_cv_qualifiers (c_pretty_printer *pp, int qualifiers, bool func_type); void pp_c_type_qualifier_list (c_pretty_printer *, tree); void pp_c_parameter_type_list (c_pretty_printer *, tree); Index: gcc/gcc/config/i386/i386.c =================================================================== --- gcc.orig/gcc/config/i386/i386.c 2010-12-22 13:43:53.679439000 +0100 +++ gcc/gcc/config/i386/i386.c 2010-12-22 14:19:21.091890600 +0100 @@ -5116,6 +5116,46 @@ ix86_function_ok_for_sibcall (tree decl, return true; } +static bool +ix86_attribute_affects_calling_convention (tree name) +{ + int ident_len; + const char *p; + + gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE); + p = IDENTIFIER_POINTER (name); + ident_len = IDENTIFIER_LENGTH (name); + + if (ident_len > 4 && p[0] == '_' && p[1] == '_' && p[ident_len - 1] == '_' + && p[ident_len - 2] == '_') + { + ident_len -= 4; + p += 2; + } + switch (ident_len) + { + case 5: + if (!strncmp (p, "cdecl", 5)) + return true; + break; + case 6: + if (!strncmp (p, "ms_abi", 6)) + return true; + break; + case 7: + if (!strncmp (p, "regparm", 7) || !strncmp (p, "stdcall", 7)) + return true; + break; + case 8: + if (!strncmp (p, "thiscall", 8) || !strncmp (p, "fastcall", 8) + || !strncmp (p, "sysv_abi", 8)) + return true; + break; + } + + return false; +} + /* Handle "cdecl", "stdcall", "fastcall", "regparm", "thiscall", and "sseregparm" calling convention attributes; arguments as in struct attribute_spec.handler. */ @@ -34813,6 +34853,10 @@ ix86_autovectorize_vector_sizes (void) #undef TARGET_CONDITIONAL_REGISTER_USAGE #define TARGET_CONDITIONAL_REGISTER_USAGE ix86_conditional_register_usage +#undef TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION +#define TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION \ + ix86_attribute_affects_calling_convention + struct gcc_target targetm = TARGET_INITIALIZER; #include "gt-i386.h" Index: gcc/gcc/doc/tm.texi =================================================================== --- gcc.orig/gcc/doc/tm.texi 2010-12-22 13:43:53.683439000 +0100 +++ gcc/gcc/doc/tm.texi 2010-12-22 14:18:28.230485800 +0100 @@ -9752,6 +9752,10 @@ when this is needed are when one attribu attribute is nullified by a subsequent definition. This function may call @code{merge_attributes} to handle machine-independent merging. +@deftypefn {Target Hook} bool TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION (const_tree @var{name}) +Returns @code{true} if given attribute by its @var{name} affects calling convention, otherwise @code{false}. +@end deftypefn + @findex TARGET_DLLIMPORT_DECL_ATTRIBUTES If the only target-specific handling you require is @samp{dllimport} for Microsoft Windows targets, you should define the macro Index: gcc/gcc/doc/tm.texi.in =================================================================== --- gcc.orig/gcc/doc/tm.texi.in 2010-12-22 13:43:53.000000000 +0100 +++ gcc/gcc/doc/tm.texi.in 2010-12-22 14:16:28.788399400 +0100 @@ -9716,6 +9716,8 @@ when this is needed are when one attribu attribute is nullified by a subsequent definition. This function may call @code{merge_attributes} to handle machine-independent merging. +@hook TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION + @findex TARGET_DLLIMPORT_DECL_ATTRIBUTES If the only target-specific handling you require is @samp{dllimport} for Microsoft Windows targets, you should define the macro