[llvm][IRBuilder] Introspection for CreateAlignmentAssumption*() functions

[lebedev.ri]

Summary

Clang calls these functions to produce IR for assume-aligned attributes.
I would like to teach UBSAN to verify these assumptions.
For that, i need to access the final pointer on which the check is performed,
and the actual icmp that does the check.

The alternative to this would be to fully re-implement this in clang.

[hfinkel]

include/llvm/IR/IRBuilder.h

2126

Please add comments here documenting the use of this struct and the meaning of the fields.

[lebedev.ri]

Document the AlignmentAssumptionIntrospection struct.

[hfinkel]

include/llvm/IR/IRBuilder.h

2127

Don't mention Clang specifics in IRBuilder's documentation. That's not relevant here.

2131

This is a question?

2132

Also, it's not clear to me why you need this. A sanitizer should get the both the original pointer and the offset to give maximum information to the user. No?

[lebedev.ri]

include/llvm/IR/IRBuilder.h

2132

A sanitizer should get the both the original pointer and the offset

Exactly, *both*. In clang, i know the original pointer, the offset (may be 0), and the alignment.

But the alignment check is performed not on the original pointer, but on f(pointer, offset),
which is in this case (((uintptr_t)pointer) - offset). So if i don't capture the PtrIntValue
(which *is* (((uintptr_t)pointer) - offset)), i will need to re-calculate it elsewhere,
and ensure that both of the calculations stay in sync.

Seems best to re-use the actual existing pre-computed values.

Please see D54590 for the actual compiler-rt tests that show the actual diag output.

[lebedev.ri]

Reword the comments as suggested.

[hfinkel]

include/llvm/IR/IRBuilder.h

2132

Having an interface where you're required to pass a, b, and (a-b), seems a bit gratuitous. If we change what the alignment assumption means, or add new parameters, you'll need to update the ubsan interface anyway (because you'll have more things to display to the user).

[lebedev.ri]

include/llvm/IR/IRBuilder.h

2132

• If i only pass a, then i will not be able to display the actual alignment (or it will lie when the offset is non-zero, but it won't even be able to detect that since b wasn't passed).
• If i pass a and b, then two implementations (computing (a-b)) will need to be kept in sync. Without breaking legacy support once/if something changes, not that it is likely to change.
• The only alternative i see is to play it dumb and not display any pointer at all (and do not display the actual alignment).

I agree that it isn't The Best Interface Ever, but from what i see,
it's the one providing the most info without duplication.

Do you have any more concrete suggestions?

[hfinkel]

include/llvm/IR/IRBuilder.h

2132

You should pass a and b. Yes, you'll need to recompute (a-b). But you need to keep these in sync anyway.

My point is: If we change something (e.g., add some additional parameter somehow), such that p != a-b, you'll need to change the interface anyway (i.e., you'll need to add a _foo_v2 ubsan function, or similar), so that you can continue to display accurate and useful information to the user.

[lebedev.ri]

Address review notes: don't store the actual pointer, just the icmp.

[lebedev.ri]

ping

[hfinkel]

LGTM

[lebedev.ri]

In D54588#1309037, @hfinkel wrote:

LGTM

Thank you for the review!

[lebedev.ri]

Rebased, NFC.

[lebedev.ri]

Rebased before commit, NFC.

include/llvm/IR/IRBuilder.h

Value *CreateExtractInteger(const DataLayout &DL, Value *From,
     assert(ExtractedTy->getBitWidth() <= IntTy->getBitWidth() &&
            "Cannot extract to a larger integer!");
     if (ExtractedTy != IntTy) {
       V = CreateTrunc(V, ExtractedTy, Name + ".trunc");
     }
     return V;
   }
 
+  /// Used to optionally propagate some additional information from the
+  /// `CreateAlignmentAssumptionHelper` to the original caller,
+  /// in addition to the normally returned `CallInst *` that was built.
+  struct AlignmentAssumptionIntrospection {
+    /// The actual value on which the alignment check is performed.
+    Value *PtrIntValue = nullptr;
+    /// The alignment check itself (`i1`), which is then used in the assumption.
+    Value *InvCond = nullptr;
+  };
+
 private:
   /// Helper function that creates an assume intrinsic call that
   /// represents an alignment assumption on the provided Ptr, Mask, Type
-  /// and Offset.
-  CallInst *CreateAlignmentAssumptionHelper(const DataLayout &DL,
-                                            Value *PtrValue, Value *Mask,
-                                            Type *IntPtrTy,
-                                            Value *OffsetValue) {
+  /// and Offset. It may be sometimes useful to do some other logic
+  /// based on this alignment check, thus it can be stored into 'Info'.
+  CallInst *CreateAlignmentAssumptionHelper(
+      const DataLayout &DL, Value *PtrValue, Value *Mask, Type *IntPtrTy,
+      Value *OffsetValue, AlignmentAssumptionIntrospection *Info = nullptr) {
     Value *PtrIntValue = CreatePtrToInt(PtrValue, IntPtrTy, "ptrint");
 
     if (OffsetValue) {
       bool IsOffsetZero = false;
       if (const auto *CI = dyn_cast<ConstantInt>(OffsetValue))
         IsOffsetZero = CI->isZero();
 
       if (!IsOffsetZero) {
         if (OffsetValue->getType() != IntPtrTy)
           OffsetValue = CreateIntCast(OffsetValue, IntPtrTy, /*isSigned*/ true,
                                       "offsetcast");
         PtrIntValue = CreateSub(PtrIntValue, OffsetValue, "offsetptr");
       }
     }
 
     Value *Zero = ConstantInt::get(IntPtrTy, 0);
     Value *MaskedPtr = CreateAnd(PtrIntValue, Mask, "maskedptr");
     Value *InvCond = CreateICmpEQ(MaskedPtr, Zero, "maskcond");
+    if (Info) {
+      Info->PtrIntValue = PtrIntValue;
+      Info->InvCond = InvCond;
+    }
     return CreateAssumption(InvCond);
   }
 
 public:
   /// Create an assume intrinsic call that represents an alignment
   /// assumption on the provided pointer.
   ///
   /// An optional offset can be provided, and if it is provided, the offset
   /// must be subtracted from the provided pointer to get the pointer with the
   /// specified alignment.
-  CallInst *CreateAlignmentAssumption(const DataLayout &DL, Value *PtrValue,
-                                      unsigned Alignment,
-                                      Value *OffsetValue = nullptr) {
+  ///
+  /// It may be sometimes useful to do some other logic
+  /// based on this alignment check, thus it can be stored into 'Info'.
+  CallInst *
+  CreateAlignmentAssumption(const DataLayout &DL, Value *PtrValue,
+                            unsigned Alignment, Value *OffsetValue = nullptr,
+                            AlignmentAssumptionIntrospection *Info = nullptr) {
     assert(isa<PointerType>(PtrValue->getType()) &&
            "trying to create an alignment assumption on a non-pointer?");
     auto *PtrTy = cast<PointerType>(PtrValue->getType());
     Type *IntPtrTy = getIntPtrTy(DL, PtrTy->getAddressSpace());
 
     Value *Mask = ConstantInt::get(IntPtrTy, Alignment > 0 ? Alignment - 1 : 0);
     return CreateAlignmentAssumptionHelper(DL, PtrValue, Mask, IntPtrTy,
-                                           OffsetValue);
+                                           OffsetValue, Info);
   }
 
   /// Create an assume intrinsic call that represents an alignment
   /// assumption on the provided pointer.
   ///
   /// An optional offset can be provided, and if it is provided, the offset
   /// must be subtracted from the provided pointer to get the pointer with the
   /// specified alignment.
   ///
+  /// It may be sometimes useful to do some other logic
+  /// based on this alignment check, thus it can be stored into 'Info'.
+  ///
   /// This overload handles the condition where the Alignment is dependent
   /// on an existing value rather than a static value.
-  CallInst *CreateAlignmentAssumption(const DataLayout &DL, Value *PtrValue,
-                                      Value *Alignment,
-                                      Value *OffsetValue = nullptr) {
+  CallInst *
+  CreateAlignmentAssumption(const DataLayout &DL, Value *PtrValue,
+                            Value *Alignment, Value *OffsetValue = nullptr,
+                            AlignmentAssumptionIntrospection *Info = nullptr) {
     assert(isa<PointerType>(PtrValue->getType()) &&
            "trying to create an alignment assumption on a non-pointer?");
     auto *PtrTy = cast<PointerType>(PtrValue->getType());
     Type *IntPtrTy = getIntPtrTy(DL, PtrTy->getAddressSpace());
 
     if (Alignment->getType() != IntPtrTy)
       Alignment = CreateIntCast(Alignment, IntPtrTy, /*isSigned*/ true,
                                 "alignmentcast");
     Value *IsPositive =
         CreateICmp(CmpInst::ICMP_SGT, Alignment,
                    ConstantInt::get(Alignment->getType(), 0), "ispositive");
     Value *PositiveMask =
         CreateSub(Alignment, ConstantInt::get(IntPtrTy, 1), "positivemask");
     Value *Mask = CreateSelect(IsPositive, PositiveMask,
                                ConstantInt::get(IntPtrTy, 0), "mask");
 
     return CreateAlignmentAssumptionHelper(DL, PtrValue, Mask, IntPtrTy,
-                                           OffsetValue);
+                                           OffsetValue, Info);
   }
 };
 
 // Create wrappers for C Binding types (see CBindingWrapping.h).
 DEFINE_SIMPLE_CONVERSION_FUNCTIONS(IRBuilder<>, LLVMBuilderRef)
 
 } // end namespace llvm
 
 #endif // LLVM_IR_IRBUILDER_H