Btw, I've filed http://llvm.org/bugs/show_bug.cgi?id=20679 to track the issue further.
~Aaron On Fri, Aug 15, 2014 at 2:45 PM, Aaron Ballman <[email protected]> wrote: > On Fri, Aug 15, 2014 at 1:55 PM, Abramo Bagnara > <[email protected]> wrote: >> Il 15/08/2014 16:51, Aaron Ballman ha scritto: >>> On Fri, Aug 15, 2014 at 10:01 AM, Abramo Bagnara >>> <[email protected]> wrote: >>>> Il 15/08/2014 15:46, Aaron Ballman ha scritto: >>>>> On Fri, Aug 15, 2014 at 9:04 AM, Abramo Bagnara >>>>> <[email protected]> wrote: >>>>>> Il 15/08/2014 13:51, Aaron Ballman ha scritto: >>>>>>> On Fri, Aug 15, 2014 at 4:10 AM, Abramo Bagnara >>>>>>> <[email protected]> wrote: >>>>>>>> Il 07/08/2014 15:42, Aaron Ballman ha scritto: >>>>>>>>> On Thu, Aug 7, 2014 at 9:30 AM, Abramo Bagnara >>>>>>>>> <[email protected]> wrote: >>>>>>>>>> On 07/08/2014 15:15, Aaron Ballman wrote: >>>>>>>>>>> On Thu, Aug 7, 2014 at 5:39 AM, Abramo Bagnara >>>>>>>>>>> <[email protected]> wrote: >>>>>>>>>>>> In the following source >>>>>>>>>>>> >>>>>>>>>>>> enum e { a }; >>>>>>>>>>>> enum e __attribute__((unused)) x; >>>>>>>>>>>> enum e >>>>>>>>>>>> __attribute__((unused)) y; >>>>>>>>>>>> >>>>>>>>>>>> the declaration of x is parsed without errors, but the declaration >>>>>>>>>>>> of y >>>>>>>>>>>> gives a parse error i.e. the presence of the newline makes a >>>>>>>>>>>> difference (!) >>>>>>>>>>>> >>>>>>>>>>>> Indeed I cannot imagine a reason why the parsing should be >>>>>>>>>>>> influenced by >>>>>>>>>>>> token position. >>>>>>>>>>>> >>>>>>>>>>>> Opinions anyone? >>>>>>>>>>> >>>>>>>>>>> Both are illegal parses -- a GNU-style attribute cannot appear in >>>>>>>>>>> that >>>>>>>>>>> location. >>>>>>>>>> >>>>>>>>>> Hmmm... but GCC compiles the source without any warning... >>>>>>>>>> >>>>>>>>>> Do you mean it is an undocumented feature of GCC or a GCC bug? >>>>>>>>> >>>>>>>>> Undocumented feature, as far as I can tell. That would attach the >>>>>>>>> attribute to the enum e type based on position, but it seems to >>>>>>>>> appertain to the declaration instead. >>>>>>>>> >>>>>>>>> But if GCC accepts this, we may want to consider implementing it as >>>>>>>>> well if there's sufficient reason. >>>>>>>> >>>>>>>> As we already behave in a GCC compatible way parsing >>>>>>>> >>>>>>>> struct s >>>>>>>> __attribute__((used)) x; >>>>>>>> int __attribute__((used)) y; >>>>>>>> >>>>>>>> I think that the proper fix is the attached one (i.e. we don't want to >>>>>>>> detect as illegal something that later we will accept) >>>>>>>> >>>>>>>> Ok to commit? >>>>>>> >>>>>>> Except that if GCC considers that parsing to be a bug, then all we're >>>>>>> really doing is enforcing their bug. Do you know of GCC's stance on >>>>>>> whether this is a bug or not? If it's not a bug, but something they >>>>>>> want to continue supporting, then I think we should fix it as well. >>>>>> >>>>>> This is clearly documented in >>>>>> https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html >>>>>> >>>>>> For compatibility with existing code written for compiler versions >>>>>> that did not implement attributes on nested declarators, some laxity is >>>>>> allowed in the placing of attributes. If an attribute that only applies >>>>>> to types is applied to a declaration, it is treated as applying to the >>>>>> type of that declaration. If an attribute that only applies to >>>>>> declarations is applied to the type of a declaration, it is treated as >>>>>> applying to that declaration; and, for compatibility with code placing >>>>>> the attributes immediately before the identifier declared, such an >>>>>> attribute applied to a function return type is treated as applying to >>>>>> the function type, and such an attribute applied to an array element >>>>>> type is treated as applying to the array type. If an attribute that >>>>>> only >>>>>> applies to function types is applied to a pointer-to-function type, it >>>>>> is treated as applying to the pointer target type; if such an attribute >>>>>> is applied to a function return type that is not a pointer-to-function >>>>>> type, it is treated as applying to the function type. >>>>>> >>>>> >>>>> The example directly preceding the paragraph you cited is what I >>>>> believe the issue is: >>>>> >>>>> char *__attribute__((aligned(8))) *f; >>>>> >>>>> specifies the type “pointer to 8-byte-aligned pointer to char”. Note >>>>> again that this does not work with >>>>> most attributes; for example, the usage of ‘aligned’ and ‘noreturn’ >>>>> attributes given above is not yet supported. >>>>> >>>>> Your code make no distinction between type attributes and declaration >>>>> attributes. GCC's documentation bears out that they don't either, but >>>>> that it is intended to be attribute-specific eventually. That's why I >>>>> don't think we should be implementing it this way. >>>>> >>>>> In Sema/attr-used.c, the definition of struct s properly diagnoses >>>>> that used only applies to variables and functions, but your elaborated >>>>> type specifier on the declaration doesn't warn despite the attribute >>>>> being in the same position and still needing to appertain to the type. >>>>> >>>>> So on the one hand, this is more compatible with how GCC behaves now, >>>>> but it's not how they document that it should behave, which is why I'm >>>>> wondering what problem this solves in practice. >>>> >>>> I think there is a misunderstanding: currently clang already parses >>>> >>>> struct s __attribute__((used)) x; >>>> int __attribute__((used)) y; >>>> >>>> as declaration attributes despite that their position are more suitable >>>> for type attributes. This is due to the fact that the attribute is >>>> declaration specific and it is perfectly congruent with what GCC >>>> documentation says wrt backward compatibility (i.e. "If an attribute >>>> that only applies to declarations is applied to the type of a >>>> declaration, it is treated as applying to that declaration"). >>> >>> It is congruent by chance, not design. >>> >>>> The patch addresses *only* a specific bug in trunk where >>>> >>>> struct s __attribute__((used)) x; >>>> >>>> and >>>> >>>> struct s // Note the presence of newline here >>>> __attribute__((used)) x; >>>> >>>> are parsed differently (the latter output a parse error with a >>>> misleading message). >>> >>> We're in agreement that these should not *parse* differently. >>> >>>> The testcase simply ensure that this will always be accepted just as the >>>> variant without newline. >>> >>> The behavior right now is that code which isn't valid is being >>> diagnosed as invalid in an unhelpful way in some situations, and not >>> being diagnosed at all in other situations. The proposed behavior is >>> to stop diagnosing entirely in all situations despite the fact that >>> the code isn't valid by design, just by chance. That doesn't seem like >>> a step in the right direction on its face, which is why I was asking >>> what problem this actually solves. >>> >>> What I think ultimately needs to happen is: 1) fixing the parse, 2) >>> fixing the semantics, 3) behaving according to the documentation of >>> what GCC says *should* be happening, 4) diagnosing resulting issues >>> consistently. Note, I am not suggesting *you* need to do this work. >>> :-) >>> >>> Does that make more sense? >>> >>> That being said, I think I've talked myself into this patch being >>> reasonable since it at least covers #1 and we can incrementally solve >>> 2-4 if we remain aware of them. We can fix the parse (which you've >>> done), add FIXMEs to the code explaining the situation, move your >>> current test case into Parser instead of Sema, add a FIXME test to >>> your current test case in Sema stating that the code *should* diagnose >>> according to GCC's documentation, and perhaps file a bug to track it >>> all. That will ease my worry about things being forgotten. If you deal >>> with the code stuff in your patch, I can write up the bug so we don't >>> lose track of this. >> >> Please check if the attached patch conforms to what you meant. > > Very close! > >> + // FIXME: we should emit diagnostics about misplaced declaration attribute >> + // parsed for compatibility with GCC and existing code > > Missing a period in the comment; should probably say "semantic > diagnostic when declaration attribute is in type attribute position" > since it's not really a parser FIXME, just that this is the jumping > off point for the fix. > >> Index: test/Parser/attributes.c >> =================================================================== >> --- test/Parser/attributes.c (revisione 215658) >> +++ test/Parser/attributes.c (copia locale) >> @@ -95,3 +95,12 @@ >> __attribute__((pure)) int testFundef6(int a) { return a; } >> >> void deprecatedTestFun(void) __attribute__((deprecated())); >> + >> +struct s { >> + int a; >> +}; >> + >> +// FIXME: we should emit diagnostics about misplaced declaration attribute >> +// parsed for compatibility with GCC and existing code >> +struct s >> +__attribute__((used)) bar; > > This code should be commented that it's in place to ensure > compatibility with parsing GNU-style attributes where the attribute is > on a separate line from the elaborated type specifier. > > We should then have a new test in Sema (gnu-attributes.c?) that does > something like: > > struct s {}; > > // FIXME: should warn that declaration attribute in type position is > being applied to the declaration instead? > struct s __attribute__((used)) foo; > > // FIXME: Should warn that type attribute in declaration position is > being applied to the type instead? > struct s *bar __attribute__((address_space(1))); > > // Should not warn because type attribute is in type position. > struct s *__attribute__((address_space(1))) baz; > > // Should not warn because declaration attribute is in declaration position. > struct s *quux __attribute__((used)); > > We can then expand this file with all of the other insane places you > can put GNU-style attributes. > > With those changes, LGTM! > > ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
