Fine grain control over some symbol visibility

[serge-sans-paille]

Summary

Setting -fvisibility=hidden when compiling Target libs has the advantage of
not being intrusive on the codebase, but it also sets the visibility of all
functions within header-only component like ADT. In the end, we end up with
some symbols with hidden visibility within llvm dylib (through the target libs),
and some with external visibility (through other libs). This paves the way for
subtle bugs like https://reviews.llvm.org/D101972

This patch explicitly set the visibility of some classes to default so that
llvm::Any related symbols keep a default visibility. Indeed a template
function with default visibility parametrized by a type with hidden
visibility is granted hidden visibility, and we don't want this for the
uniqueness of llvm::Any::TypeId.

[rnk]

Looks good to me, but I would like to see @tstellar approve as well.

+some people who were interested in visibility annotations based on my very fuzzy memory: @MaskRay @aganea

[MaskRay]

(Background: -fvisibility-inlines-hidden is already the default.)

The description says:

Setting -fvisibility=hidden when compiling Target libs has the advantage of not being intrusive on the codebase, but it also sets the visibility of all functions within header-only component like ADT.

However I cannot connect this with the effect of this patch. With this patch, what exact configurations are now working? Can you show the relevant cflags/ldflags and/or downstream usage?

[rnk]

In D108943#2973580, @MaskRay wrote:

With this patch, what exact configurations are now working? Can you show the relevant cflags/ldflags and/or downstream usage?

Previously the failures were described here:
https://reviews.llvm.org/D101972#2746842
Pointed to this task:
https://cbs.centos.org/koji/taskinfo?taskID=2483905
Extracting the cmake command line from the log produces:
https://reviews.llvm.org/P8273

The interesting part is probably just -DLLVM_BUILD_LLVM_DYLIB:BOOL=ON -DLLVM_LINK_LLVM_DYLIB:BOOL=ON, and that it causes some IRTests unit tests to fail (such as IR/./IRTests/CGSCCCallbacksTest.InstrumentedInvalidatingPasses). I haven't replicated this personally.

Presumably what is happening is that llvm::Any::TypeId<llvm::Function>::Id receives hidden visibility, so the typeids of such objects do not compare equal when they are passed between the LLVM DSO and the gtest shell test executable.

[serge-sans-paille]

@MaskRay I've updated the description to reflect the actual impact of that patch wrt. llvm::Any::TypeId.
@rnk I totally agree with your description of the situation.

[tstellar]

LGTM, Thank you.

[MaskRay]

OK. The problem is the hidden TypeId<hidden_class>::Id symbol.

template <typename T> struct TypeId {
  static const char Id;
};
template <typename T> const char TypeId<T>::Id = 0;

struct __attribute__((visibility("hidden"))) SCC {
};

const char *foo() {
  // The symbol TypeId<SCC>::Id is weak hidden.
  return &TypeId<SCC>::Id;
}

Such -fvisibility=hidden built shared objects export very few dynamic symbols (API) so may not be usable by other shared objects...
But perhaps your setup doesn't require the built libLLVM.so to be used by others.

[rnk]

In D108943#2977678, @MaskRay wrote:

Such -fvisibility=hidden built shared objects export very few dynamic symbols (API) so may not be usable by other shared objects...
But perhaps your setup doesn't require the built libLLVM.so to be used by others.

I believe in the current cmake build that -fvisibility=hidden is only passed into compile actions in lib/Target/*. The intention is to eliminate backend library symbols, but expose all other LLVM symbols. It's a coarse grained way of annotating the LLVM API. I'm guessing the problem is that, for those compiles, they see llvm::Function as a hidden class, when really it is intended to be visible.

[MaskRay]

Thanks for the explanation! I find CMAKE_CXX_VISIBILITY_PRESET in llvm/lib/Target/CMakeLists.txt and understand the direction now.
This change makes sense.

llvm/include/llvm/Analysis/LazyCallGraph.h

public:
   /// -- ignoring indirect calls and function references. It stores this as
   /// a collection of call graph nodes. While the order of nodes in the SCC is
   /// stable, it is not any particular order.
   ///
   /// The SCCs are nested within a \c RefSCC, see below for details about that
   /// outer structure. SCCs do not support mutation of the call graph, that
   /// must be done through the containing \c RefSCC in order to fully reason
   /// about the ordering and connections of the graph.
-  class SCC {
+  class LLVM_EXTERNAL_VISIBILITY SCC {
     friend class LazyCallGraph;
     friend class LazyCallGraph::Node;
 
     RefSCC *OuterRefSCC;
     SmallVector<Node *, 1> Nodes;
 
     template <typename NodeRangeT>
     SCC(RefSCC &OuterRefSCC, NodeRangeT &&Nodes)

llvm/include/llvm/Analysis/LoopInfo.h

raw_ostream &operator<<(raw_ostream &OS, const LoopBase<BlockT, LoopT> &Loop) {
   return OS;
 }
 
 // Implementation in LoopInfoImpl.h
 extern template class LoopBase<BasicBlock, Loop>;
 
 /// Represents a single loop in the control flow graph.  Note that not all SCCs
 /// in the CFG are necessarily loops.
-class Loop : public LoopBase<BasicBlock, Loop> {
+class LLVM_EXTERNAL_VISIBILITY Loop : public LoopBase<BasicBlock, Loop> {
 public:
   /// A range representing the start and end location of a loop.
   class LocRange {
     DebugLoc Start;
     DebugLoc End;
 
   public:
     LocRange() {}

llvm/include/llvm/Analysis/LoopNestAnalysis.h

 
 namespace llvm {
 
 using LoopVectorTy = SmallVector<Loop *, 8>;
 
 class LPMUpdater;
 
 /// This class represents a loop nest and can be used to query its properties.
-class LoopNest {
+class LLVM_EXTERNAL_VISIBILITY LoopNest {
 public:
   using InstrVectorTy = SmallVector<const Instruction *>;
 
   /// Construct a loop nest rooted by loop \p Root.
   LoopNest(Loop &Root, ScalarEvolution &SE);
 
   LoopNest() = delete;
 

llvm/include/llvm/CodeGen/MachineFunction.h

struct LandingPadInfo {
   SmallVector<SEHHandler, 1> SEHHandlers;  // SEH handlers active at this lpad.
   MCSymbol *LandingPadLabel = nullptr;     // Label at beginning of landing pad.
   std::vector<int> TypeIds;                // List of type ids (filters negative).
 
   explicit LandingPadInfo(MachineBasicBlock *MBB)
       : LandingPadBlock(MBB) {}
 };
 
-class MachineFunction {
+class LLVM_EXTERNAL_VISIBILITY MachineFunction {
   Function &F;
   const LLVMTargetMachine &Target;
   const TargetSubtargetInfo *STI;
   MCContext &Ctx;
   MachineModuleInfo &MMI;
 
   // RegInfo - Information about each register in use in the function.
   MachineRegisterInfo *RegInfo;

llvm/include/llvm/IR/Function.h

 class Module;
 template <typename T> class Optional;
 class raw_ostream;
 class Type;
 class User;
 class BranchProbabilityInfo;
 class BlockFrequencyInfo;
 
-class Function : public GlobalObject, public ilist_node<Function> {
+class LLVM_EXTERNAL_VISIBILITY Function : public GlobalObject,
+                                          public ilist_node<Function> {
 public:
   using BasicBlockListType = SymbolTableList<BasicBlock>;
 
   // BasicBlock iterators...
   using iterator = BasicBlockListType::iterator;
   using const_iterator = BasicBlockListType::const_iterator;
 
   using arg_iterator = Argument *;

llvm/include/llvm/IR/Module.h

 /// list of globals variables, a list of functions, a list of libraries (or
 /// other modules) this module depends on, a symbol table, and various data
 /// about the target's characteristics.
 ///
 /// A module maintains a GlobalValRefMap object that is used to hold all
 /// constant references to global variables in the module.  When a global
 /// variable is destroyed, it should have no entries in the GlobalValueRefMap.
 /// The main container class for the LLVM Intermediate Representation.
-class Module {
+class LLVM_EXTERNAL_VISIBILITY Module {
   /// @name Types And Enumerations
   /// @{
 public:
   /// The type for the list of global variables.
   using GlobalListType = SymbolTableList<GlobalVariable>;
   /// The type for the list of functions.
   using FunctionListType = SymbolTableList<Function>;
   /// The type for the list of aliases.
   using AliasListType = SymbolTableList<GlobalAlias>;
   /// The type for the list of ifuncs.