[AST] make a static local variable in a fuction hidden by -fvisibility-inlines-hidden visible

[inouehrs]

Summary

LLVM bootstarp fails due to a SEGV in DebugInfo/PDB/pdbdump-debug-subsections.test on PPC64le Fedora 28 if configured with -DBUILD_SHARED_LIBS=ON.
This SEGV is caused by make_shared in the C++ standard library because two copies of a hidden function _Sp_make_shared_tag::_S_ti() are created in llvm-pdbutil binary and libLLVMObjectYAML.so. Two copies of a static local variable __tag in this function are also created and this results in the malfunction of make_shared.
_Sp_make_shared_tag::_S_ti() is marked as hidden due to -fvisibility-inlines-hidden option.

This patch makes static local variable in a method marked as hidden by -fvisibility-inlines-hidden visible from linker by setting the visibility default. It avoids creating two instances of the __tag variable. As long as I tested, gcc does not mark __tag hidden.

Alternatively we can remove -fvisibility-inlines-hidden from LLVM build options, but it may increase the binary size more significantly. Also I think that this is not unique to our problem; other programs using -fvisibility-inlines-hidden may hit the same problem. So, removing -fvisibility-inlines-hidden from LLVM build option may not be a good solution.

[jsji]

GCC option document mentions explicitly that local static variables should NOT be affected by -fvisibility-inlines-hidden.

https://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Dialect-Options.html

-fvisibility-inlines-hidden

...

The behavior of this switch is not quite the same as marking the methods as hidden directly, because it does not affect static variables local to the function or cause the compiler to deduce that the function is defined in only one shared object.

FYI. @steven.zhang

[jsji]

lib/AST/Decl.cpp

1272 ↗

(On Diff #161462)

Is it overreaction here if we always return "DefaultVisibility"? Is it possible that the visibility of this static was already hidden without -fvisibility-inlines-hidden?

Maybe we should consider avoid calling mergeVisibility for local static only?

[rnk]

lib/AST/Decl.cpp

1272 ↗

(On Diff #161462)

I think this incorrectly gives x default visibility when the hidden visibility attribute is explicit and compiling with -fvisibility-inlines-hidden, as in the example below:

inline int *__attribute__((visibility("hidden"))) f() {
  static int x;
  return &x;
}

You should add this test and confirm that we behave as GCC does.

You should also consider cases where the attribute is explicitly put on the variable itself and make sure that is handled.

1275 ↗

(On Diff #161462)

I think the right way to do this is to compute the linkage & visibility of FD, as is done here, check if the visibility is explicit, and if not, under the same conditions that you have above, upgrade the visibility to default.

[inouehrs]

In D50968#1205958, @jsji wrote:

GCC option document mentions explicitly that local static variables should NOT be affected by -fvisibility-inlines-hidden.

Oh, thanks! I have not found this document.

lib/AST/Decl.cpp

1275 ↗

(On Diff #161462)

I agree. I will try to fix this. Thanks.

[rnk]

Oh, this was also reported as https://bugs.llvm.org/show_bug.cgi?id=37595. We should remember to close it when this lands.

[inouehrs]

• check the explicit visibility attribute before changing the visibility to default
• add more tests

[inouehrs]

lib/AST/Decl.cpp

1272 ↗

(On Diff #161462)

As long as I tested, the behavior with this patch is consistent with the behavior of gcc-8.1.

1275 ↗

(On Diff #161462)

I cannot use LV.isVisibilityExplicit() for this check because the hidden visibility by -fvisibility-inlines-hidden is considered as explicit. I use getExplicitVisibility instead.

[steven.zhang]

lib/AST/Decl.cpp

1227 ↗

(On Diff #161664)

Is it better to do the check here ? At some condition, not merge the visibility.

[inouehrs]

lib/AST/Decl.cpp

1227 ↗

(On Diff #161664)

I placed the check in the current position to catch the case if Var->hasExternalStorage() is not true.

[rnk]

It would also be nice if we handled this, but you might consider it out of scope:

inline int __attribute__((visibility("default"))) intentionally_hidden() {
  static int __attribute__((visibility("hidden"))) var;
  return ++var;
}

lib/AST/Decl.cpp

1275 ↗

(On Diff #161462)

Sounds good.

cfe/trunk/lib/AST/Decl.cpp

if (const auto *BD = dyn_cast<BlockDecl>(OuterD)) {
     LV = getLVForClosure(BD->getDeclContext()->getRedeclContext(),
                          BD->getBlockManglingContextDecl(), computation);
   } else {
     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.
+    if (isa<VarDecl>(D) && useInlineVisibilityHidden(FD) &&
+        !(!hasExplicitVisibilityAlready(computation) &&
+          getExplicitVisibility(FD, computation))) {
+      assert(cast<VarDecl>(D)->isStaticLocal());
+      return LinkageInfo(VisibleNoLinkage, DefaultVisibility, false);
+    }
+
     LV = getLVForDecl(FD, computation);
   }
   if (!isExternallyVisible(LV.getLinkage()))
     return LinkageInfo::none();
   return LinkageInfo(VisibleNoLinkage, LV.getVisibility(),
                      LV.isVisibilityExplicit());
 }
 

cfe/trunk/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
+
+// 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: 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-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: 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()
+
+
+int func(void) {
+  static int var = 0;
+  return var++;
+}
+inline int inline_func(void) {
+  static int var = 0;
+  return var++;
+}
+int __attribute__((visibility("hidden"))) hidden_func(void) {
+  static int var = 0;
+  return var++;
+}
+inline int __attribute__((visibility("hidden"))) inline_hidden_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++;
+}
+void bar(void) {
+  func();
+  inline_func();
+  hidden_func();
+  inline_hidden_func();
+  default_func();
+  inline_default_func();
+}
+