Clean up 'target' attribute diagnostics

[erichkeane]

Summary

There were a few issues previously with the target
attribute diagnostics implementation that lead to the
attribute being added to the AST despite having an error
in it.

This patch changes that, and adds a test to ensure it
does not get added to the AST.

[echristo]

Seems ok here.

[erichkeane]

Thanks! I'll give @aaron.ballman a chance to take a look, so I'll probably do something about this tomorrow.

[aaron.ballman]

test/Sema/attr-target-ast.c

1

Can you drop the svn props from this file?

test/Sema/attr-target.c

9–10

Is there ever a case where you *won't* get the "ignoring unsupported" without also getting the "'target' attribute ignored" warning as well? It seems a bit chatty to give two diagnostics for each mistake, and it makes one of those diagnostics seem incorrect -- in this case, one diagnostic says "ignoring unsupported architecture 'hiss'", implying the others are not ignored, but then the second diagnostic says the whole attribute is ignored.

[erichkeane]

test/Sema/attr-target-ast.c

1

Absolutely, I'll do that on commit.

test/Sema/attr-target.c

9–10

There is not... I actually put in the second because I thought that the 1st (ignoring unsupported...) was pretty ambiguous. I'll rev this patch seeing if I can simply make the former more clear.

[aaron.ballman]

test/Sema/attr-target.c

9–10

I think that makes sense. Or, alternatively, make the first diagnostic accurate by dropping all unsupported targets (leaving the valid ones) and adjusting the diagnostic wording depending on whether the whole attribute is dropped, or just parts of it?

[erichkeane]

Do as @aaron.ballman suggested for the message. Also, removed properties from the new file.

[aaron.ballman]

Aside from a minor wording nit, LGTM!

include/clang/Basic/DiagnosticSemaKinds.td

2448–2449 ↗

(On Diff #134643)

Can you manually quote 'target' in the diagnostic? I know it wasn't there before, but we try to stick quotes around syntactic constructs.

lib/Sema/SemaDeclAttr.cpp

bool Sema::checkTargetAttr(SourceLocation LiteralLoc, StringRef AttrStr) {
 
   for (const auto &Feature : ParsedAttrs.Features) {
     auto CurFeature = StringRef(Feature).drop_front(); // remove + or -.
     if (!Context.getTargetInfo().isValidFeatureName(CurFeature))
       return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
              << Unsupported << None << CurFeature;
   }
 
-  return true;
+  return false;
 }
 
 static void handleTargetAttr(Sema &S, Decl *D, const AttributeList &AL) {
   StringRef Str;
   SourceLocation LiteralLoc;
   if (!S.checkStringLiteralArgumentAttr(AL, 0, Str, &LiteralLoc) ||
-      !S.checkTargetAttr(LiteralLoc, Str))
+      S.checkTargetAttr(LiteralLoc, Str)) {
+    S.Diag(AL.getLoc(), diag::warn_attribute_ignored) << AL.getName();
     return;
+  }
+
   unsigned Index = AL.getAttributeSpellingListIndex();
   TargetAttr *NewAttr =
       ::new (S.Context) TargetAttr(AL.getRange(), S.Context, Str, Index);
   D->addAttr(NewAttr);
 }
 
 static void handleCleanupAttr(Sema &S, Decl *D, const AttributeList &AL) {
   Expr *E = AL.getArgAsExpr(0);

test/Sema/attr-target-ast.c

+// RUN: %clang_cc1 -triple x86_64-linux-gnu -ast-dump %s | FileCheck %s
+
+int __attribute__((target("arch=hiss,arch=woof"))) pine_tree() { return 4; }
+// CHECK-NOT: arch=hiss
+// CHECK-NOT: arch=woof

test/Sema/attr-target.c

 // RUN: %clang_cc1 -triple x86_64-linux-gnu  -fsyntax-only -verify %s
 
 int __attribute__((target("avx,sse4.2,arch=ivybridge"))) foo() { return 4; }
 int __attribute__((target())) bar() { return 4; } //expected-error {{'target' attribute takes one argument}}
+// expected-warning@+1 {{'target' attribute ignored}}
 int __attribute__((target("tune=sandybridge"))) baz() { return 4; } //expected-warning {{ignoring unsupported 'tune=' in the target attribute string}}
+// expected-warning@+1 {{'target' attribute ignored}}
 int __attribute__((target("fpmath=387"))) walrus() { return 4; } //expected-warning {{ignoring unsupported 'fpmath=' in the target attribute string}}
+// expected-warning@+1 {{'target' attribute ignored}}
 int __attribute__((target("avx,sse4.2,arch=hiss"))) meow() {  return 4; }//expected-warning {{ignoring unsupported architecture 'hiss' in the target attribute string}}
+// expected-warning@+1 {{'target' attribute ignored}}
 int __attribute__((target("woof"))) bark() {  return 4; }//expected-warning {{ignoring unsupported 'woof' in the target attribute string}}
 int __attribute__((target("arch="))) turtle() { return 4; } // no warning, same as saying 'nothing'.
+// expected-warning@+1 {{'target' attribute ignored}}
 int __attribute__((target("arch=hiss,arch=woof"))) pine_tree() { return 4; } //expected-warning {{ignoring unsupported architecture 'hiss' in the target attribute string}}
+// expected-warning@+1 {{'target' attribute ignored}}
 int __attribute__((target("arch=ivybridge,arch=haswell"))) oak_tree() { return 4; } //expected-warning {{ignoring duplicate 'arch=' in the target attribute string}}
 
 
 

test/SemaCXX/attr-target-mv.cpp

 // RUN: %clang_cc1 -triple x86_64-linux-gnu  -fsyntax-only -verify -fexceptions -fcxx-exceptions %s -std=c++14
 void __attribute__((target("default"))) invalid_features(void);
-//expected-error@+2 {{function multiversioning doesn't support feature 'hello_world'}}
-//expected-warning@+1 {{ignoring unsupported 'hello_world' in the target attribute string}}
+//expected-error@+3 {{function declaration is missing 'target' attribute in a multiversioned function}}
+//expected-warning@+2 {{ignoring unsupported 'hello_world' in the target attribute string}}
+//expected-warning@+1 {{'target' attribute ignored}}
 void __attribute__((target("hello_world"))) invalid_features(void);
 //expected-error@+1 {{function multiversioning doesn't support feature 'no-sse4.2'}}
 void __attribute__((target("no-sse4.2"))) invalid_features(void);
 
 void __attribute__((target("sse4.2"))) no_default(void);
 void __attribute__((target("arch=sandybridge")))  no_default(void);
 
 void use1(void){