Nasty parsing bug?

[Abramo Bagnara]

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.


-- 
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,9 @@
   case tok::kw___declspec:      // struct foo {...} __declspec(...)
   case tok::l_square:           // void f(struct f  [         3])
   case tok::ellipsis:           // void f(struct f  ...       [Ns])
+  // FIXME: we should emit diagnostics about misplaced declaration attribute
+  // parsed for compatibility with GCC and existing code
+  case tok::kw___attribute:     // struct foo __attribute__((used)) x;
     return true;
   case tok::colon:
     return CouldBeBitfield;     // enum E { ... }   :         2;
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;