Skip to content

Conversation

cabbaken
Copy link
Contributor

@cabbaken cabbaken commented Feb 20, 2025

The dynamic string table used by the dynamic section is referenced by the sh_link field of that section, so we should use that directly, rather than going via the dynamic symbol table.
More info: #125679 (comment)

@llvmbot
Copy link
Member

llvmbot commented Feb 20, 2025

@llvm/pr-subscribers-llvm-binary-utilities

Author: Ruoyu Qiu (cabbaken)

Changes

More info: #125679 (comment)


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

2 Files Affected:

  • (modified) llvm/test/tools/llvm-objdump/ELF/private-headers.test (+1)
  • (modified) llvm/tools/llvm-objdump/ELFDump.cpp (+2-2)
diff --git a/llvm/test/tools/llvm-objdump/ELF/private-headers.test b/llvm/test/tools/llvm-objdump/ELF/private-headers.test
index eefdc8440385c..c90cf59f4ed7d 100644
--- a/llvm/test/tools/llvm-objdump/ELF/private-headers.test
+++ b/llvm/test/tools/llvm-objdump/ELF/private-headers.test
@@ -30,6 +30,7 @@ Sections:
   - Name:    .dynamic
     Type:    SHT_DYNAMIC
     Flags:   [ SHF_ALLOC ]
+    Link:    1
     Entries:
      - Tag:   DT_NEEDED
        Value: 0x1
diff --git a/llvm/tools/llvm-objdump/ELFDump.cpp b/llvm/tools/llvm-objdump/ELFDump.cpp
index e9e5b059f1786..bce308c870ddf 100644
--- a/llvm/tools/llvm-objdump/ELFDump.cpp
+++ b/llvm/tools/llvm-objdump/ELFDump.cpp
@@ -78,8 +78,8 @@ static Expected<StringRef> getDynamicStrTab(const ELFFile<ELFT> &Elf) {
     return SectionsOrError.takeError();
 
   for (const typename ELFT::Shdr &Sec : *SectionsOrError) {
-    if (Sec.sh_type == ELF::SHT_DYNSYM)
-      return Elf.getStringTableForSymtab(Sec);
+    if (Sec.sh_type == ELF::SHT_DYNAMIC)
+      return Elf.getLinkAsStrtab(Sec);
   }
 
   return createError("dynamic string table not found");

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

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

LGTM too. Do you need assistance with merging it?

I'd be tempted to update the PR description to avoid people needing to click links to get the context. Something along the lines of "the dynamic string table used by the dynamic section is referenced by the sh_link field of that section, so we should use that directly, rather than going via the dynamic symbol table." Also, I'd add [llvm-objdump] to the PR title.

@cabbaken
Copy link
Contributor Author

LGTM too. Do you need assistance with merging it?

I'd be tempted to update the PR description to avoid people needing to click links to get the context. Something along the lines of "the dynamic string table used by the dynamic section is referenced by the sh_link field of that section, so we should use that directly, rather than going via the dynamic symbol table." Also, I'd add [llvm-objdump] to the PR title.

Yes.
ok, I will modify PR title and description.

@cabbaken cabbaken changed the title Correct .dynstr finding of getDynamicStrTab() [llvm-objdump]Correct .dynstr finding of getDynamicStrTab() Feb 24, 2025
@jh7370 jh7370 merged commit 5a2bee0 into llvm:main Feb 24, 2025
10 checks passed
@jh7370
Copy link
Collaborator

jh7370 commented Feb 24, 2025

Okay, merged. Please keep an eye out for any build bot failures (I wouldn't expect any related to the change, but that's not a guarantee).

@cabbaken cabbaken deleted the fixgetDynamicStrTab branch March 5, 2025 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants