Nasty parsing bug?

[Aaron Ballman]

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 <aaron at aaronballman.com> wrote:
> On Fri, Aug 15, 2014 at 1:55 PM, Abramo Bagnara
> <abramo.bagnara at bugseng.com> wrote:
>> Il 15/08/2014 16:51, Aaron Ballman ha scritto:
>>> On Fri, Aug 15, 2014 at 10:01 AM, Abramo Bagnara
>>> <abramo.bagnara at bugseng.com> wrote:
>>>> Il 15/08/2014 15:46, Aaron Ballman ha scritto:
>>>>> On Fri, Aug 15, 2014 at 9:04 AM, Abramo Bagnara
>>>>> <abramo.bagnara at bugseng.com> wrote:
>>>>>> Il 15/08/2014 13:51, Aaron Ballman ha scritto:
>>>>>>> On Fri, Aug 15, 2014 at 4:10 AM, Abramo Bagnara
>>>>>>> <abramo.bagnara at bugseng.com> wrote:
>>>>>>>> Il 07/08/2014 15:42, Aaron Ballman ha scritto:
>>>>>>>>> On Thu, Aug 7, 2014 at 9:30 AM, Abramo Bagnara <abramo.bagnara at gmail.com> wrote:
>>>>>>>>>> On 07/08/2014 15:15, Aaron Ballman wrote:
>>>>>>>>>>> On Thu, Aug 7, 2014 at 5:39 AM, Abramo Bagnara
>>>>>>>>>>> <abramo.bagnara at bugseng.com> 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