Skip to content

[BOLT] Fix Linux kernel static keys handling #119557

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

FLZ101
Copy link
Contributor

@FLZ101 FLZ101 commented Dec 11, 2024

Fix the following issues:

  • Cursor.tell() + (int32_t)DE.getU32(Cursor) is undefined

  • In bolt/test/X86/linux-static-keys.s, __start___jump_table
    is unaligned:

    ffffffff800001fd g     O .rodata        0000000000000000 __start___jump_table
    

    This makes the test case undeterministic.

@llvmbot
Copy link
Member

llvmbot commented Dec 11, 2024

@llvm/pr-subscribers-bolt

Author: Franklin (FLZ101)

Changes

Fix the following issues:

  • Cursor.tell() + (int32_t)DE.getU32(Cursor) is undefined

  • bolt/test/X86/linux-static-keys.s generates a binary whose .rodata is unaligned:

    3 .rodata       00000020  ffffffff800001fd  ffffffff800001fd  000001fd  2**0
                    CONTENTS, ALLOC, LOAD, READONLY, DATA
    

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

2 Files Affected:

  • (modified) bolt/lib/Rewrite/LinuxKernelRewriter.cpp (+4-3)
  • (modified) bolt/test/X86/linux-static-keys.s (+3-2)
diff --git a/bolt/lib/Rewrite/LinuxKernelRewriter.cpp b/bolt/lib/Rewrite/LinuxKernelRewriter.cpp
index 03b414b71caca7..7409c33ec84e18 100644
--- a/bolt/lib/Rewrite/LinuxKernelRewriter.cpp
+++ b/bolt/lib/Rewrite/LinuxKernelRewriter.cpp
@@ -1660,12 +1660,13 @@ Error LinuxKernelRewriter::readStaticKeysJumpTable() {
   DataExtractor::Cursor Cursor(StaticKeysJumpTableAddress - SectionAddress);
   uint32_t EntryID = 0;
   while (Cursor && Cursor.tell() < Stop->getAddress() - SectionAddress) {
+    uint64_t _;
     const uint64_t JumpAddress =
-        SectionAddress + Cursor.tell() + (int32_t)DE.getU32(Cursor);
+        (_ = SectionAddress + Cursor.tell(), _ + (int32_t)DE.getU32(Cursor));
     const uint64_t TargetAddress =
-        SectionAddress + Cursor.tell() + (int32_t)DE.getU32(Cursor);
+        (_ = SectionAddress + Cursor.tell(), _ + (int32_t)DE.getU32(Cursor));
     const uint64_t KeyAddress =
-        SectionAddress + Cursor.tell() + (int64_t)DE.getU64(Cursor);
+        (_ = SectionAddress + Cursor.tell(), _ + (int64_t)DE.getU64(Cursor));
 
     // Consume the status of the cursor.
     if (!Cursor)
diff --git a/bolt/test/X86/linux-static-keys.s b/bolt/test/X86/linux-static-keys.s
index 0bd17a375d8824..465e865458d902 100644
--- a/bolt/test/X86/linux-static-keys.s
+++ b/bolt/test/X86/linux-static-keys.s
@@ -35,13 +35,13 @@ _start:
 .L0:
   jmp L1
 # CHECK:      jit
-# CHECK-SAME: # ID: 1 {{.*}} # Likely: 0 # InitValue: 1
+# CHECK-SAME: # ID: 1 {{.*}} # Likely: 1 # InitValue: 0
   nop
 L1:
   .nops 5
   jmp .L0
 # CHECK:      jit
-# CHECK-SAME: # ID: 2 {{.*}} # Likely: 1 # InitValue: 1
+# CHECK-SAME: # ID: 2 {{.*}} # Likely: 0 # InitValue: 0
 
 ## Check that a branch profile associated with a NOP is handled properly when
 ## dynamic branch is created.
@@ -65,6 +65,7 @@ foo:
   .rodata
   .globl __start___jump_table
   .type __start___jump_table, %object
+  .align 8
 __start___jump_table:
 
   .long .L0 - . # Jump address

Fix the following issues:

* `Cursor.tell() + (int32_t)DE.getU32(Cursor)` is undefined

* In `bolt/test/X86/linux-static-keys.s` __start___jump_table
  is unaligned:

  ```
  ffffffff800001fd g     O .rodata        0000000000000000 __start___jump_table
  ```

  This makes the test case undeterministic.
@FLZ101 FLZ101 force-pushed the fix-bolt-linux-static-keys branch from adbd377 to a89b838 Compare December 11, 2024 14:19
@maksfb
Copy link
Contributor

maksfb commented Dec 12, 2024

Thank you for identifying the problem, actually two of them. I don't think we are currently affected by the order of evaluation, but it's the proper fix.

The other one with the test case is caused by the key address field, alignment just exposes it. I'v put up #119771 with the fix for the field.

@FLZ101
Copy link
Contributor Author

FLZ101 commented Dec 13, 2024

Since the test case is fixed by #119771. Close this PR for now.

@FLZ101 FLZ101 closed this Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants