[GlobalOpt] Fix a miscompile when evaluating struct initializers.

[jroelofs]

Summary

The bug was that evaluateBitcastFromPtr attempts a narrowing to a struct's 0th element of a store that covers other elements. While this is okay on the load side, applying it to stores causes us to miss the writes to the additionally covered elements.

rdar://79503568

[Gerolf]

This is straightforward. LGTM.

[efriedma]

llvm/lib/Transforms/Utils/Evaluator.cpp

374

"proposed bitcast load" doesn't make sense. Do you mean to say "The conversion is illegal if the stored type is narrower than the type of the memory location"?

The code would probably be easier to understand if MutatedMemory was represented using a tuple of "(GlobalVariable, Offset, Size)", or something like that, instead of trying to do a delicate dance with ConstantExprs. But you don't need to do that rewrite here.

379

This is a weird way to write Val->getType().

[aeubanks]

llvm/lib/Transforms/Utils/Evaluator.cpp

180

ConstantLoad?

372–382

Can you make this uppercase?

llvm/test/Transforms/GlobalOpt/store-struct-element.ll

3

is this necessary?

[jroelofs]

llvm/lib/Transforms/Utils/Evaluator.cpp

180

I thought renaming would help clarify what the callback is expected to do. Here in some sense it behaves kind of like a constexpr load would, returning a constant if it can evaluate a load from a constant pointer, and nullptr if it cannot.

374

For context, I meant "proposed bitcast load" in the sense that evaluateBitcastFromPtr making queries to its callback first for the whole struct, then for that struct's first element, etc. is a "proposal" with nullptr return being a rejection, and the evaluation of the callback itself being a sort of constexpr-through-a-bitcast. Agreed that the wording is strange; I'll reword.

The code would probably be easier to understand...

I was thinking the same, but wanted to keep the fix simple.

[jroelofs]

review feedback

[jroelofs]

llvm/lib/Transforms/Utils/Evaluator.cpp

180

Just noticed you meant the comment needed updating after renaming it TryLoad.

[jroelofs]

clang-format

[aeubanks]

lgtm

[jroelofs]

llvm/lib/Transforms/Utils/Evaluator.cpp

374

The code would probably be easier to understand if MutatedMemory was represented using a tuple of "(GlobalVariable, Offset, Size)", or something like that, instead of trying to do a delicate dance with ConstantExprs. But you don't need to do that rewrite here.

I'm beginning to think a representational change is *necessary* in order to fix all the issues here. Consider this, for example:

https://llvm.godbolt.org/z/4jhn9PGdW

llvm/lib/Transforms/Utils/Evaluator.cpp

if (CE->getOpcode() == Instruction::GetElementPtr &&
       // external globals.
       return cast<GlobalVariable>(CE->getOperand(0))->hasUniqueInitializer();
     }
   }
 
   return false;
 }
 
-/// Apply 'Func' to Ptr. If this returns nullptr, introspect the pointer's
-/// type and walk down through the initial elements to obtain additional
-/// pointers to try. Returns the first non-null return value from Func, or
-/// nullptr if the type can't be introspected further.
+/// Apply \p TryLoad to Ptr. If this returns \p nullptr, introspect the
+/// pointer's type and walk down through the initial elements to obtain
+/// additional pointers to try. Returns the first non-null return value from
+/// \p TryLoad, or \p nullptr if the type can't be introspected further.
 static Constant *
 evaluateBitcastFromPtr(Constant *Ptr, const DataLayout &DL,
                        const TargetLibraryInfo *TLI,
-                       std::function<Constant *(Constant *)> Func) {
+                       std::function<Constant *(Constant *)> TryLoad) {
   Constant *Val;
-  while (!(Val = Func(Ptr))) {
+  while (!(Val = TryLoad(Ptr))) {
     // If Ty is a non-opaque struct, we can convert the pointer to the struct
     // into a pointer to its first member.
     // FIXME: This could be extended to support arrays as well.
     Type *Ty = cast<PointerType>(Ptr->getType())->getElementType();
     if (!isa<StructType>(Ty) || cast<StructType>(Ty)->isOpaque())
       break;
 
     IntegerType *IdxTy = IntegerType::get(Ty->getContext(), 32);
static Constant *getInitializer(Constant *C) {
   return GV && GV->hasDefinitiveInitializer() ? GV->getInitializer() : nullptr;
 }
 
 /// Return the value that would be computed by a load from P after the stores
 /// reflected by 'memory' have been performed.  If we can't decide, return null.
 Constant *Evaluator::ComputeLoadResult(Constant *P, Type *Ty) {
   // If this memory location has been recently stored, use the stored value: it
   // is the most up-to-date.
-  auto findMemLoc = [this](Constant *Ptr) { return MutatedMemory.lookup(Ptr); };
+  auto TryFindMemLoc = [this](Constant *Ptr) { return MutatedMemory.lookup(Ptr); };
 
-  if (Constant *Val = findMemLoc(P))
+  if (Constant *Val = TryFindMemLoc(P))
     return Val;
 
   // Access it.
   if (GlobalVariable *GV = dyn_cast<GlobalVariable>(P)) {
     if (GV->hasDefinitiveInitializer())
       return GV->getInitializer();
     return nullptr;
   }
 
   if (ConstantExpr *CE = dyn_cast<ConstantExpr>(P)) {
     switch (CE->getOpcode()) {
     // Handle a constantexpr getelementptr.
     case Instruction::GetElementPtr:
       if (auto *I = getInitializer(CE->getOperand(0)))
         return ConstantFoldLoadThroughGEPConstantExpr(I, CE, Ty, DL);
       break;
     // Handle a constantexpr bitcast.
     case Instruction::BitCast:
       // We're evaluating a load through a pointer that was bitcast to a
       // different type. See if the "from" pointer has recently been stored.
       // If it hasn't, we may still be able to find a stored pointer by
       // introspecting the type.
       Constant *Val =
-          evaluateBitcastFromPtr(CE->getOperand(0), DL, TLI, findMemLoc);
+          evaluateBitcastFromPtr(CE->getOperand(0), DL, TLI, TryFindMemLoc);
       if (!Val)
         Val = getInitializer(CE->getOperand(0));
       if (Val)
         return ConstantFoldLoadThroughBitcast(
             Val, P->getType()->getPointerElementType(), DL);
       break;
     }
   }
if (StoreInst *SI = dyn_cast<StoreInst>(CurInst)) {
                      << "Attempting to resolve bitcast on constant ptr.\n");
           // If we're evaluating a store through a bitcast, then we need
           // to pull the bitcast off the pointer type and push it onto the
           // stored value. In order to push the bitcast onto the stored value,
           // a bitcast from the pointer's element type to Val's type must be
           // legal. If it's not, we can try introspecting the type to find a
           // legal conversion.
 
-          auto castValTy = [&](Constant *P) -> Constant * {
-            Type *Ty = cast<PointerType>(P->getType())->getElementType();
-            if (Constant *FV = ConstantFoldLoadThroughBitcast(Val, Ty, DL)) {
+          auto TryCastValTy = [&](Constant *P) -> Constant * {
+            // The conversion is illegal if the store is wider than the
+            // pointee proposed by `evaluateBitcastFromPtr`, since that would
+            // drop stores to other struct elements when the caller attempts to
+            // look through a struct's 0th element.
+            Type *NewTy = cast<PointerType>(P->getType())->getElementType();
+            Type *STy = Val->getType();
+            if (DL.getTypeSizeInBits(NewTy) < DL.getTypeSizeInBits(STy))
+              return nullptr;
+
+            if (Constant *FV = ConstantFoldLoadThroughBitcast(Val, NewTy, DL)) {
               Ptr = P;
               return FV;
             }
             return nullptr;
           };
 
           Constant *NewVal =
-              evaluateBitcastFromPtr(CE->getOperand(0), DL, TLI, castValTy);
+              evaluateBitcastFromPtr(CE->getOperand(0), DL, TLI, TryCastValTy);
           if (!NewVal) {
             LLVM_DEBUG(dbgs() << "Failed to bitcast constant ptr, can not "
                                  "evaluate.\n");
             return false;
           }
 
           Val = NewVal;
           LLVM_DEBUG(dbgs() << "Evaluated bitcast: " << *Val << "\n");

llvm/test/Transforms/GlobalOpt/store-struct-element.ll

+; RUN: opt < %s -globalopt -S -o - | FileCheck %s
+
+%class.Class = type { i8, i8, i8, i8 }
+@A = local_unnamed_addr global %class.Class undef, align 4
+@B = local_unnamed_addr global %class.Class undef, align 4
+
[e-mail]_ctors = appending global [2 x { i32, void ()*, i8* }] [
+  { i32, void ()*, i8* } { i32 65535, void ()* @initA, i8* null },
+  { i32, void ()*, i8* } { i32 65535, void ()* @initB, i8* null }
+]
+
+define internal void @initA() section "__TEXT,__StaticInit,regular,pure_instructions" {
+entry:
+  store i32 -1, i32* bitcast (%class.Class* @A to i32*), align 4
+  ret void
+}
+
+define internal void @initB() section "__TEXT,__StaticInit,regular,pure_instructions" {
+entry:
+  store i8 -1, i8* bitcast (%class.Class* @B to i8*), align 4
+  ret void
+}
+
+; rdar://79503568
+; Check that we don't miscompile when the store covers the whole struct.
+; CHECK-NOT: @A = local_unnamed_addr global %class.Class { i8 -1, i8 undef, i8 undef, i8 undef }, align 4
+
+; FIXME: We could optimzie this as { i8 -1, i8 -1, i8 -1, i8 -1 } if constant folding were a little smarter.
+; CHECK: @A = local_unnamed_addr global %class.Class undef, align 4
+
+; Check that we still perform the transform when store is smaller than the width of the 0th element.
+; CHECK: @B = local_unnamed_addr global %class.Class { i8 -1, i8 undef, i8 undef, i8 undef }, align 4
+
+; CHECK: define internal void @initA()
+; CHECK-NOT: define internal void @initB()
+