Nasty parsing bug?

[Abramo Bagnara]

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").

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).

The testcase simply ensure that this will always be accepted just as the
variant without newline.

-- 
Abramo Bagnara

BUGSENG srl - http://bugseng.com
mailto:abramo.bagnara at bugseng.com