Likely problem in attribute parsing

[Aaron Ballman]

On Fri, Feb 28, 2014 at 9:01 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> On Thu, Feb 27, 2014 at 7:51 PM, John McCall <rjmccall at apple.com> wrote:
>> On Feb 27, 2014, at 3:45 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>>> On Thu, Feb 27, 2014 at 5:17 PM, Abramo Bagnara
>>> <abramo.bagnara at bugseng.com> wrote:
>>>> $ cat z.c
>>>> void __attribute__((noinline,noreturn))
>>>> f(void);
>>>> $ _clang -cc1 -ast-dump z.c
>>>> TranslationUnitDecl 0x6630730 <<invalid sloc>>
>>>> |-TypedefDecl 0x6630c30 <<invalid sloc>> __int128_t '__int128'
>>>> |-TypedefDecl 0x6630c90 <<invalid sloc>> __uint128_t 'unsigned __int128'
>>>> |-TypedefDecl 0x6630fe0 <<invalid sloc>> __builtin_va_list
>>>> '__va_list_tag [1]'
>>>> `-FunctionDecl 0x6631120 <z.c:1:1, line:2:7> f 'void (void)
>>>> __attribute__((noreturn))'
>>>>  |-NoInlineAttr 0x66311c0 <line:1:21>
>>>>  `-NoInlineAttr 0x6631200 <col:21>
>>>> $ clang -cc1 -ast-print z.c
>>>> void f() __attribute__((noinline)) __attribute__((noinline));
>>>>
>>>> Can you confirm that the doubled attribute noinline should be considered
>>>> a bug?
>>>
>>> From what I can tell, this is happening because of the position and
>>> order of the attributes. They're positioned as type attributes, and
>>> noreturn is treated as one, but noinline is treated as a declaration
>>> attribute. So noreturn is being spliced out onto the declspec, while
>>> noinline remains on the function type, as it should. However, we save
>>> the declspec attributes in distributeFunctionTypeAttrFromDeclSpec, so
>>> what we spliced out gets restored later -- so the attributes
>>> effectively get duplicated.
>>>
>>> By disallowing the saveDeclSpecAttrs(), the issue is resolved and all
>>> regression tests pass, but I do not understand the rationale as to why
>>> they were being saved in the first place. The only other usage of
>>> saveDeclSpecsAttrs() is with ObjC pointer type attributes, prior to
>>> performing a moveAttrFromListToList. However, removing the call to
>>> saveDeclSpecAttrs() there also had no effect on the regression tests.
>>> John -- it looks like you added this functionality. Can you help me to
>>> understand what's going on?
>>
>> I expect it’s probably an attempt to ensure that attributes in the decl spec
>> are applied to all the declarations in a group.  Sounds like there’s a bug,
>> though, and then a second bug if we’re not testing it. :)
>
> Yup, that appears to be exactly the case. I'll add a test case for it,
> and see if I can solve the bug. Thank you for the help!

The bug stems from the fact that we are trying to move attributes in a
declspec attribute position to someplace more sensible when possible.
We do this by splicing out of the declspec's attribute list, and onto
the proper declarator for type attributes. However, we need to save
the original declspec attribute list so that it can be applied to
other declarations. Eg) void __attribute__((cdecl)) foo(void),
bar(void); // the cdecl attribute appertains to both foo and bar.

In this case, one of the attributes is a type attribute (noreturn),
and the other is a declaration attribute (noinline). So the type
attribute is spliced out, the declaration attribute remains as-is, and
then we restore everything back onto the declspec. During this
restoration, since we were saving the original pointers to the
attributes, the restoration re-introduces the links between the two
attributes. As a concrete demonstration:

void __attribute__((noinline, noreturn)) f(void);

The attribute linked list is: noreturn -> noinline -> null
The noreturn type attribute is spliced off the declspec and onto the
declarator chunk. The noinline attribute is left on the declspec.
After this splicing, the DS attribute linked list is: noinline -> null
(correct), and the Function declarator chunk's attribute linked list
is: noreturn -> null (correct). When restoring the attributes to the
declspec, the DS attribute linked list becomes: noreturn -> noinline
-> null (correct), and the Function declarator chunk's attribute
linked list becomes: noreturn -> noinline -> null (incorrect) because
the links are all restored, but the pointer exists in two lists. So
when restoring the links for the DS, they are accidentally restored
for the declarator chunk as well. When we go to process the parsed
attributes in SemaDeclAttr.cpp, we wind up processing noinline twice
because it is on the declspec, and was accidentally "restored" onto
the declarator chunk. Consequently, we also wind up processing
noreturn twice, but since we bail out when hasDeclarator(D) is true
(to process as a type attribute instead), it turns out to be harmless.

This entire process is.. not ideal. Suggestions welcome. :-)

~Aaron