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

2

Can you drop the svn props from this file?

test/Sema/attr-target.c

4–5

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

2

Absolutely, I'll do that on commit.

test/Sema/attr-target.c

4–5

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

4–5

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–2450

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

include/clang/Basic/DiagnosticSemaKinds.td

 def err_attribute_invalid_vector_type : Error<"invalid vector element type %0">;
 def err_attribute_bad_neon_vector_size : Error<
   "Neon vector size must be 64 or 128 bits">;
 def err_attribute_requires_positive_integer : Error<
   "%0 attribute requires a positive integral compile time constant expression">;
 def err_attribute_requires_opencl_version : Error<
   "%0 attribute requires OpenCL version %1%select{| or above}2">;
 def warn_unsupported_target_attribute
-    : Warning<"ignoring %select{unsupported|duplicate}0"
-              "%select{| architecture}1 '%2' in the target attribute string">,
+    : Warning<"%select{unsupported|duplicate}0%select{| architecture}1 '%2' in"
+              " the 'target' attribute string; 'target' attribute ignored">,
       InGroup<IgnoredAttributes>;
 def err_attribute_unsupported
     : Error<"%0 attribute is not supported for this target">;
 // The err_*_attribute_argument_not_int are separate because they're used by
 // VerifyIntegerConstantExpression.
 def err_aligned_attribute_argument_not_int : Error<
   "'aligned' attribute requires integer constant">;
 def err_align_value_attribute_argument_not_int : Error<
   "'align_value' attribute requires integer constant">;

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))
     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}}
-int __attribute__((target("tune=sandybridge"))) baz() { return 4; } //expected-warning {{ignoring unsupported 'tune=' in the target attribute string}}
-int __attribute__((target("fpmath=387"))) walrus() { return 4; } //expected-warning {{ignoring unsupported 'fpmath=' in the target attribute string}}
-int __attribute__((target("avx,sse4.2,arch=hiss"))) meow() {  return 4; }//expected-warning {{ignoring unsupported architecture 'hiss' in the target attribute string}}
-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'.
-int __attribute__((target("arch=hiss,arch=woof"))) pine_tree() { return 4; } //expected-warning {{ignoring unsupported architecture 'hiss' in the target attribute string}}
-int __attribute__((target("arch=ivybridge,arch=haswell"))) oak_tree() { return 4; } //expected-warning {{ignoring duplicate 'arch=' in the target attribute string}}
-
+//expected-error@+1 {{'target' attribute takes one argument}}
+int __attribute__((target())) bar() { return 4; }
+//expected-warning@+1 {{unsupported 'tune=' in the 'target' attribute string; 'target' attribute ignored}}
+int __attribute__((target("tune=sandybridge"))) baz() { return 4; }
+//expected-warning@+1 {{unsupported 'fpmath=' in the 'target' attribute string; 'target' attribute ignored}}
+int __attribute__((target("fpmath=387"))) walrus() { return 4; }
+//expected-warning@+1 {{unsupported architecture 'hiss' in the 'target' attribute string; 'target' attribute ignored}}
+int __attribute__((target("avx,sse4.2,arch=hiss"))) meow() {  return 4; }
+//expected-warning@+1 {{unsupported 'woof' in the 'target' attribute string; 'target' attribute ignored}}
+int __attribute__((target("woof"))) bark() {  return 4; }
+// no warning, same as saying 'nothing'.
+int __attribute__((target("arch="))) turtle() { return 4; }
+//expected-warning@+1 {{unsupported architecture 'hiss' in the 'target' attribute string; 'target' attribute ignored}}
+int __attribute__((target("arch=hiss,arch=woof"))) pine_tree() { return 4; }
+//expected-warning@+1 {{duplicate 'arch=' in the 'target' attribute string; 'target' attribute ignored}}
+int __attribute__((target("arch=ivybridge,arch=haswell"))) oak_tree() { return 4; }
 
 

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@+2 {{function declaration is missing 'target' attribute in a multiversioned function}}
+//expected-warning@+1 {{unsupported 'hello_world' in the 'target' attribute string; '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){