[PATCH] D17347: Use LLVM_LIBRARY_VISIBILITY to prevent LLVM dynamic libraries from export symbols for SROA internals.

[Justin Bogner via llvm-commits]

Owen Anderson <resistor at mac.com> writes:
> resistor created this revision.
> resistor added reviewers: chandlerc, bogner.
> resistor added a subscriber: llvm-commits.
> resistor set the repository for this revision to rL LLVM.
>
> Because the new pass manager exposes the pass types, member functions
> on those passes become exported symbols by default.  We can use
> LLVM_LIBRARY_VISIBILITY to restrict their linkage to be accessible
> within the LLVM dynamic library itself, but not exported from it.

Hiding the symbols from the "private" namespace certainly makes sense,
but I'm not completely sold that hiding the private members really gains
us much. More thoughts below.

> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D17347
>
> Files:
>   include/llvm/Transforms/Scalar/SROA.h
>   lib/Transforms/Scalar/SROA.cpp
>
> Index: lib/Transforms/Scalar/SROA.cpp
> ===================================================================
> --- lib/Transforms/Scalar/SROA.cpp
> +++ lib/Transforms/Scalar/SROA.cpp
> @@ -206,7 +206,7 @@
>  /// for the slices used and we reflect that in this structure. The uses are
>  /// stored, sorted by increasing beginning offset and with unsplittable slices
>  /// starting at a particular offset before splittable slices.
> -class llvm::sroa::AllocaSlices {
> +class LLVM_LIBRARY_VISIBILITY llvm::sroa::AllocaSlices  {
>  public:
>    /// \brief Construct the slices of a particular alloca.
>    AllocaSlices(const DataLayout &DL, AllocaInst &AI);
> @@ -4274,7 +4274,7 @@
>    static char ID;
>  };
>  
> -char SROALegacyPass::ID = 0;
> +char LLVM_LIBRARY_VISIBILITY SROALegacyPass::ID = 0;

It's probably simpler/clearer to handle AllocaSlices and
SROALegacyPass::ID by setting LLVM_LIBRARY_VISIBILITY on the internal
namespace. Ie, in SROA.h we'd just say:

  namespace sroa LLVM_LIBRARY_VISIBILITY {
  class AllocaSliceRewriter;
  class AllocaSlices;
  class Partition;
  class SROALegacyPass;
  }

However we do this though, this part is obvious and LGTM. Hiding the
symbols that are only visible to work around having to support both pass
managers is pure goodness.

>  
>  FunctionPass *llvm::createSROAPass() { return new SROALegacyPass(); }
>  
> Index: include/llvm/Transforms/Scalar/SROA.h
> ===================================================================
> --- include/llvm/Transforms/Scalar/SROA.h
> +++ include/llvm/Transforms/Scalar/SROA.h
> @@ -112,16 +112,19 @@
>  
>    /// Helper used by both the public run method and by the legacy pass.
>    PreservedAnalyses runImpl(Function &F, DominatorTree &RunDT,
> -                            AssumptionCache &RunAC);
> +                            AssumptionCache &RunAC) LLVM_LIBRARY_VISIBILITY;
>  
> -  bool presplitLoadsAndStores(AllocaInst &AI, sroa::AllocaSlices &AS);
> +  bool presplitLoadsAndStores(AllocaInst &AI, sroa::AllocaSlices &AS)
> +         LLVM_LIBRARY_VISIBILITY;
>    AllocaInst *rewritePartition(AllocaInst &AI, sroa::AllocaSlices &AS,
> -                               sroa::Partition &P);
> -  bool splitAlloca(AllocaInst &AI, sroa::AllocaSlices &AS);
> -  bool runOnAlloca(AllocaInst &AI);
> -  void clobberUse(Use &U);
> -  void deleteDeadInstructions(SmallPtrSetImpl<AllocaInst *> &DeletedAllocas);
> -  bool promoteAllocas(Function &F);
> +                               sroa::Partition &P) LLVM_LIBRARY_VISIBILITY;
> +  bool splitAlloca(AllocaInst &AI, sroa::AllocaSlices &AS)
> +         LLVM_LIBRARY_VISIBILITY;
> +  bool runOnAlloca(AllocaInst &AI) LLVM_LIBRARY_VISIBILITY;
> +  void clobberUse(Use &U) LLVM_LIBRARY_VISIBILITY;
> +  void deleteDeadInstructions(SmallPtrSetImpl<AllocaInst *> &DeletedAllocas)
> +         LLVM_LIBRARY_VISIBILITY;
> +  bool promoteAllocas(Function &F) LLVM_LIBRARY_VISIBILITY;

This, on the other hand, seems like a fair bit of boilerplate for not
much gain. These are private methods and are used by run(), so hiding
them won't really help us dead strip more or anything like that, and I
don't really see why passes should be special compared to other LLVM
classes in this regard.

The way I see it, if it's worth hiding the private members here, why
wouldn't we do something to generally hide private members in most or
all of LLVM?

>  };
>  
>  }