Don't use static variables in LambdaCapture

[john.brawn]

Summary

When static variables are used in inline functions in header files anything that uses that function ends up with a reference to the variable. Because RecursiveASTVisitor uses the inline functions in LambdaCapture that use static variables any AST plugin that uses RecursiveASTVisitor, such as the PrintFunctionNames example, ends up with a reference to these variables. This is bad on Windows when building with MSVC with LLVM_EXPORT_SYMBOLS_FOR_PLUGINS=ON as variables used across a DLL boundary need to be explicitly dllimported in the DLL using them.

This patch avoids that by adjusting LambdaCapture to be similar to before r263921, with a capture of either 'this' or a VLA represented by a null Decl pointer in DeclAndBits with an extra flag added to the bits to distinguish between the two. This requires the use of an extra bit, and while Decl does happen to be sufficiently aligned to allow this it's done in a way that means PointerIntPair doesn't realise it and gives an assertion failure. Therefore I also adjust Decl slightly to use LLVM_ALIGNAS to allow this.

[john.brawn]

Ping

[faisalv]

Seems reasonable to me if we have the bit to spare. I wasn't a huge fan of the sentinel approach - but i appreciated Richard showing me that trick. I don't think I realized that Decl has to be aligned on an 8 byte boundary. I'm not too familiar with the alignment gymnastics - but I'm assuming you've given some thought to any existing pathological code that might break if the actual compile-time alignment of Decl itself is changed (since currently it seems to be that the alignment requirements are established at run-time and only through allocation via new (not that anyone would be creating Decls on the stack)) - and whether there are any consequences of relevance there...
Thanks!

[john.brawn]

In D20732#452055, @faisalv wrote:

I'm assuming you've given some thought to any existing pathological code that might break if the actual compile-time alignment of Decl itself is changed (since currently it seems to be that the alignment requirements are established at run-time and only through allocation via new (not that anyone would be creating Decls on the stack)) - and whether there are any consequences of relevance there...

I can't think of anything that could go wrong because of it. Reducing the alignment could cause problems, if there were things that check that the alignment is at least some value, but here I'm increasing it. Decl and all of its subclasses only have protected or private constructors and the only way to create them is through the various XYZDecl::Create methods which go through new so there should be no problem there either.

[jyknight]

This looks okay. The only downside: it will increase sizeof(Decl) on 32bit platforms, since the layout is:
vtable ptr (4)
NextInContextAndBits ptr (4)
DeclCtx ptr (4)
Loc int (4)
bitfield int (4)

Totaling 20 bytes, which will now be rounded up to 24 bytes. I don't know how others feel, but it seems probably fine to me? On 64-bit platforms, it's already 32 bytes, and that won't be affected.

[john.brawn]

In D20732#456239, @jyknight wrote:

This looks okay. The only downside: it will increase sizeof(Decl) on 32bit platforms, since the layout is:
vtable ptr (4)
NextInContextAndBits ptr (4)
DeclCtx ptr (4)
Loc int (4)
bitfield int (4)

Totaling 20 bytes, which will now be rounded up to 24 bytes. I don't know how others feel, but it seems probably fine to me? On 64-bit platforms, it's already 32 bytes, and that won't be affected.

I did an experiment compiling the top 5 largest llvm/clang source files with and without this patch and got the following numbers for "Maximum resident set size (kbytes)" reported by time -v

Source File	Before	After	Change
X86ISelLowering.cpp	679824 +/- 28	679897 +/- 271	73 (0.01%)
SemaDecl.cpp	705379 +/- 440	705590 +/- 77	211 (0.03%)
SemaExpr.cpp	950729 +/- 6910	951654 +/- 5834	925 (0.10%)
DAGCombiner.cpp	443673 +/- 38	443721 +/- 16	48 (0.01%)
SemaDeclCXX.cpp	850441 +/- 4531	851891 +/- 5079	1450 (0.17%)

I think a <0.2% increase in memory usage is fine.

[jyknight]

Fine with me.

cfe/trunk/include/clang/AST/DeclBase.h

enum AvailabilityResult {
     AR_Unavailable
   };
 
 /// Decl - This represents one declaration (or definition), e.g. a variable,
 /// typedef, function, struct, etc.
 ///
 /// Note: There are objects tacked on before the *beginning* of Decl
 /// (and its subclasses) in its Decl::operator new(). Proper alignment
-/// of all subclasses (not requiring more than DeclObjAlignment) is
+/// of all subclasses (not requiring more than the alignment of Decl) is
 /// asserted in DeclBase.cpp.
-class Decl {
+class LLVM_ALIGNAS(/*alignof(uint64_t)*/ 8) Decl {
 public:
-  /// \brief Alignment guaranteed when allocating Decl and any subtypes.
-  enum { DeclObjAlignment = llvm::AlignOf<uint64_t>::Alignment };
-
   /// \brief Lists the kind of concrete classes of Decl.
   enum Kind {
 #define DECL(DERIVED, BASE) DERIVED,
 #define ABSTRACT_DECL(DECL)
 #define DECL_RANGE(BASE, START, END) \
         first##BASE = START, last##BASE = END,
 #define LAST_DECL_RANGE(BASE, START, END) \
         first##BASE = START, last##BASE = END

cfe/trunk/include/clang/AST/LambdaCapture.h

enum {
     /// \brief Flag used by the Capture class to indicate that the given
     /// capture was implicit.
     Capture_Implicit = 0x01,
 
     /// \brief Flag used by the Capture class to indicate that the
     /// given capture was by-copy.
     ///
     /// This includes the case of a non-reference init-capture.
-    Capture_ByCopy = 0x02
+    Capture_ByCopy = 0x02,
+
+    /// \brief Flag used by the Capture class to distinguish between a capture
+    /// of '*this' and a capture of a VLA type.
+    Capture_This = 0x04
   };
-  struct LLVM_ALIGNAS(4) OpaqueCapturedEntity {};
-  static OpaqueCapturedEntity ThisSentinel;
-  static OpaqueCapturedEntity VLASentinel;
-  
-  // Captured Entity could represent:
+  // Decl could represent:
   // - a VarDecl* that represents the variable that was captured or the 
   //   init-capture.
-  // - or, points to the ThisSentinel if this represents a capture of '*this'
-  //   by value or reference.
-  // - or, points to the VLASentinel if this represents a capture of a VLA type.
-  llvm::PointerIntPair<void*, 2> CapturedEntityAndBits;
+  // - or, is a nullptr and Capture_This is set in Bits if this represents a
+  //   capture of '*this' by value or reference.
+  // - or, is a nullptr and Capture_This is not set in Bits if this represents
+  //   a capture of a VLA type.
+  llvm::PointerIntPair<Decl*, 3> DeclAndBits;
 
   SourceLocation Loc;
   SourceLocation EllipsisLoc;
 
   friend class ASTStmtReader;
   friend class ASTStmtWriter;
 
 public:
LambdaCapture(SourceLocation Loc, bool Implicit, LambdaCaptureKind Kind,
                 SourceLocation EllipsisLoc = SourceLocation());
 
   /// \brief Determine the kind of capture.
   LambdaCaptureKind getCaptureKind() const;
 
   /// \brief Determine whether this capture handles the C++ \c this
   /// pointer.
   bool capturesThis() const {
-    return CapturedEntityAndBits.getPointer() == &ThisSentinel;
+    return DeclAndBits.getPointer() == nullptr &&
+          (DeclAndBits.getInt() & Capture_This);
   }
 
   /// \brief Determine whether this capture handles a variable.
   bool capturesVariable() const {
-    void *Ptr = CapturedEntityAndBits.getPointer();
-    if (Ptr != &ThisSentinel && Ptr != &VLASentinel)
-      return dyn_cast_or_null<VarDecl>(static_cast<Decl *>(Ptr));
-    return false;
+    return dyn_cast_or_null<VarDecl>(DeclAndBits.getPointer());
   }
 
   /// \brief Determine whether this captures a variable length array bound
   /// expression.
   bool capturesVLAType() const {
-    return CapturedEntityAndBits.getPointer() == &VLASentinel;
+    return DeclAndBits.getPointer() == nullptr &&
+           !(DeclAndBits.getInt() & Capture_This);
   }
 
   /// \brief Retrieve the declaration of the local variable being
   /// captured.
   ///
   /// This operation is only valid if this capture is a variable capture
   /// (other than a capture of \c this).
   VarDecl *getCapturedVar() const {
     assert(capturesVariable() && "No variable available for capture");
-    return static_cast<VarDecl *>(CapturedEntityAndBits.getPointer());
+    return static_cast<VarDecl *>(DeclAndBits.getPointer());
   }
 
   /// \brief Determine whether this was an implicit capture (not
   /// written between the square brackets introducing the lambda).
   bool isImplicit() const {
-    return CapturedEntityAndBits.getInt() & Capture_Implicit;
+    return DeclAndBits.getInt() & Capture_Implicit;
   }
 
   /// \brief Determine whether this was an explicit capture (written
   /// between the square brackets introducing the lambda).
   bool isExplicit() const { return !isImplicit(); }
 
   /// \brief Retrieve the source location of the capture.
   ///

cfe/trunk/lib/AST/DeclBase.cpp

 #define ABSTRACT_DECL(DECL)
 #include "clang/AST/DeclNodes.inc"
 
 void Decl::updateOutOfDate(IdentifierInfo &II) const {
   getASTContext().getExternalSource()->updateOutOfDateIdentifier(II);
 }
 
 #define DECL(DERIVED, BASE)                                                    \
-  static_assert(Decl::DeclObjAlignment >=                                      \
+  static_assert(llvm::AlignOf<Decl>::Alignment >=                              \
                     llvm::AlignOf<DERIVED##Decl>::Alignment,                   \
                 "Alignment sufficient after objects prepended to " #DERIVED);
 #define ABSTRACT_DECL(DECL)
 #include "clang/AST/DeclNodes.inc"
 
 void *Decl::operator new(std::size_t Size, const ASTContext &Context,
                          unsigned ID, std::size_t Extra) {
   // Allocate an extra 8 bytes worth of storage, which ensures that the
   // resulting pointer will still be 8-byte aligned.
-  static_assert(sizeof(unsigned) * 2 >= DeclObjAlignment,
+  static_assert(sizeof(unsigned) * 2 >= llvm::AlignOf<Decl>::Alignment,
                 "Decl won't be misaligned");
   void *Start = Context.Allocate(Size + Extra + 8);
   void *Result = (char*)Start + 8;
 
   unsigned *PrefixPtr = (unsigned *)Result - 2;
 
   // Zero out the first 4 bytes; this is used to store the owning module ID.
   PrefixPtr[0] = 0;
 
   // Store the global declaration ID in the second 4 bytes.
   PrefixPtr[1] = ID;
 
   return Result;
 }
 
 void *Decl::operator new(std::size_t Size, const ASTContext &Ctx,
                          DeclContext *Parent, std::size_t Extra) {
   assert(!Parent || &Parent->getParentASTContext() == &Ctx);
   // With local visibility enabled, we track the owning module even for local
   // declarations.
   if (Ctx.getLangOpts().ModulesLocalVisibility) {
     // Ensure required alignment of the resulting object by adding extra
     // padding at the start if required.
     size_t ExtraAlign =
-        llvm::OffsetToAlignment(sizeof(Module *), DeclObjAlignment);
+        llvm::OffsetToAlignment(sizeof(Module *),
+                                llvm::AlignOf<Decl>::Alignment);
     char *Buffer = reinterpret_cast<char *>(
         ::operator new(ExtraAlign + sizeof(Module *) + Size + Extra, Ctx));
     Buffer += ExtraAlign;
     return new (Buffer) Module*(nullptr) + 1;
   }
   return ::operator new(Size + Extra, Ctx);
 }
 

cfe/trunk/lib/AST/ExprCXX.cpp

for (unsigned i = 0; i != Args.size(); ++i) {
       if (Args[i]->containsUnexpandedParameterPack())
         ExprBits.ContainsUnexpandedParameterPack = true;
   
       this->Args[i] = Args[i];
     }
   }
 }
 
-LambdaCapture::OpaqueCapturedEntity LambdaCapture::ThisSentinel;
-LambdaCapture::OpaqueCapturedEntity LambdaCapture::VLASentinel;
-
 LambdaCapture::LambdaCapture(SourceLocation Loc, bool Implicit,
                              LambdaCaptureKind Kind, VarDecl *Var,
                              SourceLocation EllipsisLoc)
-  : CapturedEntityAndBits(Var, 0), Loc(Loc), EllipsisLoc(EllipsisLoc)
+  : DeclAndBits(Var, 0), Loc(Loc), EllipsisLoc(EllipsisLoc)
 {
   unsigned Bits = 0;
   if (Implicit)
     Bits |= Capture_Implicit;
   
   switch (Kind) {
   case LCK_StarThis:
     Bits |= Capture_ByCopy;
     // Fall through
   case LCK_This:
     assert(!Var && "'this' capture cannot have a variable!");
-    CapturedEntityAndBits.setPointer(&ThisSentinel);
+    Bits |= Capture_This;
     break;
 
   case LCK_ByCopy:
     Bits |= Capture_ByCopy;
     // Fall through 
   case LCK_ByRef:
     assert(Var && "capture must have a variable!");
     break;
   case LCK_VLAType:
     assert(!Var && "VLA type capture cannot have a variable!");
-    CapturedEntityAndBits.setPointer(&VLASentinel);
     break;
   }
-  CapturedEntityAndBits.setInt(Bits);
+  DeclAndBits.setInt(Bits);
 }
 
 LambdaCaptureKind LambdaCapture::getCaptureKind() const {
-  void *Ptr = CapturedEntityAndBits.getPointer();
-  if (Ptr == &VLASentinel)
+  if (capturesVLAType())
     return LCK_VLAType;
-  const unsigned Bits = CapturedEntityAndBits.getInt();
-  bool CapByCopy = Bits & Capture_ByCopy;
-  if (Ptr == &ThisSentinel)
+  bool CapByCopy = DeclAndBits.getInt() & Capture_ByCopy;
+  if (capturesThis())
     return CapByCopy ? LCK_StarThis : LCK_This;
   return CapByCopy ? LCK_ByCopy : LCK_ByRef;
 }
 
 LambdaExpr::LambdaExpr(QualType T, SourceRange IntroducerRange,
                        LambdaCaptureDefault CaptureDefault,
                        SourceLocation CaptureDefaultLoc,
                        ArrayRef<LambdaCapture> Captures, bool ExplicitParams,