Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CodeGen][arm64e] Add methods and data members to Address, which are needed to authenticate signed pointers #67454

Merged
merged 4 commits into from
Mar 26, 2024

Conversation

ahatanak
Copy link
Collaborator

@ahatanak ahatanak commented Sep 26, 2023

To authenticate pointers, CodeGen needs access to the key and discriminators that were used to sign the pointer. That information is sometimes known from the context, but not always, which is why Address needs to hold that information.

This patch adds methods and data members to Address, which will be needed in subsequent patches to authenticate signed pointers, and uses the newly added methods throughout CodeGen. Although this patch isn't strictly NFC as it causes CodeGen to use different code paths in some cases (e.g., mergeAddressesInConditionalExpr), it doesn't cause any changes in functionality as it doesn't add any information needed for authentication.

In addition to the changes mentioned above, this patch introduces class RawAddress, which contains a pointer that we know is unsigned, and adds several new functions for creating Address and LValue objects.

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:Sparc clang:codegen coroutines C++20 coroutines llvm:ir labels Sep 26, 2023
@ahatanak ahatanak self-assigned this Sep 26, 2023
@github-actions
Copy link

github-actions bot commented Sep 26, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@fhahn fhahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be helpful for reviewers to add a brief description to the PR explaining the motivation and implementation

// Indicates whether a pointer is known not to be null.
enum KnownNonNull_t { NotKnownNonNull, KnownNonNull };

/// An aligned address.
class Address {
class RawAddress {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to have a brief comment here documenting RawAddress

@ahatanak ahatanak changed the title [clang][CodeGen] Introduce class RawAddress and modify Address in preparation for adding information to it that is needed for pointer authentication [CodeGen][arm64e] Add methods and data members to Address, which are needed to authenticate signed pointers Oct 6, 2023
/// this is currently redundant with the pointer's type, but for signed
/// pointers it is useful if the pointer has been offsetted or cast from the
/// original type. In the long run, when LLVM adopts opaque pointer types,
/// this should become the notional element type of the address.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment probably needs to be updated?


LValue LV = CGF.MakeNaturalAlignAddrLValue(Address, E->getType());
LValue LV = CGF.MakeAddrLValue(Addr, E->getType());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we land this separately with appropriate tests? It looks like it changes the behavior (the computed alignment of a pointer can be less than natural alignment, I think).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was that the call to EmitPointerWithAlignment, which sets Addr, would compute the correct alignment.

Do you have an example that shows the alignment is lower after this change?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EmitPointerWithAlignment tries to compute the alignment based on the underlying lvalue. This can be higher or lower than the natural alignment of the type. Say you have something like vec f() { struct S { char c[16]; } x; return __temporal_load((vec*)x.c); }. It looks through the cast, sees the field is unaligned, and therefore concludes the pointer is unaligned. This is arguably an improvement, but it's a significant change to the generated code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree that (1) this is an improvement and (2) we should probably isolate all of these alignment changes as separate patches.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch that fixes the alignment: #75675

// Deactivate the cleanup block.
DeactivateCleanupBlock(cleanup,
cast<llvm::Instruction>(typedAddr.getPointer()));
pushFullExprCleanup<FreeException>(EHCleanup, addr.getRawPointer(*this));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new indentation here doesn't look right?

Copy link
Collaborator

@asl asl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the comment about function argument evaluation order that causes instability of the codegen with pauth.

return CreateMemCpy(Dest.getPointer(), Dest.getAlignment().getAsAlign(),
Src.getPointer(), Src.getAlignment().getAsAlign(), Size,
IsVolatile);
return CreateMemCpy(getRawPointerFromAddress(Dest),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we are having a subtle, but big problem. With pauth,getRawPointerFromAddress has a side effect. However, the argument evaluation order is unspecified. As a result, the side effects could appear in arbitrary order.

See access-softek/llvm-project#51 as an outcome of this, when pauth codegen is different on Mac and Linux.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this needs to be fixed.

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 22, 2024

@llvm/pr-subscribers-backend-systemz

@llvm/pr-subscribers-backend-powerpc

Author: Akira Hatanaka (ahatanak)

Changes

To authenticate pointers, CodeGen needs access to the key and discriminators that were used to sign the pointer. That information is sometimes known from the context, but not always, which is why Address needs to hold that information.

This patch adds methods and data members to Address, which will be needed in subsequent patches to authenticate signed pointers, and uses the newly added methods throughout CodeGen. Although this patch isn't strictly NFC as it causes CodeGen to use different code paths in some cases (e.g., mergeAddressesInConditionalExpr), it doesn't cause any changes in functionality as it doesn't add any information needed for authentication.

In addition to the changes mentioned above, this patch introduces class RawAddress, which contains a pointer that we know is unsigned, and adds several new functions for creating Address and LValue objects.


Patch is 349.69 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/67454.diff

50 Files Affected:

  • (modified) clang/lib/CodeGen/ABIInfoImpl.cpp (+5-5)
  • (modified) clang/lib/CodeGen/Address.h (+167-28)
  • (modified) clang/lib/CodeGen/CGAtomic.cpp (+29-26)
  • (modified) clang/lib/CodeGen/CGBlocks.cpp (+19-15)
  • (modified) clang/lib/CodeGen/CGBlocks.h (+2-1)
  • (modified) clang/lib/CodeGen/CGBuilder.h (+160-74)
  • (modified) clang/lib/CodeGen/CGBuiltin.cpp (+90-83)
  • (modified) clang/lib/CodeGen/CGCUDANV.cpp (+10-9)
  • (modified) clang/lib/CodeGen/CGCXXABI.cpp (+15-6)
  • (modified) clang/lib/CodeGen/CGCXXABI.h (+2-12)
  • (modified) clang/lib/CodeGen/CGCall.cpp (+96-73)
  • (modified) clang/lib/CodeGen/CGCall.h (+1)
  • (modified) clang/lib/CodeGen/CGClass.cpp (+45-31)
  • (modified) clang/lib/CodeGen/CGCleanup.cpp (+41-69)
  • (modified) clang/lib/CodeGen/CGCleanup.h (+1-1)
  • (modified) clang/lib/CodeGen/CGCoroutine.cpp (+2-2)
  • (modified) clang/lib/CodeGen/CGDecl.cpp (+14-11)
  • (modified) clang/lib/CodeGen/CGException.cpp (+10-8)
  • (modified) clang/lib/CodeGen/CGExpr.cpp (+119-108)
  • (modified) clang/lib/CodeGen/CGExprAgg.cpp (+16-13)
  • (modified) clang/lib/CodeGen/CGExprCXX.cpp (+47-57)
  • (modified) clang/lib/CodeGen/CGExprConstant.cpp (+2-2)
  • (modified) clang/lib/CodeGen/CGExprScalar.cpp (+19-4)
  • (modified) clang/lib/CodeGen/CGNonTrivialStruct.cpp (+4-4)
  • (modified) clang/lib/CodeGen/CGObjC.cpp (+19-24)
  • (modified) clang/lib/CodeGen/CGObjCGNU.cpp (+22-20)
  • (modified) clang/lib/CodeGen/CGObjCMac.cpp (+47-46)
  • (modified) clang/lib/CodeGen/CGObjCRuntime.cpp (+3-3)
  • (modified) clang/lib/CodeGen/CGOpenMPRuntime.cpp (+102-92)
  • (modified) clang/lib/CodeGen/CGOpenMPRuntime.h (+2-3)
  • (modified) clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp (+37-35)
  • (modified) clang/lib/CodeGen/CGStmt.cpp (+4-4)
  • (modified) clang/lib/CodeGen/CGStmtOpenMP.cpp (+45-42)
  • (modified) clang/lib/CodeGen/CGVTables.cpp (+4-5)
  • (modified) clang/lib/CodeGen/CGValue.h (+124-125)
  • (modified) clang/lib/CodeGen/CodeGenFunction.cpp (+39-28)
  • (modified) clang/lib/CodeGen/CodeGenFunction.h (+186-71)
  • (modified) clang/lib/CodeGen/CodeGenModule.cpp (+1-1)
  • (modified) clang/lib/CodeGen/CodeGenPGO.cpp (+6-4)
  • (modified) clang/lib/CodeGen/CodeGenPGO.h (+4-2)
  • (modified) clang/lib/CodeGen/ItaniumCXXABI.cpp (+22-30)
  • (modified) clang/lib/CodeGen/MicrosoftCXXABI.cpp (+23-35)
  • (modified) clang/lib/CodeGen/TargetInfo.h (+5)
  • (modified) clang/lib/CodeGen/Targets/NVPTX.cpp (+1-1)
  • (modified) clang/lib/CodeGen/Targets/PPC.cpp (+6-5)
  • (modified) clang/lib/CodeGen/Targets/Sparc.cpp (+1-1)
  • (modified) clang/lib/CodeGen/Targets/SystemZ.cpp (+4-5)
  • (modified) clang/lib/CodeGen/Targets/XCore.cpp (+1-1)
  • (modified) clang/utils/TableGen/MveEmitter.cpp (+1-1)
  • (modified) llvm/include/llvm/IR/IRBuilder.h (+1)
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp b/clang/lib/CodeGen/ABIInfoImpl.cpp
index 2b20d5a13346d3..1facf96ff27106 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -187,7 +187,7 @@ CodeGen::emitVoidPtrDirectVAArg(CodeGenFunction &CGF, Address VAListAddr,
   CharUnits FullDirectSize = DirectSize.alignTo(SlotSize);
   Address NextPtr =
       CGF.Builder.CreateConstInBoundsByteGEP(Addr, FullDirectSize, "argp.next");
-  CGF.Builder.CreateStore(NextPtr.getPointer(), VAListAddr);
+  CGF.Builder.CreateStore(NextPtr.getRawPointer(CGF), VAListAddr);
 
   // If the argument is smaller than a slot, and this is a big-endian
   // target, the argument will be right-adjusted in its slot.
@@ -239,8 +239,8 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address Addr1,
                               const llvm::Twine &Name) {
   assert(Addr1.getType() == Addr2.getType());
   llvm::PHINode *PHI = CGF.Builder.CreatePHI(Addr1.getType(), 2, Name);
-  PHI->addIncoming(Addr1.getPointer(), Block1);
-  PHI->addIncoming(Addr2.getPointer(), Block2);
+  PHI->addIncoming(Addr1.getRawPointer(CGF), Block1);
+  PHI->addIncoming(Addr2.getRawPointer(CGF), Block2);
   CharUnits Align = std::min(Addr1.getAlignment(), Addr2.getAlignment());
   return Address(PHI, Addr1.getElementType(), Align);
 }
@@ -400,7 +400,7 @@ Address CodeGen::EmitVAArgInstr(CodeGenFunction &CGF, Address VAListAddr,
     llvm::Type *ElementTy = CGF.ConvertTypeForMem(Ty);
     llvm::Type *BaseTy = llvm::PointerType::getUnqual(ElementTy);
     llvm::Value *Addr =
-        CGF.Builder.CreateVAArg(VAListAddr.getPointer(), BaseTy);
+        CGF.Builder.CreateVAArg(VAListAddr.getRawPointer(CGF), BaseTy);
     return Address(Addr, ElementTy, TyAlignForABI);
   } else {
     assert((AI.isDirect() || AI.isExtend()) &&
@@ -416,7 +416,7 @@ Address CodeGen::EmitVAArgInstr(CodeGenFunction &CGF, Address VAListAddr,
            "Unexpected CoerceToType seen in arginfo in generic VAArg emitter!");
 
     Address Temp = CGF.CreateMemTemp(Ty, "varet");
-    Val = CGF.Builder.CreateVAArg(VAListAddr.getPointer(),
+    Val = CGF.Builder.CreateVAArg(VAListAddr.getRawPointer(CGF),
                                   CGF.ConvertTypeForMem(Ty));
     CGF.Builder.CreateStore(Val, Temp);
     return Temp;
diff --git a/clang/lib/CodeGen/Address.h b/clang/lib/CodeGen/Address.h
index cf48df8f5e7367..89c4717afce20e 100644
--- a/clang/lib/CodeGen/Address.h
+++ b/clang/lib/CodeGen/Address.h
@@ -15,6 +15,7 @@
 #define LLVM_CLANG_LIB_CODEGEN_ADDRESS_H
 
 #include "clang/AST/CharUnits.h"
+#include "clang/AST/Type.h"
 #include "llvm/ADT/PointerIntPair.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/Support/MathExtras.h"
@@ -22,28 +23,41 @@
 namespace clang {
 namespace CodeGen {
 
+class Address;
+class CGBuilderTy;
+class CodeGenFunction;
+class CodeGenModule;
+
 // Indicates whether a pointer is known not to be null.
 enum KnownNonNull_t { NotKnownNonNull, KnownNonNull };
 
-/// An aligned address.
-class Address {
+/// An abstract representation of an aligned address. This is designed to be an
+/// IR-level abstraction, carrying just the information necessary to perform IR
+/// operations on an address like loads and stores.  In particular, it doesn't
+/// carry C type information or allow the representation of things like
+/// bit-fields; clients working at that level should generally be using
+/// `LValue`.
+/// The pointer contained in this class is known to be unsigned.
+class RawAddress {
   llvm::PointerIntPair<llvm::Value *, 1, bool> PointerAndKnownNonNull;
   llvm::Type *ElementType;
   CharUnits Alignment;
 
 protected:
-  Address(std::nullptr_t) : ElementType(nullptr) {}
+  RawAddress(std::nullptr_t) : ElementType(nullptr) {}
 
 public:
-  Address(llvm::Value *Pointer, llvm::Type *ElementType, CharUnits Alignment,
-          KnownNonNull_t IsKnownNonNull = NotKnownNonNull)
+  RawAddress(llvm::Value *Pointer, llvm::Type *ElementType, CharUnits Alignment,
+             KnownNonNull_t IsKnownNonNull = NotKnownNonNull)
       : PointerAndKnownNonNull(Pointer, IsKnownNonNull),
         ElementType(ElementType), Alignment(Alignment) {
     assert(Pointer != nullptr && "Pointer cannot be null");
     assert(ElementType != nullptr && "Element type cannot be null");
   }
 
-  static Address invalid() { return Address(nullptr); }
+  inline RawAddress(Address Addr);
+
+  static RawAddress invalid() { return RawAddress(nullptr); }
   bool isValid() const {
     return PointerAndKnownNonNull.getPointer() != nullptr;
   }
@@ -80,6 +94,133 @@ class Address {
     return Alignment;
   }
 
+  /// Return address with different element type, but same pointer and
+  /// alignment.
+  RawAddress withElementType(llvm::Type *ElemTy) const {
+    return RawAddress(getPointer(), ElemTy, getAlignment(), isKnownNonNull());
+  }
+
+  KnownNonNull_t isKnownNonNull() const {
+    assert(isValid());
+    return (KnownNonNull_t)PointerAndKnownNonNull.getInt();
+  }
+};
+
+/// Like RawAddress, an abstract representation of an aligned address, but the
+/// pointer contained in this class is possibly signed.
+class Address {
+  friend class CGBuilderTy;
+
+  // The boolean flag indicates whether the pointer is known to be non-null.
+  llvm::PointerIntPair<llvm::Value *, 1, bool> Pointer;
+
+  /// The expected IR type of the pointer. Carrying accurate element type
+  /// information in Address makes it more convenient to work with Address
+  /// values and allows frontend assertions to catch simple mistakes.
+  llvm::Type *ElementType = nullptr;
+
+  CharUnits Alignment;
+
+  /// Offset from the base pointer.
+  llvm::Value *Offset = nullptr;
+
+  llvm::Value *getRawPointerSlow(CodeGenFunction &CGF) const;
+
+protected:
+  Address(std::nullptr_t) : ElementType(nullptr) {}
+
+public:
+  Address(llvm::Value *pointer, llvm::Type *elementType, CharUnits alignment,
+          KnownNonNull_t IsKnownNonNull = NotKnownNonNull)
+      : Pointer(pointer, IsKnownNonNull), ElementType(elementType),
+        Alignment(alignment) {
+    assert(pointer != nullptr && "Pointer cannot be null");
+    assert(elementType != nullptr && "Element type cannot be null");
+    assert(!alignment.isZero() && "Alignment cannot be zero");
+  }
+
+  Address(llvm::Value *BasePtr, llvm::Type *ElementType, CharUnits Alignment,
+          llvm::Value *Offset, KnownNonNull_t IsKnownNonNull = NotKnownNonNull)
+      : Pointer(BasePtr, IsKnownNonNull), ElementType(ElementType),
+        Alignment(Alignment), Offset(Offset) {}
+
+  Address(RawAddress RawAddr)
+      : Pointer(RawAddr.isValid() ? RawAddr.getPointer() : nullptr),
+        ElementType(RawAddr.isValid() ? RawAddr.getElementType() : nullptr),
+        Alignment(RawAddr.isValid() ? RawAddr.getAlignment()
+                                    : CharUnits::Zero()) {}
+
+  static Address invalid() { return Address(nullptr); }
+  bool isValid() const { return Pointer.getPointer() != nullptr; }
+
+  /// This function is used in situations where the caller is doing some sort of
+  /// opaque "laundering" of the pointer.
+  void replaceBasePointer(llvm::Value *P) {
+    assert(isValid() && "pointer isn't valid");
+    assert(P->getType() == Pointer.getPointer()->getType() &&
+           "Pointer's type changed");
+    Pointer.setPointer(P);
+    assert(isValid() && "pointer is invalid after replacement");
+  }
+
+  CharUnits getAlignment() const { return Alignment; }
+
+  void setAlignment(CharUnits Value) { Alignment = Value; }
+
+  llvm::Value *getBasePointer() const {
+    assert(isValid() && "pointer isn't valid");
+    return Pointer.getPointer();
+  }
+
+  /// Return the type of the pointer value.
+  llvm::PointerType *getType() const {
+    return llvm::PointerType::get(
+        ElementType,
+        llvm::cast<llvm::PointerType>(Pointer.getPointer()->getType())
+            ->getAddressSpace());
+  }
+
+  /// Return the type of the values stored in this address.
+  llvm::Type *getElementType() const {
+    assert(isValid());
+    return ElementType;
+  }
+
+  /// Return the address space that this address resides in.
+  unsigned getAddressSpace() const { return getType()->getAddressSpace(); }
+
+  /// Return the IR name of the pointer value.
+  llvm::StringRef getName() const { return Pointer.getPointer()->getName(); }
+
+  // This function is called only in CGBuilderBaseTy::CreateElementBitCast.
+  void setElementType(llvm::Type *Ty) {
+    assert(hasOffset() &&
+           "this funcion shouldn't be called when there is no offset");
+    ElementType = Ty;
+  }
+
+  /// Whether the pointer is known not to be null.
+  KnownNonNull_t isKnownNonNull() const {
+    assert(isValid());
+    return (KnownNonNull_t)Pointer.getInt();
+  }
+
+  Address setKnownNonNull() {
+    assert(isValid());
+    Pointer.setInt(KnownNonNull);
+    return *this;
+  }
+
+  bool hasOffset() const { return Offset; }
+
+  llvm::Value *getOffset() const { return Offset; }
+
+  /// Return the pointer contained in this class after authenticating it and
+  /// adding offset to it if necessary.
+  llvm::Value *getRawPointer(CodeGenFunction &CGF) const {
+    return getBasePointer();
+  }
+
   /// Return address with different pointer, but same element type and
   /// alignment.
   Address withPointer(llvm::Value *NewPointer,
@@ -91,61 +232,59 @@ class Address {
   /// Return address with different alignment, but same pointer and element
   /// type.
   Address withAlignment(CharUnits NewAlignment) const {
-    return Address(getPointer(), getElementType(), NewAlignment,
+    return Address(Pointer.getPointer(), getElementType(), NewAlignment,
                    isKnownNonNull());
   }
 
   /// Return address with different element type, but same pointer and
   /// alignment.
   Address withElementType(llvm::Type *ElemTy) const {
-    return Address(getPointer(), ElemTy, getAlignment(), isKnownNonNull());
-  }
-
-  /// Whether the pointer is known not to be null.
-  KnownNonNull_t isKnownNonNull() const {
-    assert(isValid());
-    return (KnownNonNull_t)PointerAndKnownNonNull.getInt();
-  }
-
-  /// Set the non-null bit.
-  Address setKnownNonNull() {
-    assert(isValid());
-    PointerAndKnownNonNull.setInt(true);
-    return *this;
+    if (!hasOffset())
+      return Address(getBasePointer(), ElemTy, getAlignment(), nullptr,
+                     isKnownNonNull());
+    Address A(*this);
+    A.ElementType = ElemTy;
+    return A;
   }
 };
 
+inline RawAddress::RawAddress(Address Addr)
+    : PointerAndKnownNonNull(Addr.isValid() ? Addr.getBasePointer() : nullptr,
+                             Addr.isValid() ? Addr.isKnownNonNull()
+                                            : NotKnownNonNull),
+      ElementType(Addr.isValid() ? Addr.getElementType() : nullptr),
+      Alignment(Addr.isValid() ? Addr.getAlignment() : CharUnits::Zero()) {}
+
 /// A specialization of Address that requires the address to be an
 /// LLVM Constant.
-class ConstantAddress : public Address {
-  ConstantAddress(std::nullptr_t) : Address(nullptr) {}
+class ConstantAddress : public RawAddress {
+  ConstantAddress(std::nullptr_t) : RawAddress(nullptr) {}
 
 public:
   ConstantAddress(llvm::Constant *pointer, llvm::Type *elementType,
                   CharUnits alignment)
-      : Address(pointer, elementType, alignment) {}
+      : RawAddress(pointer, elementType, alignment) {}
 
   static ConstantAddress invalid() {
     return ConstantAddress(nullptr);
   }
 
   llvm::Constant *getPointer() const {
-    return llvm::cast<llvm::Constant>(Address::getPointer());
+    return llvm::cast<llvm::Constant>(RawAddress::getPointer());
   }
 
   ConstantAddress withElementType(llvm::Type *ElemTy) const {
     return ConstantAddress(getPointer(), ElemTy, getAlignment());
   }
 
-  static bool isaImpl(Address addr) {
+  static bool isaImpl(RawAddress addr) {
     return llvm::isa<llvm::Constant>(addr.getPointer());
   }
-  static ConstantAddress castImpl(Address addr) {
+  static ConstantAddress castImpl(RawAddress addr) {
     return ConstantAddress(llvm::cast<llvm::Constant>(addr.getPointer()),
                            addr.getElementType(), addr.getAlignment());
   }
 };
-
 }
 
 // Present a minimal LLVM-like casting interface.
diff --git a/clang/lib/CodeGen/CGAtomic.cpp b/clang/lib/CodeGen/CGAtomic.cpp
index a8d846b4f6a592..841a40d547ea39 100644
--- a/clang/lib/CodeGen/CGAtomic.cpp
+++ b/clang/lib/CodeGen/CGAtomic.cpp
@@ -80,7 +80,7 @@ namespace {
         AtomicSizeInBits = C.toBits(
             C.toCharUnitsFromBits(Offset + OrigBFI.Size + C.getCharWidth() - 1)
                 .alignTo(lvalue.getAlignment()));
-        llvm::Value *BitFieldPtr = lvalue.getBitFieldPointer();
+        llvm::Value *BitFieldPtr = lvalue.getRawBitFieldPointer(CGF);
         auto OffsetInChars =
             (C.toCharUnitsFromBits(OrigBFI.Offset) / lvalue.getAlignment()) *
             lvalue.getAlignment();
@@ -139,13 +139,13 @@ namespace {
     const LValue &getAtomicLValue() const { return LVal; }
     llvm::Value *getAtomicPointer() const {
       if (LVal.isSimple())
-        return LVal.getPointer(CGF);
+        return LVal.getRawPointer(CGF);
       else if (LVal.isBitField())
-        return LVal.getBitFieldPointer();
+        return LVal.getRawBitFieldPointer(CGF);
       else if (LVal.isVectorElt())
-        return LVal.getVectorPointer();
+        return LVal.getRawVectorPointer(CGF);
       assert(LVal.isExtVectorElt());
-      return LVal.getExtVectorPointer();
+      return LVal.getRawExtVectorPointer(CGF);
     }
     Address getAtomicAddress() const {
       llvm::Type *ElTy;
@@ -365,7 +365,7 @@ bool AtomicInfo::emitMemSetZeroIfNecessary() const {
     return false;
 
   CGF.Builder.CreateMemSet(
-      addr.getPointer(), llvm::ConstantInt::get(CGF.Int8Ty, 0),
+      addr.getRawPointer(CGF), llvm::ConstantInt::get(CGF.Int8Ty, 0),
       CGF.getContext().toCharUnitsFromBits(AtomicSizeInBits).getQuantity(),
       LVal.getAlignment().getAsAlign());
   return true;
@@ -1052,7 +1052,8 @@ RValue CodeGenFunction::EmitAtomicExpr(AtomicExpr *E) {
       return getTargetHooks().performAddrSpaceCast(
           *this, V, AS, LangAS::opencl_generic, DestType, false);
     };
-    Args.add(RValue::get(CastToGenericAddrSpace(Ptr.getPointer(),
+
+    Args.add(RValue::get(CastToGenericAddrSpace(Ptr.getRawPointer(*this),
                                                 E->getPtr()->getType())),
              getContext().VoidPtrTy);
 
@@ -1083,10 +1084,10 @@ RValue CodeGenFunction::EmitAtomicExpr(AtomicExpr *E) {
       LibCallName = "__atomic_compare_exchange";
       RetTy = getContext().BoolTy;
       HaveRetTy = true;
-      Args.add(RValue::get(CastToGenericAddrSpace(Val1.getPointer(),
+      Args.add(RValue::get(CastToGenericAddrSpace(Val1.getRawPointer(*this),
                                                   E->getVal1()->getType())),
                getContext().VoidPtrTy);
-      Args.add(RValue::get(CastToGenericAddrSpace(Val2.getPointer(),
+      Args.add(RValue::get(CastToGenericAddrSpace(Val2.getRawPointer(*this),
                                                   E->getVal2()->getType())),
                getContext().VoidPtrTy);
       Args.add(RValue::get(Order), getContext().IntTy);
@@ -1102,7 +1103,7 @@ RValue CodeGenFunction::EmitAtomicExpr(AtomicExpr *E) {
     case AtomicExpr::AO__scoped_atomic_exchange:
     case AtomicExpr::AO__scoped_atomic_exchange_n:
       LibCallName = "__atomic_exchange";
-      Args.add(RValue::get(CastToGenericAddrSpace(Val1.getPointer(),
+      Args.add(RValue::get(CastToGenericAddrSpace(Val1.getRawPointer(*this),
                                                   E->getVal1()->getType())),
                getContext().VoidPtrTy);
       break;
@@ -1117,7 +1118,7 @@ RValue CodeGenFunction::EmitAtomicExpr(AtomicExpr *E) {
       LibCallName = "__atomic_store";
       RetTy = getContext().VoidTy;
       HaveRetTy = true;
-      Args.add(RValue::get(CastToGenericAddrSpace(Val1.getPointer(),
+      Args.add(RValue::get(CastToGenericAddrSpace(Val1.getRawPointer(*this),
                                                   E->getVal1()->getType())),
                getContext().VoidPtrTy);
       break;
@@ -1196,8 +1197,9 @@ RValue CodeGenFunction::EmitAtomicExpr(AtomicExpr *E) {
     if (!HaveRetTy) {
       // Value is returned through parameter before the order.
       RetTy = getContext().VoidTy;
-      Args.add(RValue::get(CastToGenericAddrSpace(Dest.getPointer(), RetTy)),
-               getContext().VoidPtrTy);
+      Args.add(
+          RValue::get(CastToGenericAddrSpace(Dest.getRawPointer(*this), RetTy)),
+          getContext().VoidPtrTy);
     }
     // Order is always the last parameter.
     Args.add(RValue::get(Order),
@@ -1507,7 +1509,7 @@ RValue AtomicInfo::EmitAtomicLoad(AggValueSlot ResultSlot, SourceLocation Loc,
     } else
       TempAddr = CreateTempAlloca();
 
-    EmitAtomicLoadLibcall(TempAddr.getPointer(), AO, IsVolatile);
+    EmitAtomicLoadLibcall(TempAddr.getRawPointer(CGF), AO, IsVolatile);
 
     // Okay, turn that back into the original value or whole atomic (for
     // non-simple lvalues) type.
@@ -1660,9 +1662,9 @@ std::pair<RValue, llvm::Value *> AtomicInfo::EmitAtomicCompareExchange(
   if (shouldUseLibcall()) {
     // Produce a source address.
     Address ExpectedAddr = materializeRValue(Expected);
-    Address DesiredAddr = materializeRValue(Desired);
-    auto *Res = EmitAtomicCompareExchangeLibcall(ExpectedAddr.getPointer(),
-                                                 DesiredAddr.getPointer(),
+    llvm::Value *ExpectedPtr = ExpectedAddr.getRawPointer(CGF);
+    llvm::Value *DesiredPtr = materializeRValue(Desired).getRawPointer(CGF);
+    auto *Res = EmitAtomicCompareExchangeLibcall(ExpectedPtr, DesiredPtr,
                                                  Success, Failure);
     return std::make_pair(
         convertAtomicTempToRValue(ExpectedAddr, AggValueSlot::ignored(),
@@ -1744,7 +1746,7 @@ void AtomicInfo::EmitAtomicUpdateLibcall(
 
   Address ExpectedAddr = CreateTempAlloca();
 
-  EmitAtomicLoadLibcall(ExpectedAddr.getPointer(), AO, IsVolatile);
+  EmitAtomicLoadLibcall(ExpectedAddr.getRawPointer(CGF), AO, IsVolatile);
   auto *ContBB = CGF.createBasicBlock("atomic_cont");
   auto *ExitBB = CGF.createBasicBlock("atomic_exit");
   CGF.EmitBlock(ContBB);
@@ -1758,10 +1760,10 @@ void AtomicInfo::EmitAtomicUpdateLibcall(
                                            AggValueSlot::ignored(),
                                            SourceLocation(), /*AsValue=*/false);
   EmitAtomicUpdateValue(CGF, *this, OldRVal, UpdateOp, DesiredAddr);
+  llvm::Value *ExpectedPtr = ExpectedAddr.getRawPointer(CGF);
+  llvm::Value *DesiredPtr = DesiredAddr.getRawPointer(CGF);
   auto *Res =
-      EmitAtomicCompareExchangeLibcall(ExpectedAddr.getPointer(),
-                                       DesiredAddr.getPointer(),
-                                       AO, Failure);
+      EmitAtomicCompareExchangeLibcall(ExpectedPtr, DesiredPtr, AO, Failure);
   CGF.Builder.CreateCondBr(Res, ExitBB, ContBB);
   CGF.EmitBlock(ExitBB, /*IsFinished=*/true);
 }
@@ -1830,7 +1832,7 @@ void AtomicInfo::EmitAtomicUpdateLibcall(llvm::AtomicOrdering AO,
 
   Address ExpectedAddr = CreateTempAlloca();
 
-  EmitAtomicLoadLibcall(ExpectedAddr.getPointer(), AO, IsVolatile);
+  EmitAtomicLoadLibcall(ExpectedAddr.getRawPointer(CGF), AO, IsVolatile);
   auto *ContBB = CGF.createBasicBlock("atomic_cont");
   auto *ExitBB = CGF.createBasicBlock("atomic_exit");
   CGF.EmitBlock(ContBB);
@@ -1841,10 +1843,10 @@ void AtomicInfo::EmitAtomicUpdateLibcall(llvm::AtomicOrdering AO,
     CGF.Builder.CreateStore(OldVal, DesiredAddr);
   }
   EmitAtomicUpdateValue(CGF, *this, UpdateRVal, DesiredAddr);
+  llvm::Value *ExpectedPtr = ExpectedAddr.getRawPointer(CGF);
+  llvm::Value *DesiredPtr = DesiredAddr.getRawPointer(CGF);
   auto *Res =
-      EmitAtomicCompareExchangeLibcall(ExpectedAddr.getPointer(),
-                                       DesiredAddr.getPointer(),
-                                       AO, Failure);
+      EmitAtomicCompareExchangeLibcall(ExpectedPtr, DesiredPtr, AO, Failure);
   CGF.Builder.CreateCondBr(Res, ExitBB, ContBB);
   CGF.EmitBlock(ExitBB, /*IsFinished=*/true);
 }
@@ -1944,7 +1946,8 @@ void CodeGenFunction::EmitAtomicStore(RValue rvalue, LValue dest,
       args.add(RValue::get(atomics.getAtomicSizeValue()),
                ...
[truncated]

@ahatanak
Copy link
Collaborator Author

I think I've addressed all the feedback I got. Are there any other comments?

@ahatanak
Copy link
Collaborator Author

ahatanak commented Mar 2, 2024

ping

@asl asl self-requested a review March 2, 2024 02:38
@asl
Copy link
Collaborator

asl commented Mar 2, 2024

I am ok with the changes. Though with pauth we'd probably need to rename getRawPointerFromAddress to indicate clearer that it has side effects (like extractRawPointerFromAddress or something like this)

@asl asl added this to the LLVM 19.X Release milestone Mar 3, 2024
@asl
Copy link
Collaborator

asl commented Mar 11, 2024

We discussed the naming thing (extractRawPointerFromAddress vs getRawPointerFromAddress) at pauth call and decided that it would be better to do this renaming now. @ahmedbougacha will coordinate that renaming.

@ahatanak
Copy link
Collaborator Author

I'm not sure extractRawPointerFromAddress conveys the fact that the function might do code-gen instead of just returning some pointer. I wonder if there's a better name.

computeRawPointerFromAddress
genRawPointerFromAddress
generateRawPointerFromAddress
codeGenRawPointerFromAddress

Thoughts?

@asl
Copy link
Collaborator

asl commented Mar 11, 2024

I'm not sure extractRawPointerFromAddress conveys the fact that the function might do code-gen instead of just returning some pointer. I wonder if there's a better name.

computeRawPointerFromAddress genRawPointerFromAddress generateRawPointerFromAddress codeGenRawPointerFromAddress

Thoughts?

I do not have particular preference. But probably like compute / gen slightly more.

@ahatanak
Copy link
Collaborator Author

Maybe emitRawPointerFromAddress is better. I see a lot of functions starting with emit in CodeGen.

@asl
Copy link
Collaborator

asl commented Mar 12, 2024

Maybe emitRawPointerFromAddress is better. I see a lot of functions starting with emit in CodeGen.

Works for me!

preparation for adding information to it that is needed for pointer
authentication
- Fix indentation.
- Update comment for Address:ElementType.
- Fix a bug where calls to getRawPointer in call argument expressions
  were causing the output to be dependent on the compiler.
@ahatanak
Copy link
Collaborator Author

Any other comments? Do the changes look good?

@ahatanak ahatanak merged commit 8bd1f91 into llvm:main Mar 26, 2024
5 checks passed
@ahatanak ahatanak deleted the arm64e-rawaddress branch March 26, 2024 01:06
ahatanak added a commit to ahatanak/llvm-project that referenced this pull request Mar 26, 2024
…ich are needed to authenticate signed pointers (llvm#67454)"

This reverts commit 8bd1f91.

It appears that the commit broke msan bots.
ahatanak added a commit that referenced this pull request Mar 26, 2024
…ich are needed to authenticate signed pointers (#67454)" (#86674)

This reverts commit 8bd1f91.

It appears that the commit broke msan bots.
ahatanak added a commit to ahatanak/llvm-project that referenced this pull request Mar 26, 2024
…needed to authenticate signed pointers (llvm#67454)

To authenticate pointers, CodeGen needs access to the key and
discriminators that were used to sign the pointer. That information is
sometimes known from the context, but not always, which is why `Address`
needs to hold that information.

This patch adds methods and data members to `Address`, which will be
needed in subsequent patches to authenticate signed pointers, and uses
the newly added methods throughout CodeGen. Although this patch isn't
strictly NFC as it causes CodeGen to use different code paths in some
cases (e.g., `mergeAddressesInConditionalExpr`), it doesn't cause any
changes in functionality as it doesn't add any information needed for
authentication.

In addition to the changes mentioned above, this patch introduces class
`RawAddress`, which contains a pointer that we know is unsigned, and
adds several new functions for creating `Address` and `LValue` objects.

This reapplies 8bd1f91. The commit
broke msan bots because LValue::IsKnownNonNull was uninitialized.
ahatanak added a commit to ahatanak/llvm-project that referenced this pull request Mar 27, 2024
…needed to authenticate signed pointers (llvm#67454)

To authenticate pointers, CodeGen needs access to the key and
discriminators that were used to sign the pointer. That information is
sometimes known from the context, but not always, which is why `Address`
needs to hold that information.

This patch adds methods and data members to `Address`, which will be
needed in subsequent patches to authenticate signed pointers, and uses
the newly added methods throughout CodeGen. Although this patch isn't
strictly NFC as it causes CodeGen to use different code paths in some
cases (e.g., `mergeAddressesInConditionalExpr`), it doesn't cause any
changes in functionality as it doesn't add any information needed for
authentication.

In addition to the changes mentioned above, this patch introduces class
`RawAddress`, which contains a pointer that we know is unsigned, and
adds several new functions for creating `Address` and `LValue` objects.

This reapplies 8bd1f91. The commit
broke msan bots because LValue::IsKnownNonNull was uninitialized.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:PowerPC backend:Sparc backend:SystemZ clang:codegen clang:openmp OpenMP related changes to Clang clang Clang issues not falling into any other category coroutines C++20 coroutines llvm:ir
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants