[BOLT] Skip the validation of CFG after it is finalized

[Kepontry]

Summary

When current state is CFG_Finalized, function validateCFG() should return true directly.

[Kepontry]

In the validateSuccessorInvariants function, we validate the correspondence between the number of successors and the type of branching instruction. An error is triggered when an instruction is an unconditional branch and has no successors.

When there is a "b _ZdlPvm@PLT" TailCall instruction at the end of a basic block. This instruction jumps to the PLT table, and does not have any successors. Although this instruction is an unconditional jump, the function “isUnconditionalBranch” returns false because the inst is a TailCall and doesn’t need CFG validation.

However, when the “inst-lowering” Pass is applied, the lowerTailCall function is invoked, removing the “kTailCall” label from this instruction. Consequently, the isTailCall(Inst) function returns true, which then leads to an error during the CFG validation of this instruction.

[yota9]

Agree, it was bothering me a long time ago :) LGTM, let's wait for meta team opinion

[Kepontry]

Any suggestions from the Meta team?
:-)

[maksfb]

I agree that we shouldn't validate CFG after the instruction lowering pass. Thanks for the fix and for providing the explanation.

Implementation-wise, I think we should make the validation requirement even stronger. Once a CFG is finalized, i.e. a function is in CFG_Finalized state, we should skip the check. You can simply return true from validateCFG().

[maksfb]

@Kepontry, if you already have a test case, could you please add it?

[maksfb]

Thanks!

bolt/lib/Core/BinaryFunction.cpp

3160–3162

nit: drop curly braces.

bolt/lib/Core/BinaryFunction.cpp

if (opts::Verbosity >= 1) {
              << Filename << " for output.\n";
     }
     return;
   }
   dumpGraph(of);
 }
 
 bool BinaryFunction::validateCFG() const {
+  // Skip the validation of CFG after it is finalized
+  if (CurrentState == State::CFG_Finalized)
+    return true;
+
   bool Valid = true;
   for (BinaryBasicBlock *BB : BasicBlocks)
     Valid &= BB->validateSuccessorInvariants();
 
   if (!Valid)
     return Valid;
 
   // Make sure all blocks in CFG are valid.

bolt/test/verify-cfg.test

+# Verify if the `--verify-cfg` option might produce incorrect alerts.
+
+REQUIRES: system-linux
+
+RUN: %clang %cflags %p/Inputs/hello.c -o %t -Wl,-q
+RUN: llvm-bolt %t -o %t.bolt --verify-cfg 2>&1 | FileCheck %s
+
+CHECK-NOT: BOLT-ERROR: Invalid CFG detected after pass {{.*}}