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

[Chandler Carruth via llvm-commits]

+1 to what Justin said here. Both the namespace trick and the private
members being questionable.

On Tue, Feb 23, 2016 at 3:19 PM Justin Bogner via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> 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?
>
> >  };
> >
> >  }
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160223/0cce9b78/attachment.html>