Nasty parsing bug?

[Aaron Ballman]

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.

> Note however that the proposed patch does not change current parsing
> strategy, but only adapt an helper predicate to correctly return the
> parsability status of an accepted fragment to avoid that the presence or
> the absence of a newline influences the output of a parse error.

Which is why I was asking for parsing tests. Something which
previously parsed one way is now being parsed another way, and
ensuring we don't accidentally regress that later is important. I was
envisioning that the semantic tests would warn on declaration
attributes in the wrong position, and ensure that the attribute is
attached in the right location in the AST.

>> As for your patch -- it's missing parsing and semantics test cases for
>> both C and C++ (eg, how well does this work with enum class
>> declarations in C++?). Otherwise, the approach looks reasonable to me.
>
> I've attached a patch with a testcase that does not fail any longer.
>
> Concerning enum class and C++ I really don't understand which kind of
> testcase are you imagining and how is this related to original issue.

I forgot that using elaborated type specifiers for enum classes must
elide the class keyword, so it doesn't apply.

~Aaron