Skip to content

[BOLT] Detect Linux kernel version if the binary is a Linux kernel #119088

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

Merged
merged 8 commits into from
Dec 26, 2024

Conversation

FLZ101
Copy link
Contributor

@FLZ101 FLZ101 commented Dec 7, 2024

This makes it easier to handle differences (e.g. of exception table
entry size) between versions of Linux kernel

@llvmbot
Copy link
Member

llvmbot commented Dec 7, 2024

@llvm/pr-subscribers-bolt

Author: Franklin (FLZ101)

Changes

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

3 Files Affected:

  • (modified) bolt/include/bolt/Core/BinaryContext.h (+3)
  • (modified) bolt/lib/Rewrite/RewriteInstance.cpp (+22)
  • (added) bolt/test/X86/linux-version.s (+30)
diff --git a/bolt/include/bolt/Core/BinaryContext.h b/bolt/include/bolt/Core/BinaryContext.h
index 115e59ca0697e5..052a145efe4a4b 100644
--- a/bolt/include/bolt/Core/BinaryContext.h
+++ b/bolt/include/bolt/Core/BinaryContext.h
@@ -670,6 +670,9 @@ class BinaryContext {
   /// Indicates if the binary is Linux kernel.
   bool IsLinuxKernel{false};
 
+  /// Linux kernel version (major, minor, reversion)
+  std::tuple<unsigned, unsigned, unsigned> LinuxKernelVersion;
+
   /// Indicates if relocations are available for usage.
   bool HasRelocations{false};
 
diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp
index 76e1f0156f828d..b88da11701b354 100644
--- a/bolt/lib/Rewrite/RewriteInstance.cpp
+++ b/bolt/lib/Rewrite/RewriteInstance.cpp
@@ -61,6 +61,7 @@
 #include <fstream>
 #include <memory>
 #include <optional>
+#include <regex>
 #include <system_error>
 
 #undef  DEBUG_TYPE
@@ -1030,6 +1031,27 @@ void RewriteInstance::discoverFileObjects() {
       continue;
     }
 
+    if (BC->IsLinuxKernel && SymName == "linux_banner") {
+      const StringRef SectionContents =
+          cantFail(Section->getContents(), "can not get section contents");
+      const std::string S =
+          SectionContents
+              .substr(SymbolAddress - Section->getAddress(), SymbolSize)
+              .str();
+
+      const std::regex Re(R"---(Linux version ((\d+)\.(\d+)(\.(\d+))?))---");
+      std::smatch Match;
+      if (std::regex_search(S, Match, Re)) {
+        unsigned Major = std::stoi(Match[2].str());
+        unsigned Minor = std::stoi(Match[3].str());
+        unsigned Rev = Match.size() > 5 ? std::stoi(Match[5].str()) : 0;
+        BC->LinuxKernelVersion = std::make_tuple(Major, Minor, Rev);
+        BC->outs() << "BOLT-INFO: Linux kernel version is " << Match[1].str();
+      } else {
+        BC->errs() << "BOLT-WARNING: Linux kernel version is unknown\n";
+      }
+    }
+
     if (!Section->isText()) {
       assert(SymbolType != SymbolRef::ST_Function &&
              "unexpected function inside non-code section");
diff --git a/bolt/test/X86/linux-version.s b/bolt/test/X86/linux-version.s
new file mode 100644
index 00000000000000..079910be931cdb
--- /dev/null
+++ b/bolt/test/X86/linux-version.s
@@ -0,0 +1,30 @@
+# REQUIRES: system-linux
+
+## Check that BOLT correctly detects the Linux kernel version
+
+# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown %s -o %t.o
+# RUN: %clang %cflags -nostdlib %t.o -o %t.exe \
+# RUN:   -Wl,--image-base=0xffffffff80000000,--no-dynamic-linker,--no-eh-frame-hdr
+
+# RUN: llvm-bolt %t.exe -o %t.out | FileCheck %s
+
+# CHECK: BOLT-INFO: Linux kernel version is 6.6.61
+
+	.text
+	.globl	f
+	.type	f, @function
+f:
+	ret
+	.size	f, .-f
+
+	.globl	linux_banner
+	.section	.rodata
+	.align 16
+	.type	linux_banner, @object
+	.size	linux_banner, 22
+linux_banner:
+	.string	"Linux version 6.6.61\n"
+
+## Fake Linux Kernel sections.
+  .section __ksymtab,"a",@progbits
+  .section __ksymtab_gpl,"a",@progbits

@FLZ101 FLZ101 force-pushed the feature-bolt-linux-kernel-version branch 2 times, most recently from 4e99abe to 782c464 Compare December 7, 2024 19:16
This makes it easier to handle differences (e.g. of exception table
entry size) between versions of Linux kernel
@FLZ101 FLZ101 force-pushed the feature-bolt-linux-kernel-version branch from 782c464 to 69554a8 Compare December 8, 2024 04:17
Copy link
Member

@dcci dcci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind the direction in general, but if you can add a subsequent patch showing how you want to use the functionality, that would be great.

@@ -670,6 +670,9 @@ class BinaryContext {
/// Indicates if the binary is Linux kernel.
bool IsLinuxKernel{false};

/// Linux kernel version (major, minor, reversion)
std::tuple<unsigned, unsigned, unsigned> LinuxKernelVersion;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This maybe should be a struct instead of a tuple

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -1205,6 +1225,9 @@ void RewriteInstance::discoverFileObjects() {
PreviousFunction = BF;
}

if (BC->IsLinuxKernel && !std::get<0>(BC->LinuxKernelVersion))
BC->errs() << "BOLT-WARNING: Linux kernel version is unknown\n";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the version of the kernel is unknown, you just emit a warning instead of erroring out?

Copy link
Contributor Author

@FLZ101 FLZ101 Dec 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If BOLT errors out here, then all Linux kernel related tests need to specify a proper version.

It would be better to do that later when functions like readAltInstructions() and readStaticKeysJumpTable() have been modified to utilize LinuxKernelVersion to handle differences between Linux kernel versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a Linux kernel version for each related test and changed the behavior to erroring out.

Use struct instead of tuple to represent Linux kernel version
@FLZ101 FLZ101 force-pushed the feature-bolt-linux-kernel-version branch from 7dc58da to 6b931b3 Compare December 8, 2024 17:04
Copy link
Contributor

@maksfb maksfb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR.

@@ -670,6 +698,8 @@ class BinaryContext {
/// Indicates if the binary is Linux kernel.
bool IsLinuxKernel{false};

LKVersion LinuxKernelVersion;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't expect to use the kernel version outside of the LinuxKernelRewriter, I suggest we move it in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -1030,6 +1031,25 @@ void RewriteInstance::discoverFileObjects() {
continue;
}

if (BC->IsLinuxKernel && SymName == "linux_banner") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reading part also should be in the LinuxKernelRewriter. You can access symbols via BinaryData objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 1045 to 1047
unsigned Major = std::stoi(Match[2].str());
unsigned Minor = std::stoi(Match[3].str());
unsigned Rev = Match.size() > 5 ? std::stoi(Match[5].str()) : 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: constify.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@FLZ101 FLZ101 force-pushed the feature-bolt-linux-kernel-version branch 3 times, most recently from 816d1aa to bb683eb Compare December 9, 2024 18:11
Move LinuxKernelVersion to LinuxKernelRewriter
@FLZ101
Copy link
Contributor Author

FLZ101 commented Dec 9, 2024

Multiple symbols with the same address but different sizes are mapped to the same BinaryData, which has a size of the first symbol.

So we usually can not be certain about the size of a BinaryData we get from a symbol, since there might be a symbol with the same address but different size registered first. I think it is not good.

Is it by design?

@maksfb
Copy link
Contributor

maksfb commented Dec 9, 2024

Multiple symbols with the same address but different sizes are mapped to the same BinaryData, which has a size of the first symbol.

So we usually can not be certain about the size of a BinaryData we get from a symbol, since there might be a symbol with the same address but different size registered first. I think it is not good.

Is it by design?

BinaryData concept was introduced for the purpose of data reordering optimizations some time ago. We didn't pursue the optimization any further (for a number of reasons), but the framework remains. Regarding the sizes, I vaguely recall there was a support for embedded objects that start at the same address and all data objects could be accessed. But I could be wrong. In any case, we are open to changing the BinaryData handling.

@maksfb
Copy link
Contributor

maksfb commented Dec 11, 2024

Thank you for refactoring the code. Please add tests for the Linux version and include non-standard versions.

@FLZ101
Copy link
Contributor Author

FLZ101 commented Dec 11, 2024

Thank you for refactoring the code. Please add tests for the Linux version and include non-standard versions.

There is a failed test case, which I have created a PR #119557 to fix.

I'll add missing tests soon.

@FLZ101
Copy link
Contributor Author

FLZ101 commented Dec 16, 2024

Thank you for refactoring the code. Please add tests for the Linux version and include non-standard versions.

Tests are added.

Copy link
Contributor

@maksfb maksfb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks! Please address the nit before committing.

linux_banner:

#ifdef A
.string "Linux version 6.6.61\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could you please place CHECKs next to the object you are checking against?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

maksfb added a commit to maksfb/llvm-project that referenced this pull request Dec 18, 2024
maksfb added a commit that referenced this pull request Dec 19, 2024
@FLZ101
Copy link
Contributor Author

FLZ101 commented Dec 23, 2024

I don't mind the direction in general, but if you can add a subsequent patch showing how you want to use the functionality, that would be great.

@dcci An example usage of LinuxKernelVersion is included in #120929.

It will be used more in [BOLT][AArch64] Basic support for Linux kernel.

@FLZ101 FLZ101 requested a review from dcci December 24, 2024 01:49
Copy link
Member

@dcci dcci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@FLZ101
Copy link
Contributor Author

FLZ101 commented Dec 25, 2024

@maksfb @dcci Can you merge this PR? I have no write access to this repository.

@FLZ101 FLZ101 requested a review from yota9 as a code owner December 26, 2024 01:14
@FLZ101 FLZ101 requested review from maksfb and dcci December 26, 2024 11:07
@dcci dcci merged commit 6e8a1a4 into llvm:main Dec 26, 2024
7 checks passed
github-actions bot pushed a commit to arm/arm-toolchain that referenced this pull request Jan 10, 2025
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.

4 participants