On Feb 28, 2014, at 10:25 AM, Aaron Ballman <[email protected]> wrote: > On Fri, Feb 28, 2014 at 9:01 AM, Aaron Ballman <[email protected]> wrote: >> On Thu, Feb 27, 2014 at 7:51 PM, John McCall <[email protected]> wrote: >>> On Feb 27, 2014, at 3:45 PM, Aaron Ballman <[email protected]> wrote: >>>> On Thu, Feb 27, 2014 at 5:17 PM, Abramo Bagnara >>>> <[email protected]> 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. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
