[PATCH] Fix assertion on C++ attributes in fillAttributedTypeLoc (this fixes http://llvm.org/PR17424)

[Richard Smith]

On Fri, May 1, 2015 at 7:13 AM, Aaron Ballman <aaron.ballman at gmail.com>
wrote:

> On Mon, Apr 27, 2015 at 6:23 AM, Alexey Frolov <alexfrolov1878 at yandex.ru>
> wrote:
> > Hi rsmith, aaron.ballman,
> >
> > fillAttributedTypeLoc() function is only called with AttributeLists of
> either //DeclarationChunk// (which is used for each type in a declarator
> being parsed) or //DeclSpec// (which captures information about declaration
> specifiers).
> > As C++11 attributes actually appertain to declarators, they are moved
> straight to the declarator’s attr list in
> distributeFunctionTypeAttrFromDeclSpec() function.
> > 'Put them wherever you like' semantics is not supported for C++11
> attributes (but is allowed for GNU attributes, for example).
> > So when we meet an attribute while parsing the declaration, we cannot be
> sure if it appertains to either //DeclarationChunk// or //DeclSpec//.
> >
> > This investigation correlates with the history of changes of
> SemaType.cpp:
> > • Asserts in fillAttributedTypeLoc() were added on 3 Mar 2011 in r126986
> (
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20110228/039638.html
> );
> > • Distributing C++11 attrs to the declarator was added on 14 Jan 2013 in
> r172504 (
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20130114/071830.html
> ).
> >
> > Considering all written above I changed asserts in
> fillAttributedTypeLoc() to nullptr checks.
>
> It seems like this means we have an attributed type location that has
> no corresponding attribute, which is weird. We then just leave the
> type location in this weird state by early returning. I'm not certain
> whether that's a good idea or not. It feels like we should not have
> the attributed type location in the first place, but I could be wrong.
> Richard may have further thoughts.


Well, I agree that the early return is wrong; the assert is right and
turning it off is not the right fix. The problem is also unrelated to the
attributes getting spliced around; this case asserts too:

  void pr17424_1 [[gnu::fastcall]]();

At first glance, I'd say that the problem is that we apply these as if
they're type attributes (they modify the type) but we model them as
declaration attributes (we put them on the declarator). Then, when we come
to fill in the attribute information, we look at the AttributeList from the
type, and the attribute isn't there.

We should also look for C++11 attributes in the attribute list on the
declarator when looking for attributes applied to the top level of a type,
to match processTypeAttrs, which applies type attributes in TAL_DeclName
mode from the declarator onto the declared entity's type.

> This fixes PR17424 and related assertion on
> > ```
> >   [[gnu::fastcall]] void __stdcall foo();
> > ```
> >
> > REPOSITORY
> >   rL LLVM
> >
> > http://reviews.llvm.org/D9288
> >
> > Files:
> >   lib/Sema/SemaType.cpp
> >   test/SemaCXX/cxx11-gnu-attrs.cpp
> >
> > Index: test/SemaCXX/cxx11-gnu-attrs.cpp
> > ===================================================================
> > --- test/SemaCXX/cxx11-gnu-attrs.cpp
> > +++ test/SemaCXX/cxx11-gnu-attrs.cpp
> > @@ -8,6 +8,17 @@
> >  // expected-error at -1 {{'unused' attribute cannot be applied to types}}
> >  int *[[gnu::unused]] attr_on_ptr;
> >  // expected-warning at -1 {{attribute 'unused' ignored, because it cannot
> be applied to a type}}
> > +[[gnu::fastcall]] void pr17424_1();
> > +// expected-warning at -1 {{calling convention 'fastcall' ignored for
> this target}}
> > +[[gnu::fastcall]] [[gnu::stdcall]] void pr17424_2();
> > +// expected-warning at -1 {{calling convention 'fastcall' ignored for
> this target}}
> > +// expected-warning at -2 {{calling convention 'stdcall' ignored for this
> target}}
> > +[[gnu::fastcall]] __stdcall void pr17424_3();
> > +// expected-warning at -1 {{calling convention 'fastcall' ignored for
> this target}}
> > +// expected-warning at -2 {{calling convention '__stdcall' ignored for
> this target}}
> > +[[gnu::fastcall]] void pr17424_4() [[gnu::stdcall]];
> > +// expected-warning at -1 {{calling convention 'fastcall' ignored for
> this target}}
> > +// expected-warning at -2 {{attribute 'stdcall' ignored, because it
> cannot be applied to a type}}
> >
> >  // Valid cases.
> >
> > Index: lib/Sema/SemaType.cpp
> > ===================================================================
> > --- lib/Sema/SemaType.cpp
> > +++ lib/Sema/SemaType.cpp
> > @@ -3492,13 +3492,15 @@
> >
> >  static void fillAttributedTypeLoc(AttributedTypeLoc TL,
> >                                    const AttributeList *attrs) {
> > -  AttributedType::Kind kind = TL.getAttrKind();
> > +  if (!attrs)
> > +    return;
> >
> > -  assert(attrs && "no type attributes in the expected location!");
> > +  AttributedType::Kind kind = TL.getAttrKind();
> >    AttributeList::Kind parsedKind = getAttrListKind(kind);
> >    while (attrs->getKind() != parsedKind) {
> >      attrs = attrs->getNext();
> > -    assert(attrs && "no matching attribute in expected location!");
> > +    if (!attrs)
> > +      return;
> >    }
> >
> >    TL.setAttrNameLoc(attrs->getLoc());
> >
> > EMAIL PREFERENCES
> >   http://reviews.llvm.org/settings/panel/emailpreferences/
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150518/42cc7392/attachment.html>