[BOLT] Don't remove nops in non-simple functions

[yota9]

Follow the logic of https://reviews.llvm.org/D143887 patch (fixed later
by #71377) we don't want to remove nops in non-simple function just in
case there is undetected jump table to minimize chances to break offsets
in it.

[llvmbot]

@llvm/pr-subscribers-bolt

Author: Vladislav Khmelevsky (yota9)

Changes

Follow the logic of https://reviews.llvm.org/D143887 patch (fixed later
by #71377) we don't want to remove nops in non-simple function just in
case there is undetected jump table to minimize chances to break offsets
in it.

---

Full diff: https://github.com/llvm/llvm-project/pull/101964.diff

1 Files Affected:

• (modified) bolt/lib/Passes/BinaryPasses.cpp (+1-1)

diff --git a/bolt/lib/Passes/BinaryPasses.cpp b/bolt/lib/Passes/BinaryPasses.cpp
index fa95ad7324ac1..fbbbe1e05df2c 100644
--- a/bolt/lib/Passes/BinaryPasses.cpp
+++ b/bolt/lib/Passes/BinaryPasses.cpp
@@ -1923,7 +1923,7 @@ Error RemoveNops::runOnFunctions(BinaryContext &BC) {
   };
 
   ParallelUtilities::PredicateTy SkipFunc = [&](const BinaryFunction &BF) {
-    return BF.shouldPreserveNops();
+    return BF.shouldPreserveNops() || (!opts::StrictMode && !BF.isSimple());
   };
 
   ParallelUtilities::runOnEachFunction(

[peterwaller-arm]

Hold on, it looks like there are test failures.

/var/lib/buildkite-agent/builds/linux-56-59b8f5d88-24sjh-1/llvm-project/github-pull-requests/bolt/test/RISCV/reloc-label-diff.s:22:11: error: CHECK: expected string not found in input
// CHECK: 0x{{.*}} 04000000
          ^
<stdin>:2:29: note: scanning from here
Hex dump of section '.data':
                            ^
<stdin>:3:7: note: possible intended match here
0x0001219c 08000000 ....
      ^
Input file: <stdin>
Check file: /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-24sjh-1/llvm-project/github-pull-requests/bolt/test/RISCV/reloc-label-diff.s
-dump-input=help explains the following input dump.
Input was:
<<<<<<
            1:
            2: Hex dump of section '.data':
check:22'0                                 X error: no match found
            3: 0x0001219c 08000000 ....
check:22'0     ~~~~~~~~~~~~~~~~~~~~~~~~~
check:22'1           ?                   possible intended match
>>>>>>
--
********************

[yota9]

Yeah, thanks @peterwaller-arm . I would need to build bolt with riscv support one day :) I probably fixed it, would check it now. But anyway I'm not ready to submit without one of meta team members approve - I'm pretty sure some of their internal test would fail and it probably needs some kind of test which I don't currently have (the reason it is NFC)

[yota9]

Add test, so now it is not NFC patch. Looking forward to meta team comments about this patch and their internal tests passed

[aaupov]

This is non-NFC, and probably a regression, at least on X86.

we don't want to remove nops in non-simple function just in case there is undetected jump table to minimize chances to break offsets in it.

I think that should be covered by relocation mode. If there are issues with jump table identification, that should be fixed separately.

Let's hear from @maksfb @rafaelauler

[yota9]

@aaupov Could you please check that the regression is not caused by the tests (e.g. function size, instruction sequence & etc)?
Currently we have a problem that the nop could be removed before adrrelaxation pass in non-simple function. In order to address this we need to either change multiple passes order (so it should be function splitting -> adrrelaxation -> nop removal) or make this change, which I think is preferable.
The addrelaxation change D143887 addresses the issue that at least on ARM we saw that offsets were not fixed by bolt. It was not me who find this problem, AFAIK the relocations might be in a form of symbol + offset - jump table address (PREL ones), so BOLT would only change symbol value of function and and jump table addresses properly, but offset would be constant in this case, so removing the NOP instruction offset would result in changing offset value and potential problems.
My suggestion here is that we should not change pass order, but preserve the logic from the patch above, so non-simple functions would preserve their nops. I don't think it's a big deal anyway, but if you insist I can set PreserveNops in BF exclusively for aarch64 targets somewhere in the code :)

[maksfb]

Hi @yota9, I've tried the test case for AArch64 and it succeeds even without applying the fix. I have some other changes in my repo, by I doubt they are related. Could you please double check that the test case is valid?

[maksfb]

We currently set PreserveNops on aarch64 whenever we encounter an indirect branch. This happens in the relocation mode (the only officially supported mode for aarch64) regardless if the containing function will be later marked as non-simple or not. I don't have a background (or at least I cannot recollect) why this is the way.

I general, I agree that we should preserve an internal function layout for non-simple functions, i.e. maintain offsets of all potential branch destinations. This should be done for all platforms.