Force visibility of llvm::Any to external

[serge-sans-paille]

Summary

llvm::Any::TypeId::Id relies on the uniqueness of the address of a
static variable defined in a template function. hidden visibility implies vague
linkage for that variable, which does not guarantee the uniqueness of the address
across a binary and a shared library. This totally breaks the implementation of
llvm::Any.

See https://gcc.gnu.org/wiki/Visibility and https://gcc.gnu.org/onlinedocs/gcc/Vague-Linkage.html
for more information on that topic.

[tstellar]

Adding @rnk, who reviewed the original patch for awareness...

It's unfortunate, but I'm fine with this change. We could workaround the specific issue with llvm::Any another way, but there is nothing in place that would prevent someone else from adding a new feature that would hit the same issue again. It seems like disabling this completely is the correct approach, and we will need to come up with a different strategy to limit the number of exported symbols.

[rnk]

As an example, llvm::Any::TypeId::Id relies on the uniqueness of the address of a
static variable defined in a template function.

I just read that line, and I'm thinking to myself, obviously that won't work, don't do that, it doesn't work with DLLs, it doesn't work with hidden visibility. I *just* helped prepare a talk explaining that this code pattern isn't portable. Ideally, I would like to move in the direction of compiling *more* of LLVM with hidden visibility, not less. LLVM shared libraries already have far too many dynamic symbols. I believe Lang Hames attempted to use this code pattern in the past to refactor the pass ID into a template base class, and we reverted it for the same reasons. I wasn't able to quickly find the patch for reference.

What code actually uses llvm::Any? I only see 8 includes of Any.h across the codebase. It looks like @zturner added it in 2018 for use in unit tests. Could we remove it? If we do need it, surely the standard C++17 implementation of std::any supports compiling with fvisibility=hidden, so could we just use whatever solution the STL uses? Maybe just put default visibility on the typeid global?

[tstellar]

@rnk I agree with

In D101972#2742231, @rnk wrote:

As an example, llvm::Any::TypeId::Id relies on the uniqueness of the address of a
static variable defined in a template function.

I just read that line, and I'm thinking to myself, obviously that won't work, don't do that, it doesn't work with DLLs, it doesn't work with hidden visibility. I *just* helped prepare a talk explaining that this code pattern isn't portable. Ideally, I would like to move in the direction of compiling *more* of LLVM with hidden visibility, not less. LLVM shared libraries already have far too many dynamic symbols. I believe Lang Hames attempted to use this code pattern in the past to refactor the pass ID into a template base class, and we reverted it for the same reasons. I wasn't able to quickly find the patch for reference.

What code actually uses llvm::Any? I only see 8 includes of Any.h across the codebase. It looks like @zturner added it in 2018 for use in unit tests. Could we remove it? If we do need it, surely the standard C++17 implementation of std::any supports compiling with fvisibility=hidden, so could we just use whatever solution the STL uses? Maybe just put default visibility on the typeid global?

I also would really like to see more of LLVM with hidden visibility too. My main concern with this issue is that without some kind of enforcement in place, this same code pattern is going to get reintroduced somewhere else in the codebase. and debugging this issue was very costly. Do you have any ideas for how we might be able to ensure this pattern doesn't re-emerge?

[serge-sans-paille]

@rnk it's used in the newPassManager. forcing the visibility of llvm::Any indeed fixes the bug I met (that was my original patch), but I also understand the concern of @tstellar concerning the possibility of having similar issue across the code base. A possible way to solve the issue the hard way would be to design a clang warning / a static analysis that spots this kind of issues. Thinking of it, maybe the check could be done at link time to, or on the final shared library?

[rnk]

In D101972#2742314, @tstellar wrote:

I also would really like to see more of LLVM with hidden visibility too. My main concern with this issue is that without some kind of enforcement in place, this same code pattern is going to get reintroduced somewhere else in the codebase. and debugging this issue was very costly. Do you have any ideas for how we might be able to ensure this pattern doesn't re-emerge?

I don't have any brilliant ideas other than developer education. This is just one of those weird portable development things that you need to know as a C++ developer. It's a difference between C++ as it is standardized and C++ as it works in practice.

[tstellar]

@rnk Even if we were to commit this patch, do you think we would still need to fix/remove llvm:Any?

[rnk]

In D101972#2743043, @tstellar wrote:

@rnk Even if we were to commit this patch, do you think we would still need to fix/remove llvm:Any?

Yes, even if we commit this patch, it seems likely that someone would still run into these kinds of issues later. Passing an Any instance from one DLL to another and then attempting to cast wouldn't work, the ids will be different, and I don't think there is an easy way to fix it.

[serge-sans-paille]

https://github.com/llvm/llvm-project/blob/d480f968ad8b56d3ee4a6b6df5532d485b0ad01e/libcxx/include/any#L159

This is what libcxx does for std::any when rti is not available.

[rnk]

In D101972#2743698, @serge-sans-paille wrote:

https://github.com/llvm/llvm-project/blob/d480f968ad8b56d3ee4a6b6df5532d485b0ad01e/libcxx/include/any#L159

This is what libcxx does for std::any when rti is not available.

Right, _LIBCPP_TEMPLATE_VIS presumably expands to __attribute__((visibility("default"))). I think it's reasonable to go ahead and do the same thing in LLVM. Enabling RTTI and using typeid is probably the only way to make this work well in the general case.

[dcavalca]

FYI, without this patch we're seeing consistent test failures when building LLVM 12 in CentOS Stream 8:

Failed Tests (10):
  LLVM-Unit :: IR/./IRTests/CGSCCCallbacksTest.InstrumentedInvalidatingPasses
  LLVM-Unit :: IR/./IRTests/CGSCCCallbacksTest.InstrumentedPasses
  LLVM-Unit :: IR/./IRTests/CGSCCCallbacksTest.InstrumentedSkippedPasses
  LLVM-Unit :: IR/./IRTests/FunctionCallbacksTest.InstrumentedPasses
  LLVM-Unit :: IR/./IRTests/FunctionCallbacksTest.InstrumentedSkippedPasses
  LLVM-Unit :: IR/./IRTests/LoopCallbacksTest.InstrumentedInvalidatingLoopNestPasses
  LLVM-Unit :: IR/./IRTests/LoopCallbacksTest.InstrumentedInvalidatingPasses
  LLVM-Unit :: IR/./IRTests/LoopCallbacksTest.InstrumentedPasses
  LLVM-Unit :: IR/./IRTests/LoopCallbacksTest.InstrumentedSkippedPasses
  LLVM-Unit :: IR/./IRTests/ModuleCallbacksTest.InstrumentedSkippedPasses

See https://cbs.centos.org/koji/taskinfo?taskID=2483905 for logs and https://bugzilla.redhat.com/show_bug.cgi?id=1952248 for more details. Thanks @tstellar for pointing out this fix!

[rnk]

So, should we go ahead and apply default visibility to llvm::Any::TypeId<>::Id?

Coincidentally, people have discovered the same problem in absl, Google's general C++ utility library: https://crbug.com/1096380

[serge-sans-paille]

@rnk setting the whole class as external is the less intrusive patch and I've tested it successfully. Maybe it's enough to set the default visibility to id() and template <typename T> const char Any::TypeId<T>::Id = 0 ?

[rnk]

My first thought was to put it on the TypeId class, like so:
template <typename T> struct LLVM_EXTERNAL_VISIBILITY TypeId {
I don't think the id() method visibility matters, and fewer dynamic symbols is usually better.

[tstellar]

FWIW, I was originally against adding LLVM_EXTERNAL_VISIBILITY to the class, because I thought it was just working around a fundamental flaw in -fvisiblity=hidden, but based on the discussion here, I've fine with the proposed solution.

llvm/include/llvm/ADT/Any.h

26

Can you add a comment explaining why we need this here?

[serge-sans-paille]

Force external visibilty on a limited set of symbols

[serge-sans-paille]

@dcavalca can you confirm this fixes your issue?

[rnk]

llvm/include/llvm/ADT/Any.h

113

Sorry for the back and forth, but if it is necessary to mark any_isa with default visibility, then I would prefer your original patch of putting it on the class.

However, I think it should only be necessary to give TypeId<T>::Id default visibility. It shouldn't matter which copy of any_isa or id() gets called, they will all take the address of the same unique Id global.

[serge-sans-paille]

In D101972#2753900, @serge-sans-paille wrote:

@dcavalca can you confirm this fixes your issue?

cc @tstellar I went ahead and ran a validation : these three symbols seems to be enough.
@rnk are you fine with the change now?

[serge-sans-paille]

@rnk, let me double check that.

[serge-sans-paille]

cc @tstellar I went ahead and ran a validation : these three symbols seems to be enough.

I actually messed up with my validation: these three symbols are not enough. I'm verifying that setting the visibility on the whole class works.

[serge-sans-paille]

I've successfully tested that one and it works fine.

[MaskRay]

(I'll add a note that this is one of the things blocking us from upgrading -Bsymbolic-functions => -Bsymbolic (there is really little benefit for upgrade since data symbols don't matter that much) )

[dcavalca]

In D101972#2753900, @serge-sans-paille wrote:

@dcavalca can you confirm this fixes your issue?

Yes, the latest iteration of this diff works fine for me and all tests are passing: https://cbs.centos.org/koji/taskinfo?taskID=2487471

[serge-sans-paille]

@rnk: does that look like an acceptable solution to you now?

[rnk]

Yes, we should land this and fix the issue. Sorry for the delay, I think I started to write something and got distracted.

I'm a bit concerned that we don't seem to understand the fundamental issue. From reading the code, it seems clear that the Id static data member should have default visibility. It's not clear why anything else needs default visibility, and that is unsettling.

llvm/include/llvm/ADT/Any.h

26

Please do add a comment here. I'd suggest:

The `Typeid<T>::Id` static data member below is a globally unique identifier for the type `T`. It is explicitly marked with default visibility so that when `-fvisibility=hidden` is used, the loader still merges duplicate definitions across DSO boundaries.

[serge-sans-paille]

Patch updated with the advised comment.

I second @rnk opinion on this patch being non-optimal, but I also think we need to land it as is for 12.0.1 release.

[tstellar]

I just tried building LLVM 12.0.1 with clang (12.0.1) and LTO enabled, and I'm seeing these failures again. I thought I had tested this combination before and it worked, but maybe I didn't actually do this.

[serge-sans-paille]

After weeks of painful quest, I now understand why this is not enough:

compiling the following:

class __attribute__((visibility("default"))) foo{};
class __attribute__((visibility("hidden"))) bar{};

template<class T>
__attribute__((visibility("default"))) bool check() __attribute__((noinline)) {
  return true;
}

void pain(bool& x, bool& y) {
  x = check<foo>();
  y = check<bar>();
}

with

clang++ -shared -fPIC -o liba.so a.cpp -nostdlib

and inspecting the symbol table with

nm liba.so -DC

yields

0000000000201020 D __bss_start
0000000000201020 D _edata
0000000000201020 D _end
0000000000000330 T pain(bool&, bool&)
0000000000000360 W bool check<foo>()

interestingly, check<bar> is not listed, because a default linkage function parametrized by an hidden linkage class turns into an hidden linkage function (!). So a potential fix here would be to flag every llvm::Any::TypeID parameter class as default linkage (but we can't static assert on that, because we can't inspect such attributes). So the way forward really should be in the spirit of https://reviews.llvm.org/D108690

[rnk]

Thanks for the explanation, that makes sense to me.

I think, for portability reasons, we really don't want to go in the direction of D108690. We want to move in the direction of default hidden visibility, and explicitly marking publicly visible things.

Can the issue you've encountered be addressed by giving the classes which are passed across DSO boundaries default visibility? The issue shouldn't arise if a hidden class is not used across DSO boundaries, but if there are two template instantiations of the same class in two DSOs and that causes an issue, that suggests that such classes are being used in both DSOs.

Going further, I think it would be incorrect for the linker/loader to behave differently: I should be able to provide two different definitions of the hidden class bar in separate DSOs and neither they nor their template instantiations should conflict.

[serge-sans-paille]

@rnk : something in the spirit of https://reviews.llvm.org/D108943 is indeed much less intrusive

llvm/include/llvm/ADT/Any.h

 #include "llvm/ADT/STLExtras.h"
 
 #include <cassert>
 #include <memory>
 #include <type_traits>
 
 namespace llvm {
 
-class Any {
+class LLVM_EXTERNAL_VISIBILITY Any {
   template <typename T> struct TypeId { static const char Id; };
 
   struct StorageBase {
     virtual ~StorageBase() = default;
     virtual std::unique_ptr<StorageBase> clone() const = 0;
     virtual const void *id() const = 0;
   };
 
private:
   template <class T> friend T *any_cast(Any *Value);
   template <typename T> friend bool any_isa(const Any &Value);
 
   std::unique_ptr<StorageBase> Storage;
 };
 
 template <typename T> const char Any::TypeId<T>::Id = 0;
 
-
 template <typename T> bool any_isa(const Any &Value) {
   if (!Value.Storage)
     return false;
   return Value.Storage->id() == &Any::TypeId<remove_cvref_t<T>>::Id;
 }
 
 template <class T> T any_cast(const Any &Value) {
   return static_cast<T>(*any_cast<remove_cvref_t<T>>(&Value));
 }