[Object] fixed invalid symbol handling in ELFObjectFile::getSymbolName

[JestrTulip]

Summary

Found a bug in ElfObjectFile.h that occurred when there was an invalid Symbol Name in an object file. This error affected the behavior of the Expected<> value and leading it to abort, rather than behave as normal. I found this as I was adding tests to llvm-cm, as prompted by @jhenderson.

Without this fix, upon encountering an invalid symbol and trying to get its name, the program states that

Expected<T> must be checked before access or destruction

and aborts.

[mtrofin]

you probably want to change the commit title to something more specific - e.g. "fix invalid section handling in ELFObjectFile::getSymbolName" and capture in the description what happens - i.e. without the fix, the test in this patch segfaults (iirc)

llvm/test/tools/llvm-objdump/ELF/section-no-symbol-name.test

19 ↗

(On Diff #537918)

newline

[MaskRay]

llvm/test/tools/llvm-objdump/ELF/section-no-symbol-name.test

4 ↗

(On Diff #537918)

If there is a warning or error, we usually include warning: or error:. See other tests.

17 ↗

(On Diff #537918)

Add a comment ## Invalid section index

[JestrTulip]

feedback

[MaskRay]

llvm/test/tools/llvm-objdump/ELF/section-no-symbol-name.test

2 ↗

(On Diff #538010)

I think you want to add a llvm-objdump test.

This directory is not for the proposed llvm-cm.

[JestrTulip]

feedback

[JestrTulip]

llvm/test/tools/llvm-objdump/ELF/section-no-symbol-name.test

2 ↗

(On Diff #538010)

fixed!

[MaskRay]

To me, the filename section-no-symbol-name.test is not accurate as almost all STT_SECTION symbols don't have names.
Tools like llvm-nm/llvm-readelf use heuristics to give the section name (see D57105).

I am mainly thinking of https://maskray.me/blog/2021-08-08-toolchain-testing "I don't know an existing test can be enhanced".
I usually search for the diagnostics (invalid section index) to find related tests. In this case, it seems that we do want a llvm-objdump -d test as llvm-objdump -t reports an error instead of warning, stopping further processing.
With more investigation, I think we probably should port a llvm-readobj behavior. See D154754.

With D154754, I think you can enhance a test by adding -d beside --syms.

[JestrTulip]

removed redundant test

[JestrTulip]

updated section-symbols.test

[MaskRay]

I made a suggestion to the test. The title probably should use a tag like [Object]

llvm/test/tools/llvm-objdump/ELF/section-symbols.test

14

This check separates CHECK for another RUN line.
I suggest that we move llvm-objdump and # CHECK-DISAS after all CHECK.

## Test that we consume an error in ELFObjectFile<ELFT>::getSymbolName.
# RUN: llvm-objdump -d ...

# CHECK-DISAS: ...

[JestrTulip]

feedback

[chapuni]

llvm/test/tools/llvm-objdump/ELF/section-symbols.test

23

Disassembling doesn't work if x86 is not configured.

[michaelplatings]

llvm/test/tools/llvm-objdump/ELF/section-symbols.test

23

Yes, this test is failing in my build with -DLLVM_TARGETS_TO_BUILD="AArch64;ARM"

[luporl]

llvm/test/tools/llvm-objdump/ELF/section-symbols.test

23

This also broke some bots:

• https://lab.llvm.org/buildbot/#/builders/245/builds/10929
• https://lab.llvm.org/buildbot/#/builders/188/builds/32254
• https://lab.llvm.org/buildbot/#/builders/65/builds/10478

[mtrofin]

llvm/test/tools/llvm-objdump/ELF/section-symbols.test

23

@JestrTulip @MaskRay judging from line 32, the test should require x86 target? Or -r is expected to run anyway, and it's just -d that can't, and thus should be split off in its own X86/section-symbols-disassemble.test maybe (to give it a name)?

llvm/include/llvm/Object/ELFObjectFile.h

Expected<StringRef> ELFObjectFile<ELFT>::getSymbolName(DataRefImpl Sym) const {
   if (!SymStrTabOrErr)
     return SymStrTabOrErr.takeError();
   Expected<StringRef> Name = (*SymOrErr)->getName(*SymStrTabOrErr);
   if (Name && !Name->empty())
     return Name;
 
   // If the symbol name is empty use the section name.
   if ((*SymOrErr)->getType() == ELF::STT_SECTION) {
-    if (Expected<section_iterator> SecOrErr = getSymbolSection(Sym)) {
-      consumeError(Name.takeError());
+    Expected<section_iterator> SecOrErr = getSymbolSection(Sym);
+    if (SecOrErr)
       return (*SecOrErr)->getName();
-    }
+    return SecOrErr.takeError();
   }
   return Name;
 }
 
 template <class ELFT>
 uint64_t ELFObjectFile<ELFT>::getSectionFlags(DataRefImpl Sec) const {
   return getSection(Sec)->sh_flags;
 }

llvm/test/tools/llvm-objdump/ELF/section-symbols.test

 ## ELF section symbols use the corresponding section names when printing
 ## unnamed symbols. This test verifies this and also that appropriate things
 ## are printed if the section is somehow invalid.
 
 # RUN: yaml2obj %s -o %t1
 # RUN: llvm-objdump -r --syms %t1 2>&1 | \
 # RUN:   FileCheck %s -DFILE=%t1 --implicit-check-not=warning:
 
 # CHECK:      SYMBOL TABLE:
 # CHECK-NEXT: 00000000 l    d  .foo   00000000 .foo
 # CHECK-NEXT: 00000000 l    d  .foo   00000000 .foo
 # CHECK-NEXT: warning: '[[FILE]]': invalid section index: 67
 # CHECK-NEXT: warning: '[[FILE]]': invalid section index: 68
 
 # CHECK:      RELOCATION RECORDS FOR [.foo]:
 # CHECK-NEXT: OFFSET   TYPE                     VALUE
 # CHECK-NEXT: 00000001 R_X86_64_NONE            .foo
 # CHECK-NEXT: 00000002 R_X86_64_NONE            .foo
 # CHECK-NEXT: 00000003 R_X86_64_NONE            {{$}}
 # CHECK-NEXT: 00000004 R_X86_64_NONE            {{$}}
 
+## Test that we consume an error in ELFObjectFile<ELFT>::getSectionName when disassembling. 
+# RUN: llvm-objdump -d --syms %t1 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t1 --check-prefix=CHECK-DISAS
+# CHECK-DISAS: warning: '[[FILE]]': invalid section index: 67
+
 --- !ELF
 FileHeader:
   Class:   ELFCLASS32
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_X86_64
 Sections:
   - Name: .foo