[AST] Use -fvisibility value when ignoring -fv-i-h* inline static locals

[rnk]

Summary

In r340386 we added code to give static locals in inline functions
default visibility. Instead, we should use the "default" visibility
passed on the command line, which could be hidden or protected, as GCC
does.

Most codebases probably only use either -fvisibility=hidden or
-fvisibility-inlines-hidden, since marking inlines hidden should be a
no-op if everything was already going to be hidden, but it looks like
Chromium uses both.

Fixes PR39236

[hans]

Most codebases probably only use either -fvisibility=hidden or
-fvisibility-inlines-hidden, since marking inlines hidden should be a
no-op if everything was already going to be hidden, but it looks like
Chromium uses both.

I think using both flags is useful in code bases that want hidden visibility by default, but mark some classes visible explicitly, and then still want the inline functions hidden as an optimization:

struct __attribute__((visibility("default"))) S {
  int foo() {
    static int x;
    return x++;
  }
  int bar();
};

int S::bar() { return 45; }
void use(S *s) { s->foo(); }

With both -fvisibility=hidden and -fvisibility-inlines-hidden, S::bar() will be visible, but S::foo() will be hidden. S::foo()'s static local should be visible though. If the class didn't have the attribute, everything including the static local should be hidden.

The patch looks good to me, but perhaps we should add the above as a test case?

[rnk]

Mmm, looks like we get that wrong. I would've expected us to go down the "explicit attribute" path, but apparently not.

[rnk]

• substantial rewrite

[rnk]

This is rewritten enough that you might want to re-review it, but I'm going to push it today so we don't have to wait another day/night cycle for the fix.

[hans]

In D53052#1261158, @rnk wrote:

This is rewritten enough that you might want to re-review it, but I'm going to push it today so we don't have to wait another day/night cycle for the fix.

Looks good to me. It's hard to follow the logic, but the testing seems pretty comprehensive now. It's a lot like the dll attributes in that way :-)

lib/AST/Decl.cpp

if (!LV.isVisibilityExplicit()) {
           computation.isValueVisibility()
               ? Context.getLangOpts().getValueVisibilityMode()
               : Context.getLangOpts().getTypeVisibilityMode();
       LV.mergeVisibility(globalVisibility, /*explicit*/ false);
 
       // If we're paying attention to global visibility, apply
       // -finline-visibility-hidden if this is an inline method.
       if (useInlineVisibilityHidden(D))
-        LV.mergeVisibility(HiddenVisibility, true);
+        LV.mergeVisibility(HiddenVisibility, /*visibilityExplicit=*/false);
     }
   }
 
   // C++ [basic.link]p4:
 
   //   A name having namespace scope has external linkage if it is the
   //   name of
   //
if (!hasExplicitVisibilityAlready(computation)) {
     if (Optional<Visibility> Vis = getExplicitVisibility(D, computation))
       LV.mergeVisibility(*Vis, true);
     // If we're paying attention to global visibility, apply
     // -finline-visibility-hidden if this is an inline method.
     //
     // Note that we do this before merging information about
     // the class visibility.
     if (!LV.isVisibilityExplicit() && useInlineVisibilityHidden(D))
-      LV.mergeVisibility(HiddenVisibility, true);
+      LV.mergeVisibility(HiddenVisibility, /*visibilityExplicit=*/false);
   }
 
   // If this class member has an explicit visibility attribute, the only
   // thing that can change its visibility is the template arguments, so
   // only look for them when processing the class.
   LVComputationKind classComputation = computation;
   if (LV.isVisibilityExplicit())
     classComputation = withExplicitVisibilityAlready(computation);
if (const auto *BD = dyn_cast<BlockDecl>(OuterD)) {
     const auto *FD = cast<FunctionDecl>(OuterD);
     if (!FD->isInlined() &&
         !isTemplateInstantiation(FD->getTemplateSpecializationKind()))
       return LinkageInfo::none();
 
     // If a function is hidden by -fvisibility-inlines-hidden option and
     // is not explicitly attributed as a hidden function,
     // we should not make static local variables in the function hidden.
+    LV = getLVForDecl(FD, computation);
     if (isa<VarDecl>(D) && useInlineVisibilityHidden(FD) &&
-        !(!hasExplicitVisibilityAlready(computation) &&
-          getExplicitVisibility(FD, computation))) {
+        !LV.isVisibilityExplicit()) {
       assert(cast<VarDecl>(D)->isStaticLocal());
-      return LinkageInfo(VisibleNoLinkage, DefaultVisibility, false);
+      // If this was an implicitly hidden inline method, check again for
+      // explicit visibility on the parent class, and use that for static locals
+      // if present.
+      if (const auto *MD = dyn_cast<CXXMethodDecl>(FD))
+        LV = getLVForDecl(MD->getParent(), computation);
+      if (!LV.isVisibilityExplicit()) {
+        Visibility globalVisibility =
+            computation.isValueVisibility()
+                ? Context.getLangOpts().getValueVisibilityMode()
+                : Context.getLangOpts().getTypeVisibilityMode();
+        return LinkageInfo(VisibleNoLinkage, globalVisibility,
+                           /*visibilityExplicit=*/false);
+      }
     }
-
-    LV = getLVForDecl(FD, computation);
   }
   if (!isExternallyVisible(LV.getLinkage()))
     return LinkageInfo::none();
   return LinkageInfo(VisibleNoLinkage, LV.getVisibility(),
                      LV.isVisibilityExplicit());
 }
 
 static inline const CXXRecordDecl*

test/CodeGenCXX/visibility-inlines-hidden-staticvar.cpp

 // RUN: %clang_cc1 -triple i386-unknown-unknown -std=c++11 -fvisibility-inlines-hidden -emit-llvm -o - %s -O2 -disable-llvm-passes | FileCheck %s
 // RUN: %clang_cc1 -triple i386-unknown-unknown -std=c++11 -emit-llvm -o - %s -O2 -disable-llvm-passes | FileCheck -check-prefixes=CHECK-NO-VIH %s
+// RUN: %clang_cc1 -triple i386-unknown-unknown -std=c++11 -fvisibility hidden -fvisibility-inlines-hidden -emit-llvm -o - %s -O2 -disable-llvm-passes | FileCheck %s --check-prefix=CHECK-VIS-HIDDEN
+// RUN: %clang_cc1 -triple i386-unknown-unknown -std=c++11 -fvisibility protected -fvisibility-inlines-hidden -emit-llvm -o - %s -O2 -disable-llvm-passes | FileCheck %s --check-prefix=CHECK-VIS-PROTECTED
 
 // When a function is hidden due to -fvisibility-inlines-hidden option, static local variables of the function should not be hidden by the option.
 
 // CHECK-DAG: @_ZZ4funcvE3var = internal global i32 0
 // CHECK-DAG: @_ZZ11hidden_funcvE3var = internal global i32 0
 // CHECK-DAG: @_ZZ12default_funcvE3var = internal global i32 0
 // CHECK-DAG: @_ZZ11inline_funcvE3var = linkonce_odr global i32 0, comdat
 // CHECK-DAG: @_ZZ18inline_hidden_funcvE3var = linkonce_odr hidden global i32 0, comdat
 // CHECK-DAG: @_ZZ19inline_default_funcvE3var = linkonce_odr global i32 0, comdat
+// CHECK-DAG: @_ZZN13ExportedClass10inl_methodEvE3var = linkonce_odr global i32 0, comdat, align 4
 // CHECK-DAG: define i32 @_Z4funcv()
 // CHECK-DAG: define hidden i32 @_Z11hidden_funcv()
 // CHECK-DAG: define i32 @_Z12default_funcv()
 // CHECK-DAG: define linkonce_odr hidden i32 @_Z11inline_funcv()
 // CHECK-DAG: define linkonce_odr hidden i32 @_Z18inline_hidden_funcv()
 // CHECK-DAG: define linkonce_odr i32 @_Z19inline_default_funcv()
+// CHECK-DAG: define linkonce_odr hidden i32 @_ZN13ExportedClass10inl_methodEv({{.*}})
+// CHECK-DAG: define i32 @_ZN13ExportedClass10ext_methodEv({{.*}})
 
 // CHECK-NO-VIH-DAG: @_ZZ4funcvE3var = internal global i32 0
 // CHECK-NO-VIH-DAG: @_ZZ11hidden_funcvE3var = internal global i32 0
 // CHECK-NO-VIH-DAG: @_ZZ12default_funcvE3var = internal global i32 0
 // CHECK-NO-VIH-DAG: @_ZZ11inline_funcvE3var = linkonce_odr global i32 0, comdat
 // CHECK-NO-VIH-DAG: @_ZZ18inline_hidden_funcvE3var = linkonce_odr hidden global i32 0, comdat
 // CHECK-NO-VIH-DAG: @_ZZ19inline_default_funcvE3var = linkonce_odr global i32 0, comdat
+// CHECK-NO-VIH-DAG: @_ZZN13ExportedClass10inl_methodEvE3var = linkonce_odr global i32 0, comdat, align 4
 // CHECK-NO-VIH-DAG: define i32 @_Z4funcv()
 // CHECK-NO-VIH-DAG: define hidden i32 @_Z11hidden_funcv()
 // CHECK-NO-VIH-DAG: define i32 @_Z12default_funcv()
 // CHECK-NO-VIH-DAG: define linkonce_odr i32 @_Z11inline_funcv()
 // CHECK-NO-VIH-DAG: define linkonce_odr hidden i32 @_Z18inline_hidden_funcv()
 // CHECK-NO-VIH-DAG: define linkonce_odr i32 @_Z19inline_default_funcv()
+// CHECK-NO-VIH-DAG: define linkonce_odr i32 @_ZN13ExportedClass10inl_methodEv({{.*}})
+// CHECK-NO-VIH-DAG: define i32 @_ZN13ExportedClass10ext_methodEv({{.*}})
 
+// CHECK-VIS-HIDDEN-DAG: @_ZZ4funcvE3var = internal global i32 0
+// CHECK-VIS-HIDDEN-DAG: @_ZZ11hidden_funcvE3var = internal global i32 0
+// CHECK-VIS-HIDDEN-DAG: @_ZZ12default_funcvE3var = internal global i32 0
+// CHECK-VIS-HIDDEN-DAG: @_ZZ11inline_funcvE3var = linkonce_odr hidden global i32 0, comdat
+// CHECK-VIS-HIDDEN-DAG: @_ZZ18inline_hidden_funcvE3var = linkonce_odr hidden global i32 0, comdat
+// CHECK-VIS-HIDDEN-DAG: @_ZZ19inline_default_funcvE3var = linkonce_odr global i32 0, comdat
+// CHECK-VIS-HIDDEN-DAG: @_ZZN13ExportedClass10inl_methodEvE3var = linkonce_odr global i32 0, comdat, align 4
+// CHECK-VIS-HIDDEN-DAG: define hidden i32 @_Z4funcv()
+// CHECK-VIS-HIDDEN-DAG: define hidden i32 @_Z11hidden_funcv()
+// CHECK-VIS-HIDDEN-DAG: define i32 @_Z12default_funcv()
+// CHECK-VIS-HIDDEN-DAG: define linkonce_odr hidden i32 @_Z11inline_funcv()
+// CHECK-VIS-HIDDEN-DAG: define linkonce_odr hidden i32 @_Z18inline_hidden_funcv()
+// CHECK-VIS-HIDDEN-DAG: define linkonce_odr i32 @_Z19inline_default_funcv()
+// CHECK-VIS-HIDDEN-DAG: define linkonce_odr hidden i32 @_ZN13ExportedClass10inl_methodEv({{.*}})
+// CHECK-VIS-HIDDEN-DAG: define i32 @_ZN13ExportedClass10ext_methodEv({{.*}})
+
+// CHECK-VIS-PROTECTED-DAG: @_ZZ4funcvE3var = internal global i32 0
+// CHECK-VIS-PROTECTED-DAG: @_ZZ11hidden_funcvE3var = internal global i32 0
+// CHECK-VIS-PROTECTED-DAG: @_ZZ12default_funcvE3var = internal global i32 0
+// CHECK-VIS-PROTECTED-DAG: @_ZZ11inline_funcvE3var = linkonce_odr protected global i32 0, comdat
+// CHECK-VIS-PROTECTED-DAG: @_ZZ18inline_hidden_funcvE3var = linkonce_odr hidden global i32 0, comdat
+// CHECK-VIS-PROTECTED-DAG: @_ZZ19inline_default_funcvE3var = linkonce_odr global i32 0, comdat
+// CHECK-VIS-PROTECTED-DAG: @_ZZN13ExportedClass10inl_methodEvE3var = linkonce_odr global i32 0, comdat, align 4
+// CHECK-VIS-PROTECTED-DAG: define protected i32 @_Z4funcv()
+// CHECK-VIS-PROTECTED-DAG: define hidden i32 @_Z11hidden_funcv()
+// CHECK-VIS-PROTECTED-DAG: define i32 @_Z12default_funcv()
+// CHECK-VIS-PROTECTED-DAG: define linkonce_odr hidden i32 @_Z11inline_funcv()
+// CHECK-VIS-PROTECTED-DAG: define linkonce_odr hidden i32 @_Z18inline_hidden_funcv()
+// CHECK-VIS-PROTECTED-DAG: define linkonce_odr i32 @_Z19inline_default_funcv()
+// CHECK-VIS-PROTECTED-DAG: define linkonce_odr hidden i32 @_ZN13ExportedClass10inl_methodEv({{.*}})
+// CHECK-VIS-PROTECTED-DAG: define i32 @_ZN13ExportedClass10ext_methodEv({{.*}})
 
 int func(void) {
   static int var = 0;
   return var++;
 }
 inline int inline_func(void) {
   static int var = 0;
   return var++;

 int __attribute__((visibility("default"))) default_func(void) {
   static int var = 0;
   return var++;
 }
 inline int __attribute__((visibility("default"))) inline_default_func(void) {
   static int var = 0;
   return var++;
 }
+struct __attribute__((visibility("default"))) ExportedClass {
+  int inl_method() {
+    static int var = 0;
+    return var++;
+  }
+  int ext_method();
+};
+int ExportedClass::ext_method() { return inl_method(); }
 void bar(void) {
   func();
   inline_func();
   hidden_func();
   inline_hidden_func();
   default_func();
   inline_default_func();
 }
-