Skip to content
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

[BOLT] Fix build-time assertion in RewriteInstance #90540

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

maksfb
Copy link
Contributor

@maksfb maksfb commented Apr 30, 2024

We use pwrite() in RewriteInstance to update contents of existing sections. pwrite() requires file position to be set past the written offset which we guarantee at the start of rewriteFile(). Then we had an implicit assumption in patchBuildID() that the file position will be set again in patchELFSymTabs() after being reset in patchELFPHDRTable(). That assumption was broken in #90300. The fix is to save and restore file position in patchELFPHDRTable(). Then we don't have to update it again in patchELFSymTabs().

We use pwrite() in RewriteInstance to update contents of existing
sections. pwrite() requires file position to be set past the written
offset which we guarantee at the start of rewriteFile(). Then we had an
implicit assumption in patchBuildID() that the file position will be set
again in patchELFSymTabs() after being reset in patchELFPHDRTable().
That assumption was broken in llvm#90300. The fix is to save and restore
file position in patchELFPHDRTable(). Then we don't have to update it
again in patchELFSymTabs().
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 30, 2024

@llvm/pr-subscribers-bolt

Author: Maksim Panchenko (maksfb)

Changes

We use pwrite() in RewriteInstance to update contents of existing sections. pwrite() requires file position to be set past the written offset which we guarantee at the start of rewriteFile(). Then we had an implicit assumption in patchBuildID() that the file position will be set again in patchELFSymTabs() after being reset in patchELFPHDRTable(). That assumption was broken in #90300. The fix is to save and restore file position in patchELFPHDRTable(). Then we don't have to update it again in patchELFSymTabs().


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

1 Files Affected:

  • (modified) bolt/lib/Rewrite/RewriteInstance.cpp (+3-4)
diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp
index 644d87eeca42e6..23f79e3c135a78 100644
--- a/bolt/lib/Rewrite/RewriteInstance.cpp
+++ b/bolt/lib/Rewrite/RewriteInstance.cpp
@@ -4047,6 +4047,7 @@ void RewriteInstance::patchELFPHDRTable() {
     NewWritableSegmentSize = NextAvailableAddress - NewWritableSegmentAddress;
   }
 
+  const uint64_t SavedPos = OS.tell();
   OS.seek(PHDRTableOffset);
 
   auto createNewTextPhdr = [&]() {
@@ -4151,6 +4152,8 @@ void RewriteInstance::patchELFPHDRTable() {
         << "BOLT-ERROR: could not find PT_GNU_STACK program header to modify\n";
     exit(1);
   }
+
+  OS.seek(SavedPos);
 }
 
 namespace {
@@ -5041,10 +5044,6 @@ void RewriteInstance::patchELFSymTabs(ELFObjectFile<ELFT> *File) {
   assert((DynSymSection || BC->IsStaticExecutable) &&
          "dynamic symbol table expected");
   if (DynSymSection) {
-    // Set pointer to the end of the section, so we can use pwrite to update
-    // the dynamic symbol table.
-    Out->os().seek(DynSymSection->sh_offset + DynSymSection->sh_size);
-
     updateELFSymbolTable(
         File,
         /*IsDynSym=*/true,

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.

Neat.

@maksfb maksfb merged commit 49bb993 into llvm:main Apr 30, 2024
6 checks passed
@maksfb maksfb deleted the gh-fix-seek branch April 30, 2024 21:33
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.

None yet

4 participants