[llvm] [LLVM] [X86] Fix integer overflows in frame layout for huge frames (PR #101840)

Wesley Wiser via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 21 19:00:20 PDT 2024


wesleywiser wrote:

Looking at the repro mentioned,, I see exactly one difference in the asm between prior to my change and after my change:

```diff
.LBB0_46:                               # %while.body.i.preheader.i
                                        #   in Loop: Header=BB0_10 Depth=1
	movl	-20(%ebp), %ebx                 # 4-byte Reload
-	leal	2147483423(%ebp), %eax
+	leal	-2147483873(%ebp), %eax
	movl	$1, %ecx
	addl	$2147483647, %ebx               # imm = 0x7FFFFFFF
```

If I explicitly truncate the offset back to 32-bits, then I get the same output as before my change:

```diff
@@ -988,7 +988,7 @@ X86RegisterInfo::eliminateFrameIndex(MachineBasicBlock::iterator II,
     assert((Is64Bit || FitsIn32Bits) &&
            "Requesting 64-bit offset in 32-bit immediate!");
     if (Offset != 0 || !tryOptimizeLEAtoMOV(II))
-      MI.getOperand(FIOperandNum + 3).ChangeToImmediate(Offset);
+      MI.getOperand(FIOperandNum + 3).ChangeToImmediate((int64_t)((int)FIOffset + (int)Imm));
   } else {
     // Offset is symbolic. This is extremely rare.
     uint64_t Offset = FIOffset +
```

So I think the actual issue here is that the assertion was [previously written incorrectly](https://github.com/llvm/llvm-project/pull/101840/files#diff-9634de34ebe6c0515879267e2eddfa69a67d18de32f7767c24c09588e85f3654L960-L962) and didn't fire when it should have. 

I guess one possible "fix" here is to just remove the assert. Alternatively, we could truncate the offset back to 32-bit when targeting x86 like we used to. 

Does that seem reasonable? Suggestions? 🙂 

https://github.com/llvm/llvm-project/pull/101840


More information about the llvm-commits mailing list