[CodeGen] Require use of Address::invalid() for invalid address

[nikic]

Summary

This no longer allows creating an invalid Address through the regular constructor. There were only two places that did this (AggValueSlot and EHCleanupScope) which did this by converting a potential nullptr into an Address. I've fixed both of these by directly storing an Address.

I'm going through a private nullptr_t constructor here, maybe there some kind of nicer pattern that I'm not aware of?

This is intended as a bit of preliminary cleanup for D103465.

[rjmccall]

LGTM

[yubing]

clang/lib/CodeGen/Address.h

79

Has anyone encountered buildfail due to missing "std" before nullptr_t?

[nikic]

clang/lib/CodeGen/Address.h

79

Yeah, this broke a lot of buildbots and should be fixed by https://github.com/llvm/llvm-project/commit/b81450afb6529cd4d1eece46e9945caa5de51c11. Please tell me if you still see a build failure.

clang/lib/CodeGen/Address.h

 
 namespace clang {
 namespace CodeGen {
 
 /// An aligned address.
 class Address {
   llvm::Value *Pointer;
   CharUnits Alignment;
+
+protected:
+  Address(nullptr_t) : Pointer(nullptr) {}
+
 public:
   Address(llvm::Value *pointer, CharUnits alignment)
       : Pointer(pointer), Alignment(alignment) {
-    assert((!alignment.isZero() || pointer == nullptr) &&
-           "creating valid address with invalid alignment");
+    assert(pointer != nullptr && "Pointer cannot be null");
+    assert(!alignment.isZero() && "Alignment cannot be zero");
   }
 
-  static Address invalid() { return Address(nullptr, CharUnits()); }
+  static Address invalid() { return Address(nullptr); }
   bool isValid() const { return Pointer != nullptr; }
 
   llvm::Value *getPointer() const {
     assert(isValid());
     return Pointer;
   }
 
   /// Return the type of the pointer value.
CharUnits getAlignment() const {
     assert(isValid());
     return Alignment;
   }
 };
 
 /// A specialization of Address that requires the address to be an
 /// LLVM Constant.
 class ConstantAddress : public Address {
+  ConstantAddress(nullptr_t) : Address(nullptr) {}
+
 public:
   ConstantAddress(llvm::Constant *pointer, CharUnits alignment)
     : Address(pointer, alignment) {}
 
   static ConstantAddress invalid() {
-    return ConstantAddress(nullptr, CharUnits());
+    return ConstantAddress(nullptr);
   }
 
   llvm::Constant *getPointer() const {
     return llvm::cast<llvm::Constant>(Address::getPointer());
   }
 
   ConstantAddress getBitCast(llvm::Type *ty) const {
     return ConstantAddress(llvm::ConstantExpr::getBitCast(getPointer(), ty),

clang/lib/CodeGen/CGCall.cpp

void CodeGenFunction::EmitCallArg(CallArgList &args, const Expr *E,
 
   // In the Microsoft C++ ABI, aggregate arguments are destructed by the callee.
   // However, we still have to push an EH-only cleanup in case we unwind before
   // we make it to the call.
   if (type->isRecordType() &&
       type->castAs<RecordType>()->getDecl()->isParamDestroyedInCallee()) {
     // If we're using inalloca, use the argument memory.  Otherwise, use a
     // temporary.
-    AggValueSlot Slot;
-    if (args.isUsingInAlloca())
-      Slot = createPlaceholderSlot(*this, type);
-    else
-      Slot = CreateAggTemp(type, "agg.tmp");
+    AggValueSlot Slot = args.isUsingInAlloca()
+        ? createPlaceholderSlot(*this, type) : CreateAggTemp(type, "agg.tmp");
 
     bool DestroyedInCallee = true, NeedsEHCleanup = true;
     if (const auto *RD = type->getAsCXXRecordDecl())
       DestroyedInCallee = RD->hasNonTrivialDestructor();
     else
       NeedsEHCleanup = needsEHCleanup(type.isDestructedType());
 
     if (DestroyedInCallee)

clang/lib/CodeGen/CGCleanup.h

class alignas(8) EHCleanupScope : public EHScope {
   EHScopeStack::stable_iterator EnclosingEH;
 
   /// The dual entry/exit block along the normal edge.  This is lazily
   /// created if needed before the cleanup is popped.
   llvm::BasicBlock *NormalBlock;
 
   /// An optional i1 variable indicating whether this cleanup has been
   /// activated yet.
-  llvm::AllocaInst *ActiveFlag;
+  Address ActiveFlag;
 
   /// Extra information required for cleanups that have resolved
   /// branches through them.  This has to be allocated on the side
   /// because everything on the cleanup stack has be trivially
   /// movable.
   struct ExtInfo {
     /// The destinations of normal branch-afters and branch-throughs.
     llvm::SmallPtrSet<llvm::BasicBlock*, 4> Branches;
public:
   }
 
   EHCleanupScope(bool isNormal, bool isEH, unsigned cleanupSize,
                  unsigned fixupDepth,
                  EHScopeStack::stable_iterator enclosingNormal,
                  EHScopeStack::stable_iterator enclosingEH)
       : EHScope(EHScope::Cleanup, enclosingEH),
         EnclosingNormal(enclosingNormal), NormalBlock(nullptr),
-        ActiveFlag(nullptr), ExtInfo(nullptr), FixupDepth(fixupDepth) {
+        ActiveFlag(Address::invalid()), ExtInfo(nullptr),
+        FixupDepth(fixupDepth) {
     CleanupBits.IsNormalCleanup = isNormal;
     CleanupBits.IsEHCleanup = isEH;
     CleanupBits.IsActive = true;
     CleanupBits.IsLifetimeMarker = false;
     CleanupBits.TestFlagInNormalCleanup = false;
     CleanupBits.TestFlagInEHCleanup = false;
     CleanupBits.CleanupSize = cleanupSize;
 
public:
   bool isEHCleanup() const { return CleanupBits.IsEHCleanup; }
 
   bool isActive() const { return CleanupBits.IsActive; }
   void setActive(bool A) { CleanupBits.IsActive = A; }
 
   bool isLifetimeMarker() const { return CleanupBits.IsLifetimeMarker; }
   void setLifetimeMarker() { CleanupBits.IsLifetimeMarker = true; }
 
-  bool hasActiveFlag() const { return ActiveFlag != nullptr; }
+  bool hasActiveFlag() const { return ActiveFlag.isValid(); }
   Address getActiveFlag() const {
-    return Address(ActiveFlag, CharUnits::One());
+    return ActiveFlag;
   }
   void setActiveFlag(Address Var) {
     assert(Var.getAlignment().isOne());
-    ActiveFlag = cast<llvm::AllocaInst>(Var.getPointer());
+    ActiveFlag = Var;
   }
 
   void setTestFlagInNormalCleanup() {
     CleanupBits.TestFlagInNormalCleanup = true;
   }
   bool shouldTestFlagInNormalCleanup() const {
     return CleanupBits.TestFlagInNormalCleanup;
   }

clang/lib/CodeGen/CGExprAgg.cpp

void AggExprEmitter::withReturnValueSlot(
 
   RValue Src =
       EmitCall(ReturnValueSlot(RetAddr, Dest.isVolatile(), IsResultUnused,
                                Dest.isExternallyDestructed()));
 
   if (!UseTemp)
     return;
 
-  assert(Dest.getPointer() != Src.getAggregatePointer());
+  assert(Dest.isIgnored() || Dest.getPointer() != Src.getAggregatePointer());
   EmitFinalDestCopy(E->getType(), Src);
 
   if (!RequiresDestruction && LifetimeStartInst) {
     // If there's no dtor to run, the copy was the last use of our temporary.
     // Since we're not guaranteed to be in an ExprWithCleanups, clean up
     // eagerly.
     CGF.DeactivateCleanupBlock(LifetimeEndBlock, LifetimeStartInst);
     CGF.EmitLifetimeEnd(LifetimeSizePtr, RetAllocaAddr.getPointer());

clang/lib/CodeGen/CGValue.h

public:
   RValue asAggregateRValue(CodeGenFunction &CGF) const {
     return RValue::getAggregate(getAddress(CGF), isVolatileQualified());
   }
 };
 
 /// An aggregate value slot.
 class AggValueSlot {
   /// The address.
-  llvm::Value *Addr;
+  Address Addr;
 
   // Qualifiers
   Qualifiers Quals;
 
-  unsigned Alignment;
-
   /// DestructedFlag - This is set to true if some external code is
   /// responsible for setting up a destructor for the slot.  Otherwise
   /// the code which constructs it should push the appropriate cleanup.
   bool DestructedFlag : 1;
 
   /// ObjCGCFlag - This is set to true if writing to the memory in the
   /// slot might require calling an appropriate Objective-C GC
   /// barrier.  The exact interaction here is unnecessarily mysterious.
class AggValueSlot {
 
   /// If is set to true, sanitizer checks are already generated for this address
   /// or not required. For instance, if this address represents an object
   /// created in 'new' expression, sanitizer checks for memory is made as a part
   /// of 'operator new' emission and object constructor should not generate
   /// them.
   bool SanitizerCheckedFlag : 1;
 
+  AggValueSlot(Address Addr, Qualifiers Quals, bool DestructedFlag,
+               bool ObjCGCFlag, bool ZeroedFlag, bool AliasedFlag,
+               bool OverlapFlag, bool SanitizerCheckedFlag)
+      : Addr(Addr), Quals(Quals), DestructedFlag(DestructedFlag),
+        ObjCGCFlag(ObjCGCFlag), ZeroedFlag(ZeroedFlag),
+        AliasedFlag(AliasedFlag), OverlapFlag(OverlapFlag),
+        SanitizerCheckedFlag(SanitizerCheckedFlag) {}
+
 public:
   enum IsAliased_t { IsNotAliased, IsAliased };
   enum IsDestructed_t { IsNotDestructed, IsDestructed };
   enum IsZeroed_t { IsNotZeroed, IsZeroed };
   enum Overlap_t { DoesNotOverlap, MayOverlap };
   enum NeedsGCBarriers_t { DoesNotNeedGCBarriers, NeedsGCBarriers };
   enum IsSanitizerChecked_t { IsNotSanitizerChecked, IsSanitizerChecked };
 
public:
   static AggValueSlot forAddr(Address addr,
                               Qualifiers quals,
                               IsDestructed_t isDestructed,
                               NeedsGCBarriers_t needsGC,
                               IsAliased_t isAliased,
                               Overlap_t mayOverlap,
                               IsZeroed_t isZeroed = IsNotZeroed,
                        IsSanitizerChecked_t isChecked = IsNotSanitizerChecked) {
-    AggValueSlot AV;
-    if (addr.isValid()) {
-      AV.Addr = addr.getPointer();
-      AV.Alignment = addr.getAlignment().getQuantity();
-    } else {
-      AV.Addr = nullptr;
-      AV.Alignment = 0;
-    }
-    AV.Quals = quals;
-    AV.DestructedFlag = isDestructed;
-    AV.ObjCGCFlag = needsGC;
-    AV.ZeroedFlag = isZeroed;
-    AV.AliasedFlag = isAliased;
-    AV.OverlapFlag = mayOverlap;
-    AV.SanitizerCheckedFlag = isChecked;
-    return AV;
+    return AggValueSlot(addr, quals, isDestructed, needsGC, isZeroed, isAliased,
+                        mayOverlap, isChecked);
   }
 
   static AggValueSlot
   forLValue(const LValue &LV, CodeGenFunction &CGF, IsDestructed_t isDestructed,
             NeedsGCBarriers_t needsGC, IsAliased_t isAliased,
             Overlap_t mayOverlap, IsZeroed_t isZeroed = IsNotZeroed,
             IsSanitizerChecked_t isChecked = IsNotSanitizerChecked) {
     return forAddr(LV.getAddress(CGF), LV.getQuals(), isDestructed, needsGC,
Qualifiers::ObjCLifetime getObjCLifetime() const {
     return Quals.getObjCLifetime();
   }
 
   NeedsGCBarriers_t requiresGCollection() const {
     return NeedsGCBarriers_t(ObjCGCFlag);
   }
 
   llvm::Value *getPointer() const {
-    return Addr;
+    return Addr.getPointer();
   }
 
   Address getAddress() const {
-    return Address(Addr, getAlignment());
+    return Addr;
   }
 
   bool isIgnored() const {
-    return Addr == nullptr;
+    return !Addr.isValid();
   }
 
   CharUnits getAlignment() const {
-    return CharUnits::fromQuantity(Alignment);
+    return Addr.getAlignment();
   }
 
   IsAliased_t isPotentiallyAliased() const {
     return IsAliased_t(AliasedFlag);
   }
 
   Overlap_t mayOverlap() const {
     return Overlap_t(OverlapFlag);