Nasty parsing bug?

[Abramo Bagnara]

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.

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.

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

-- 
Abramo Bagnara

BUGSENG srl - http://bugseng.com
mailto:abramo.bagnara at bugseng.com
-------------- next part --------------
Index: lib/Parse/ParseDeclCXX.cpp
===================================================================
--- lib/Parse/ParseDeclCXX.cpp	(revisione 215658)
+++ lib/Parse/ParseDeclCXX.cpp	(copia locale)
@@ -1068,6 +1068,8 @@
   case tok::kw___declspec:      // struct foo {...} __declspec(...)
   case tok::l_square:           // void f(struct f  [         3])
   case tok::ellipsis:           // void f(struct f  ...       [Ns])
+  // GCC compatibility
+  case tok::kw___attribute:     // struct foo __attribute__((used)) x;
     return true;
   case tok::colon:
     return CouldBeBitfield;     // enum E { ... }   :         2;
Index: test/Sema/attr-used.c
===================================================================
--- test/Sema/attr-used.c	(revisione 215658)
+++ test/Sema/attr-used.c	(copia locale)
@@ -18,3 +18,8 @@
 }
 
 static void __attribute__((used)) f0(void);
+
+// Attribute is applied to the variable for compatibility with GCC
+// documented behaviour
+struct s
+__attribute__((used)) x;