Likely problem in attribute parsing

[John McCall]

On Feb 28, 2014, at 10:25 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> 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. :-)

Maybe declarator processing should just collect all the non-type
attributes on the Declarator, then just make SemaDeclAttr process
that instead of re-walking the declarator?

John.