Page MenuHomePhabricator

[llvm-nm/objdump/size] Add tests for dumping symbol tables with invalid sh_size.
ClosedPublic

Authored by Higuoxing on Apr 10 2020, 3:44 AM.

Details

Summary

This change adds tests for llvm-nm, llvm-objdump and llvm-size when dumping symbol tables with invalid sh_size (sh_size % sizeof(Elf_Sym) != 0).

Diff Detail

Event Timeline

Higuoxing created this revision.Apr 10 2020, 3:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2020, 3:44 AM
Higuoxing retitled this revision from [llvm/tools] Enable error reporting in clients when getSymbolFlags() fails. to [llvm-nm/objdump/size] Enable error reporting in clients when getSymbolFlags() fails..Apr 10 2020, 5:24 AM
Higuoxing edited the summary of this revision. (Show Details)
jhenderson added inline comments.Apr 17 2020, 7:24 AM
llvm/test/tools/llvm-nm/invalid-symbol-table-size.test
2

will emit -> emits

(same in other tests)

llvm/test/tools/llvm-size/invalid-symbol-table-size.test
6

I think you need testing for both the sysv and berkeley output formats, as you've handled the error in two different places.

grimar added inline comments.Apr 20 2020, 3:20 AM
llvm/test/tools/llvm-nm/invalid-symbol-table-size.test
13

In this and other test cases you have 2 sections: .symtab and .dynsym, but only one section (with index 2)
is reported in error messages.

Higuoxing marked an inline comment as done.Apr 20 2020, 4:39 AM
Higuoxing added inline comments.
llvm/test/tools/llvm-nm/invalid-symbol-table-size.test
13

Yes, because there are malformed .symtab and .dynsym symbol tables in one ELF file. And getSymbolFlags() checks .symtab first. See ELFObjectFile.h.

if (Expected<typename ELFT::SymRange> SymbolsOrErr =
        EF.symbols(DotSymtabSec)) {
  ...
} else
  // TODO: Test this error.
  return SymbolsOrErr.takeError();

if (Expected<typename ELFT::SymRange> SymbolsOrErr =
        EF.symbols(DotDynSymSec)) {
  ...
} else
  // TODO: Test this error.
  return SymbolsOrErr.takeError();

I think it's a good point, and it's helpful to report errors on our interested sections.


Currently, I have two approaches in mind.

  1. We could add one more argument IsDynamic to getSymbolFlags(DataRefImpl Symb, bool IsDynamic=false). So that we can skip first checker if we've already known the symbol is a dynamic symbol. (Possible)
  1. Change the return type of symbol iterator API from symbol_iterator to Expected<symbol_iterator>, so that we could get error reports earlier. (I didn't give it a try, and it seems involving many changes.)
grimar added inline comments.Apr 20 2020, 4:52 AM
llvm/test/tools/llvm-nm/invalid-symbol-table-size.test
13

Why not to change the test so that you have a one broken test at once?

Case 1: Broken .symtab. Valid .dynsym.
Case 2: Valid .symtab. Broken .dynsym.

13
  • a one broken section
Higuoxing marked an inline comment as done.Apr 20 2020, 5:05 AM
Higuoxing added inline comments.
llvm/test/tools/llvm-nm/invalid-symbol-table-size.test
13

I think change test cases partly resolve this problem. I mean, if we one broken .symtab, and a good .dynsym section, consider the following situation.

# broken .symtab prevents us dumping .dynsym
$ llvm-nm --dynamic broken-symtab.elf
$ llvm-objdump --dynamic-syms broken-symtab.elf
grimar added inline comments.Apr 20 2020, 5:22 AM
llvm/test/tools/llvm-nm/invalid-symbol-table-size.test
13

You can have

Case 3: Both .symtab and .dynsym are broken. The same behavior as in Case 1.

Higuoxing marked an inline comment as done.Apr 20 2020, 6:03 AM
Higuoxing added inline comments.
llvm/test/tools/llvm-nm/invalid-symbol-table-size.test
13

Ok, thanks a lot.

Higuoxing updated this revision to Diff 261693.Sun, May 3, 6:04 AM

Addressed comments.

  • Test .symtab and .dynsym separately.
  • Add tests in llvm-size for different output format (sysv and berkeley).
Higuoxing retitled this revision from [llvm-nm/objdump/size] Enable error reporting in clients when getSymbolFlags() fails. to [llvm-nm/objdump/size] Add tests for dumping symbol tables with invalid sh_size..Sun, May 3, 6:08 AM
Higuoxing edited the summary of this revision. (Show Details)
jhenderson accepted this revision.Tue, May 5, 12:38 AM

LGTM! @grimar is away this week, I believe, so I don't think you need to wait for him.

This revision is now accepted and ready to land.Tue, May 5, 12:38 AM
MaskRay accepted this revision.Tue, May 5, 10:15 AM
This revision was automatically updated to reflect the committed changes.