[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

const auto *ND please.

2621

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

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

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

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

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

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.

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 (auto ND = dyn_cast<NamedDecl>(D)) {
+    if (!ND->isExternallyVisible()) {
+      S.Diag(AL.getRange().getBegin(), diag::warn_attribute_ignored) << AL;
+      return;
+    }
+  }
+
   // 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)) {

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 ignored}}
+static int test9 __attribute__((visibility("hidden"))); // expected-warning {{'visibility' attribute ignored}}
+static int test10 __attribute__((visibility("internal"))); // expected-warning {{'visibility' attribute ignored}}
+
+static int test11() __attribute__((visibility("default"))); // expected-warning {{'visibility' attribute ignored}}

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{};

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 ignored}}
+
+namespace {
+  int test5 __attribute__((visibility("default"))); // expected-warning {{'visibility' attribute ignored}}
+};