[PATCH] Downgrade 'optnone' attribute conflicts to warning

[Aaron Ballman]

On Mon, Jan 12, 2015 at 3:31 PM, Paul Robinson
<Paul_Robinson at playstation.sony.com> wrote:
> Hi aaron.ballman,
>
> When attribute 'optnone' appears on the same declaration with a
> conflicting attribute, warn about the conflict and pick a "winning"
> attribute to preserve, instead of emitting an error.  This matches the
> behavior when the conflicting attributes are on different declarations.
>
> Along the way I discovered that conflicts involving __forceinline were
> reported as 'always_inline' (alternate spellings for the same
> attribute) so I hacked in a way to provide the spelling used in the
> declaration. There must be a better way, though...

LGTM, with one possible suggestion below.

>
> http://reviews.llvm.org/D6933
>
> Files:
>   include/clang/Sema/Sema.h
>   lib/Sema/SemaDecl.cpp
>   lib/Sema/SemaDeclAttr.cpp
>   test/SemaCXX/attr-optnone.cpp
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/

> Index: include/clang/Sema/Sema.h
> ===================================================================
> --- include/clang/Sema/Sema.h
> +++ include/clang/Sema/Sema.h
> @@ -2009,6 +2009,7 @@
>    SectionAttr *mergeSectionAttr(Decl *D, SourceRange Range, StringRef Name,
>                                  unsigned AttrSpellingListIndex);
>    AlwaysInlineAttr *mergeAlwaysInlineAttr(Decl *D, SourceRange Range,
> +                                          StringRef Name,
>                                            unsigned AttrSpellingListIndex);
>    MinSizeAttr *mergeMinSizeAttr(Decl *D, SourceRange Range,
>                                  unsigned AttrSpellingListIndex);
> Index: lib/Sema/SemaDecl.cpp
> ===================================================================
> --- lib/Sema/SemaDecl.cpp
> +++ lib/Sema/SemaDecl.cpp
> @@ -2155,7 +2155,8 @@
>                                         AttrSpellingListIndex,
>                                         IA->getSemanticSpelling());
>    else if (const auto *AA = dyn_cast<AlwaysInlineAttr>(Attr))
> -    NewAttr = S.mergeAlwaysInlineAttr(D, AA->getRange(), AttrSpellingListIndex);
> +    NewAttr = S.mergeAlwaysInlineAttr(D, AA->getRange(), AA->getSpelling(),
> +                                      AttrSpellingListIndex);
>    else if (const auto *MA = dyn_cast<MinSizeAttr>(Attr))
>      NewAttr = S.mergeMinSizeAttr(D, MA->getRange(), AttrSpellingListIndex);
>    else if (const auto *OA = dyn_cast<OptimizeNoneAttr>(Attr))
> Index: lib/Sema/SemaDeclAttr.cpp
> ===================================================================
> --- lib/Sema/SemaDeclAttr.cpp
> +++ lib/Sema/SemaDeclAttr.cpp
> @@ -3141,9 +3141,14 @@
>  }
>
>  AlwaysInlineAttr *Sema::mergeAlwaysInlineAttr(Decl *D, SourceRange Range,
> +                                              StringRef Name,
>                                                unsigned AttrSpellingListIndex) {
>    if (OptimizeNoneAttr *Optnone = D->getAttr<OptimizeNoneAttr>()) {
> -    Diag(Range.getBegin(), diag::warn_attribute_ignored) << "'always_inline'";
> +    SmallString<40> QuotedName;
> +    QuotedName = "'";
> +    QuotedName += Name;
> +    QuotedName += "'";
> +    Diag(Range.getBegin(), diag::warn_attribute_ignored) << QuotedName;
>      Diag(Optnone->getLocation(), diag::note_conflicting_attribute);
>      return nullptr;
>    }

You could pass in the name as an IdentifierInfo * and ditch the manual
quoting. I realize that Attr::getSpelling() returns a StringRef, but
getting the IdentifierInfo should be low-cost and slightly less ugly.


> @@ -3191,34 +3196,23 @@
>
>  static void handleAlwaysInlineAttr(Sema &S, Decl *D,
>                                     const AttributeList &Attr) {
> -  if (checkAttrMutualExclusion<OptimizeNoneAttr>(S, D, Attr))
> -    return;
> -
> -  D->addAttr(::new (S.Context)
> -             AlwaysInlineAttr(Attr.getRange(), S.Context,
> -                              Attr.getAttributeSpellingListIndex()));
> +  if (AlwaysInlineAttr *Inline = S.mergeAlwaysInlineAttr(
> +          D, Attr.getRange(), Attr.getName()->getName(),
> +          Attr.getAttributeSpellingListIndex()))
> +    D->addAttr(Inline);
>  }
>
> -static void handleMinSizeAttr(Sema &S, Decl *D,
> -                              const AttributeList &Attr) {
> -  if (checkAttrMutualExclusion<OptimizeNoneAttr>(S, D, Attr))
> -    return;
> -
> -  D->addAttr(::new (S.Context)
> -             MinSizeAttr(Attr.getRange(), S.Context,
> -                         Attr.getAttributeSpellingListIndex()));
> +static void handleMinSizeAttr(Sema &S, Decl *D, const AttributeList &Attr) {
> +  if (MinSizeAttr *MinSize = S.mergeMinSizeAttr(
> +          D, Attr.getRange(), Attr.getAttributeSpellingListIndex()))
> +    D->addAttr(MinSize);
>  }
>
>  static void handleOptimizeNoneAttr(Sema &S, Decl *D,
>                                     const AttributeList &Attr) {
> -  if (checkAttrMutualExclusion<AlwaysInlineAttr>(S, D, Attr))
> -    return;
> -  if (checkAttrMutualExclusion<MinSizeAttr>(S, D, Attr))
> -    return;
> -
> -  D->addAttr(::new (S.Context)
> -             OptimizeNoneAttr(Attr.getRange(), S.Context,
> -                              Attr.getAttributeSpellingListIndex()));
> +  if (OptimizeNoneAttr *Optnone = S.mergeOptimizeNoneAttr(
> +          D, Attr.getRange(), Attr.getAttributeSpellingListIndex()))
> +    D->addAttr(Optnone);
>  }
>
>  static void handleGlobalAttr(Sema &S, Decl *D, const AttributeList &Attr) {
> Index: test/SemaCXX/attr-optnone.cpp
> ===================================================================
> --- test/SemaCXX/attr-optnone.cpp
> +++ test/SemaCXX/attr-optnone.cpp
> @@ -3,17 +3,17 @@
>  int foo() __attribute__((optnone));
>  int bar() __attribute__((optnone)) __attribute__((noinline));
>
> -int baz() __attribute__((always_inline)) __attribute__((optnone)); // expected-error{{'always_inline' and 'optnone' attributes are not compatible}}
> -int quz() __attribute__((optnone)) __attribute__((always_inline)); // expected-error{{'optnone' and 'always_inline' attributes are not compatible}}
> +int baz() __attribute__((always_inline)) __attribute__((optnone)); // expected-warning{{'always_inline' attribute ignored}} expected-note{{conflicting attribute is here}}
> +int quz() __attribute__((optnone)) __attribute__((always_inline)); // expected-warning{{'always_inline' attribute ignored}} expected-note{{conflicting attribute is here}}
>
>  __attribute__((always_inline)) int baz1(); // expected-warning{{'always_inline' attribute ignored}}
>  __attribute__((optnone)) int baz1() { return 1; } // expected-note{{conflicting attribute is here}}
>
>  __attribute__((optnone)) int quz1(); // expected-note{{conflicting attribute is here}}
>  __attribute__((always_inline)) int quz1() { return 1; } // expected-warning{{'always_inline' attribute ignored}}
>
> -int bay() __attribute__((minsize)) __attribute__((optnone)); // expected-error{{'minsize' and 'optnone' attributes are not compatible}}
> -int quy() __attribute__((optnone)) __attribute__((minsize)); // expected-error{{'optnone' and 'minsize' attributes are not compatible}}
> +int bay() __attribute__((minsize)) __attribute__((optnone)); // expected-warning{{'minsize' attribute ignored}} expected-note{{conflicting}}
> +int quy() __attribute__((optnone)) __attribute__((minsize)); // expected-warning{{'minsize' attribute ignored}} expected-note{{conflicting}}
>
>  __attribute__((minsize)) int bay1(); // expected-warning{{'minsize' attribute ignored}}
>  __attribute__((optnone)) int bay1() { return 1; } // expected-note{{conflicting attribute is here}}
> @@ -27,8 +27,13 @@
>  __attribute__((optnone)) // expected-note 2 {{conflicting}}
>  void bay2() {}
>
> -__forceinline __attribute__((optnone)) int bax(); // expected-error{{'__forceinline' and 'optnone' attributes are not compatible}}
> -__attribute__((optnone)) __forceinline int qux(); // expected-error{{'optnone' and '__forceinline' attributes are not compatible}}
> +__forceinline __attribute__((optnone)) int bax(); // expected-warning{{'__forceinline' attribute ignored}} expected-note{{conflicting}}
> +__attribute__((optnone)) __forceinline int qux(); // expected-warning{{'__forceinline' attribute ignored}} expected-note{{conflicting}}
> +
> +__forceinline int bax2(); // expected-warning{{'__forceinline' attribute ignored}}
> +__attribute__((optnone)) int bax2() { return 1; } // expected-note{{conflicting}}
> +__attribute__((optnone)) int qux2(); // expected-note{{conflicting}}
> +__forceinline int qux2() { return 1; } // expected-warning{{'__forceinline' attribute ignored}}
>
>  int globalVar __attribute__((optnone)); // expected-warning{{'optnone' attribute only applies to functions}}
>
> @@ -50,8 +55,8 @@
>  [[clang::optnone]]
>  int bar2() __attribute__((noinline));
>
> -[[clang::optnone]]
> -int baz2() __attribute__((always_inline)); // expected-error{{'always_inline' and 'optnone' attributes are not compatible}}
> +[[clang::optnone]] // expected-note {{conflicting}}
> +int baz2() __attribute__((always_inline)); // expected-warning{{'always_inline' attribute ignored}}
>
>  [[clang::optnone]] int globalVar2; //expected-warning{{'optnone' attribute only applies to functions}}
>
>

~Aaron