[Sema] Emit warning for visibility attribute on internal-linkage declaration

[scott.linder]

Summary

GCC warns on these cases, but we currently just silently ignore the attribute.

[aaron.ballman]

lib/Sema/SemaDeclAttr.cpp

2619 ↗

(On Diff #196528)

const auto *ND please.

2621 ↗

(On Diff #196528)

I think it would be more helpful for users if we introduced a new diagnostic here that explained why the attribute is being ignored, otherwise the diagnostic is somewhat indecipherable.

2622 ↗

(On Diff #196528)

We shouldn't early return here (it's not an error, just a warning), so that the other diagnostics can also trigger if needed.

[scott.linder]

lib/Sema/SemaDeclAttr.cpp

2621 ↗

(On Diff #196528)

Sure, I am amenable to anything here. GCC uses the same short message, but it does seem to indicate e.g. that the keyword static played some role by using additional source ranges. I don't know how we would go about that with the Sema/AST split? We could just go with a more explicit message. Do you have any thoughts on the wording?

warning: 'visibility' attribute ignored on non-external symbol

2622 ↗

(On Diff #196528)

Should I also fix the other early-return-on-warning's in the same function, e.g. the function begins with:

// Visibility attributes don't mean anything on a typedef.
if (isa<TypedefNameDecl>(D)) {
  S.Diag(AL.getRange().getBegin(), diag::warn_attribute_ignored) << AL;
  return;
}

I suppose the difference is that the attribute "can" be placed on the symbol in this case, but it is ignored just the same.

[scott.linder]

Clean up use of auto

[aaron.ballman]

lib/Sema/SemaDeclAttr.cpp

2621 ↗

(On Diff #196528)

Your suggested diagnostic text seems pretty close to me, but how about: 'visibility' attribute is ignore on a non-external symbol in the IgnoredAttributes group.

2622 ↗

(On Diff #196528)

That might be worth fixing as well, but as a separate patch because it's also kind of unrelated to the functionality in this patch. Good catch!

[scott.linder]

Use distinct sema diagnostic and do not return early.

I am going to bump DIAG_SIZE_SEMA in another patch, as it seems like we have hit the current limit.

[aaron.ballman]

LGTM, thank you!

[scott.linder]

Richard Smith on cfe-dev pointed out some cases where this patch is incorrect, stemming from trying to calculate the linkage too early; the warning will either have to work without the use of isExternallyVisible or will have to be emitted later. This was reverted in r359858 for now.

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_dllimport_dropped_from_inline_function : Warning<
   "%q0 redeclared inline; %1 attribute ignored">,
   InGroup<IgnoredAttributes>;
 def warn_attribute_ignored : Warning<"%0 attribute ignored">,
   InGroup<IgnoredAttributes>;
 def warn_attribute_ignored_on_inline :
   Warning<"%0 attribute ignored on inline function">,
   InGroup<IgnoredAttributes>;
+def warn_attribute_ignored_on_non_external :
+  Warning<"%0 attribute is ignored on a non-external symbol">,
+  InGroup<IgnoredAttributes>;
 def warn_nocf_check_attribute_ignored :
   Warning<"'nocf_check' attribute ignored; use -fcf-protection to enable the attribute">,
   InGroup<IgnoredAttributes>;
 def warn_attribute_after_definition_ignored : Warning<
   "attribute %0 after definition is ignored">,
    InGroup<IgnoredAttributes>;
 def warn_cxx11_gnu_attribute_on_type : Warning<
   "attribute %0 ignored, because it cannot be applied to a type">,

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

if (isTypeVisibility &&
       !(isa<TagDecl>(D) ||
         isa<ObjCInterfaceDecl>(D) ||
         isa<NamespaceDecl>(D))) {
     S.Diag(AL.getRange().getBegin(), diag::err_attribute_wrong_decl_type)
         << AL << ExpectedTypeOrNamespace;
     return;
   }
 
+  // Visibility attributes have no effect on symbols with internal linkage.
+  if (const auto *ND = dyn_cast<NamedDecl>(D)) {
+    if (!ND->isExternallyVisible())
+      S.Diag(AL.getRange().getBegin(),
+             diag::warn_attribute_ignored_on_non_external)
+          << AL;
+  }
+
   // Check that the argument is a string literal.
   StringRef TypeStr;
   SourceLocation LiteralLoc;
   if (!S.checkStringLiteralArgumentAttr(AL, 0, TypeStr, &LiteralLoc))
     return;
 
   VisibilityAttr::VisibilityType type;
   if (!VisibilityAttr::ConvertStrToVisibilityType(TypeStr, type)) {

cfe/trunk/test/Sema/attr-visibility.c

 extern int test7 __attribute__((visibility("default"))); // expected-note {{previous attribute is here}}
 extern int test7 __attribute__((visibility("hidden"))); // expected-error {{visibility does not match previous declaration}}
 
 typedef int __attribute__((visibility("default"))) bar; // expected-warning {{'visibility' attribute ignored}}
 
 int x __attribute__((type_visibility("default"))); // expected-error {{'type_visibility' attribute only applies to types and namespaces}}
 
 int PR17105 __attribute__((visibility(hidden))); // expected-error {{'visibility' attribute requires a string}}
+
+static int test8 __attribute__((visibility("default"))); // expected-warning {{'visibility' attribute is ignored on a non-external symbol}}
+static int test9 __attribute__((visibility("hidden"))); // expected-warning {{'visibility' attribute is ignored on a non-external symbol}}
+static int test10 __attribute__((visibility("internal"))); // expected-warning {{'visibility' attribute is ignored on a non-external symbol}}
+
+static int test11() __attribute__((visibility("default"))); // expected-warning {{'visibility' attribute is ignored on a non-external symbol}}

cfe/trunk/test/SemaCXX/ast-print.cpp

case 1:
       // CHECK: {{\[\[clang::fallthrough\]\]}}
       [[clang::fallthrough]];
     case 2:
       break;
   }
 }
 }
 
-namespace {
 // CHECK: struct {{\[\[gnu::visibility\(\"hidden\"\)\]\]}} S;
 struct [[gnu::visibility("hidden")]] S;
-}
 
 // CHECK:      struct CXXFunctionalCastExprPrint {
 // CHECK-NEXT: } fce = CXXFunctionalCastExprPrint{};
 struct CXXFunctionalCastExprPrint {} fce = CXXFunctionalCastExprPrint{};
 
 // CHECK:      struct CXXTemporaryObjectExprPrint {
 // CHECK-NEXT:   CXXTemporaryObjectExprPrint();
 // CHECK-NEXT: } toe = CXXTemporaryObjectExprPrint{};

cfe/trunk/test/SemaCXX/attr-visibility.cpp

 template <>
  __attribute__((visibility("default"))) // expected-error {{visibility does not match previous declaration}}
 void foo<int>() {
 }
 
 struct x3 {
   static int y;
 } __attribute((visibility("default"))); // expected-warning {{attribute 'visibility' after definition is ignored}}
+
+const int test4 __attribute__((visibility("default"))) = 0; // expected-warning {{'visibility' attribute is ignored on a non-external symbol}}
+
+namespace {
+  int test5 __attribute__((visibility("default"))); // expected-warning {{'visibility' attribute is ignored on a non-external symbol}}
+};