Page MenuHomePhabricator

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

Authored by inouehrs on Aug 20 2018, 5:46 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

inouehrs created this revision.Aug 20 2018, 5:46 AM
inouehrs edited the summary of this revision. (Show Details)Aug 20 2018, 5:54 AM

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 added a reviewer: jsji.Aug 20 2018, 8:38 AM
jsji resigned from this revision.Aug 20 2018, 8:43 AM
jsji removed a reviewer: jsji.
jsji added inline comments.
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 added inline comments.Aug 20 2018, 9:38 AM
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.

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 added a comment.Aug 20 2018, 9:55 AM

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 retitled this revision from [AST] make a static local variable in a hidden inlined fuction visible to [AST] make a static local variable in a fuction hidden by -fvisibility-inlines-hidden visible.Aug 20 2018, 9:58 AM
inouehrs updated this revision to Diff 161664.Aug 21 2018, 1:32 AM
  • check the explicit visibility attribute before changing the visibility to default
  • add more tests
inouehrs added inline comments.Aug 21 2018, 1:37 AM
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 added inline comments.Aug 21 2018, 3:35 AM
lib/AST/Decl.cpp
1227 ↗(On Diff #161664)

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

inouehrs added inline comments.Aug 21 2018, 4:03 AM
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 accepted this revision.Aug 21 2018, 12:47 PM

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.

This revision is now accepted and ready to land.Aug 21 2018, 12:47 PM
This revision was automatically updated to reflect the committed changes.