Weaken MFI Max Call Frame Size Assertion

[oskarwirga]

Summary

A year ago when I was not invested at all into compilers, I found an assertion error when building an AArch64 debug build with LTO + CFI, among other combinations.

It was posted as a github issue here: https://github.com/llvm/llvm-project/issues/54088

I took it upon myself to revisit the issue now that I have spent some more time working on LLVM.

[barannikov88]

llvm/test/CodeGen/AArch64/compute-call-frame-size-unreachable-pass.ll

2

The test without -mtriple will run for the default target (LLVM_DEFAULT_TARGET_TRIPLE). Does it trigger on any backend?
If so, can it be moved to llvm/test/CodeGen/Generic?

[barannikov88]

llvm/test/CodeGen/AArch64/compute-call-frame-size-unreachable-pass.ll

2

Ah, never mind, it contains the triple string.
It is a bit of unusual though, llc test don't usually have neither triple nor datalayout.

[oskarwirga]

llvm/test/CodeGen/AArch64/compute-call-frame-size-unreachable-pass.ll

2

I don't know what is usual when it comes to this stuff, happy to change things to make it proper :)

[barannikov88]

I'm not sure this is the correct fix. The assertion specifically checks that MaxCallFrameSize and AdjustsStack computed here are the same as computed by MachineFrameInfo::computeMaxCallFrameSize.
Have you been able to figure out why the results of the calculations are different?

llvm/test/CodeGen/AArch64/compute-call-frame-size-unreachable-pass.ll

2

llc tests usually have -mtriple argument instead of specifying 'target triple' in the source and usually don't have 'target datalayout' (it is inferred from the triple).
This allows you to write multiple RUN lines with different -mtriple's, i.e. makes tests more flexible.

[oskarwirga]

In D151276#4366712, @barannikov88 wrote:

I'm not sure this is the correct fix. The assertion specifically checks that MaxCallFrameSize and AdjustsStack computed here are the same as computed by MachineFrameInfo::computeMaxCallFrameSize.
Have you been able to figure out why the results of the calculations are different?

Yes, bcl5980 commented on the github issue:

In the AArch64TargetLowering::finalizeLowering it call MFI.computeMaxCallFrameSize(MF) to get MaxCallFrameSize
MaxCallFrameSize is 8 because a ADJCALLSTACKDOWN/ADJCALLSTACKUP in bb.2
And in the pass unreachable-mbb-elimination, bb.2 will be removed
Then the assert will be triggered in the pass prologepilog
Not sure how to fix the assert

I was able to fix this assertion by appending to UnreachableMachineBlockElim::runOnMachineFunction

MachineFrameInfo &MFI = F.getFrameInfo();
MFI.computeMaxCallFrameSize(F);

but was advised to weaken this assertion by @MatzeB with his reasoning being that the assertion was mostly to catch cases where the value grew later, rather than shrank.

[barannikov88]

In D151276#4366847, @oskarwirga wrote:

In D151276#4366712, @barannikov88 wrote:

I'm not sure this is the correct fix. The assertion specifically checks that MaxCallFrameSize and AdjustsStack computed here are the same as computed by MachineFrameInfo::computeMaxCallFrameSize.
Have you been able to figure out why the results of the calculations are different?

Yes, bcl5980 commented on the github issue:

In the AArch64TargetLowering::finalizeLowering it call MFI.computeMaxCallFrameSize(MF) to get MaxCallFrameSize
MaxCallFrameSize is 8 because a ADJCALLSTACKDOWN/ADJCALLSTACKUP in bb.2
And in the pass unreachable-mbb-elimination, bb.2 will be removed
Then the assert will be triggered in the pass prologepilog
Not sure how to fix the assert

I was able to fix this assertion by appending to UnreachableMachineBlockElim::runOnMachineFunction

MachineFrameInfo &MFI = F.getFrameInfo();
MFI.computeMaxCallFrameSize(F);

but was advised to weaken this assertion by @MatzeB with his reasoning being that the assertion was mostly to catch cases where the value grew later, rather than shrank.

In this case AdjustsStack on line 352 should be initialized to false and MFI.adjustsStack() == AdjustsStack should also be removed making this assertion kind of useless.
If @MatzeB is happy with this, I'm too.

[oskarwirga]

Fix the test to be more normal

[MatzeB]

In this case AdjustsStack on line 352 should be initialized to false and MFI.adjustsStack() == AdjustsStack should also be removed making this assertion kind of useless.
If @MatzeB is happy with this, I'm too.

Maybe we can handle this in a similar manner. AdjustsStack==true should be the more conservative answer and we should be fine going from an initial true to false after optimizations. So we can weaken this check as well to only assert that we are not going from false to true...

[MatzeB]

llvm/test/CodeGen/AArch64/compute-call-frame-size-unreachable-pass.ll

2

A simple -mtriple aarch64-- may work too.

11–27

can you simplify (in the sense of less lines in the .ll file) this test more by dropping the !type annotations and the data for them, the #0 attribute function attributes, internal and fastcc, the !nosanitize metadata?

[oskarwirga]

Make sure we don't go from false to true.

[barannikov88]

llvm/lib/CodeGen/PrologEpilogInserter.cpp

352

This should be initialized to false.
There is no difference in practice, but it will be clearer that the flag is recalculated.

[oskarwirga]

Make the target generic aarch64

[oskarwirga]

llvm/lib/CodeGen/PrologEpilogInserter.cpp

352

Interestingly setting this to false causes CodeGen/X86/asan-check-memaccess-add.ll to crash :D

[oskarwirga]

Is there any reason why this would be emitting less instructions for just asan-check-memaccess-add.ll?

[barannikov88]

llvm/lib/CodeGen/PrologEpilogInserter.cpp

352

Probably it changed from true to false, which could never happen before.
I didn't expect a change in behavior, so feel free to revert this part

[oskarwirga]

Lets go back to what makes the least changes

[MatzeB]

Let's see how this goes. LGTM

llvm/lib/CodeGen/PrologEpilogInserter.cpp

 /// variables for the function's frame information and eliminate call frame
 /// pseudo instructions.
 void PEI::calculateCallFrameInfo(MachineFunction &MF) {
   const TargetInstrInfo &TII = *MF.getSubtarget().getInstrInfo();
   const TargetFrameLowering *TFI = MF.getSubtarget().getFrameLowering();
   MachineFrameInfo &MFI = MF.getFrameInfo();
 
   unsigned MaxCallFrameSize = 0;
   bool AdjustsStack = MFI.adjustsStack();
 
   // Get the function call frame set-up and tear-down instruction opcode
   unsigned FrameSetupOpcode = TII.getCallFrameSetupOpcode();
   unsigned FrameDestroyOpcode = TII.getCallFrameDestroyOpcode();
 
   // Early exit for targets which have no call frame setup/destroy pseudo
   // instructions.
   if (FrameSetupOpcode == ~0u && FrameDestroyOpcode == ~0u)
for (MachineBasicBlock::iterator I = BB.begin(); I != BB.end(); ++I)
       } else if (I->isInlineAsm()) {
         // Some inline asm's need a stack frame, as indicated by operand 1.
         unsigned ExtraInfo = I->getOperand(InlineAsm::MIOp_ExtraInfo).getImm();
         if (ExtraInfo & InlineAsm::Extra_IsAlignStack)
           AdjustsStack = true;
       }
 
   assert(!MFI.isMaxCallFrameSizeComputed() ||
-         (MFI.getMaxCallFrameSize() == MaxCallFrameSize &&
-          MFI.adjustsStack() == AdjustsStack));
+         (MFI.getMaxCallFrameSize() >= MaxCallFrameSize &&
+          !(AdjustsStack && !MFI.adjustsStack())));
   MFI.setAdjustsStack(AdjustsStack);
   MFI.setMaxCallFrameSize(MaxCallFrameSize);
 
   for (MachineBasicBlock::iterator I : FrameSDOps) {
     // If call frames are not being included as part of the stack frame, and
     // the target doesn't indicate otherwise, remove the call frame pseudos
     // here. The sub/add sp instruction pairs are still inserted, but we don't
     // need to track the SP adjustment for frame index elimination.

llvm/test/CodeGen/AArch64/compute-call-frame-size-unreachable-pass.ll

+; RUN: llc < %s -mtriple aarch64--
+
+; This tests that the MFI assert in unreachableblockelim pass
+; does not trigger
+
+%struct.ngtcp2_crypto_aead = type { i8*, i64 }
+%struct.ngtcp2_crypto_aead_ctx = type { i8* }
+
+; Function Attrs: noinline optnone
+define internal fastcc void @decrypt_pkt() unnamed_addr #0 !type !0 !type !1 {
+entry:
+  br i1 false, label %cont, label %trap, !nosanitize !2
+
+trap:                                             ; preds = %entry
+  unreachable, !nosanitize !2
+
+cont:                                             ; preds = %entry
+  %call = call i32 undef(i8* undef, %struct.ngtcp2_crypto_aead* undef, %struct.ngtcp2_crypto_aead_ctx* undef, i8* undef, i64 undef, i8* undef, i64 undef, i8* undef, i64 undef)
+  ret void
+}
+
+attributes #0 = { noinline optnone }
+
+!0 = !{i64 0, !"_ZTSFlPhPK18ngtcp2_crypto_aeadPKhmS4_mlP16ngtcp2_crypto_kmPFiS_S2_PK22ngtcp2_crypto_aead_ctxS4_mS4_mS4_mEE"}
+!1 = !{i64 0, !"_ZTSFlPvPKvS1_mS1_mlS_S_E.generalized"}
+!2 = !{}