This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objdump] Add symbol 'O' for object data
ClosedPublic

Authored by Higuoxing on Nov 10 2018, 8:57 AM.

Details

Summary

When dumping elf object's symbol table info, gnu-objdump will give:

SYMBOL TABLE:
0000000000001004 l     F .text	0000000000000000 lfoo
0000000000001008 l     O .text	0000000000000000 lbar
0000000000001004 g     F .text	0000000000000000 foo
0000000000001008 g     O .text	0000000000000000 bar

llvm-objdump will give:

0000000000001004 l     F .text		 00000000 lfoo
0000000000001008 l       .text		 00000000 lbar
0000000000001004 g     F .text		 00000000 foo
0000000000001008 g       .text		 00000000 bar

To pass llvm-objdump tests, I have to change some tests for Mach-O and Wasm which I am not so familiar with ... Is this ok to apply these changes? Because gnu-objdump display different info on Mach-O, compared with llvm-objdump

Diff Detail

Event Timeline

Higuoxing created this revision.Nov 10 2018, 8:57 AM
MaskRay accepted this revision.Nov 10 2018, 1:05 PM
MaskRay added a subscriber: MaskRay.
MaskRay added inline comments.
test/tools/llvm-objdump/common-symbol-elf.test
3

GNU objdump does not print the g flag because in binutils-gdb/bfd/syms.c:bfd_print_symbol_vandf, a common symbol is neither BSF_GLOBAL nor BSF_LOCAL (though a common symbol is usually STB_GLOBAL)... I consider that a quirk that we do not necessarily replicate the behavior. Also see some discussion in https://reviews.llvm.org/D53782

This revision is now accepted and ready to land.Nov 10 2018, 1:05 PM
Higuoxing added inline comments.Nov 10 2018, 8:51 PM
test/tools/llvm-objdump/common-symbol-elf.test
3

Thanks a lot : )

kristina accepted this revision.Nov 11 2018, 6:26 AM

LGTM with minor nitpick, wouldn't using CHECK-NEXT in the Mach-O test make more sense like in the other tests?

Also, do you need it committed?

LGTM with minor nitpick, wouldn't using CHECK-NEXT in the Mach-O test make more sense like in the other tests?

Thanks a lot, I will update it later.

Also, do you need it committed?

Yes, please

Higuoxing updated this revision to Diff 173557.Nov 11 2018, 7:41 AM

Update test cases

LGTM with minor nitpick, wouldn't using CHECK-NEXT in the Mach-O test make more sense like in the other tests?

Thanks a lot, I will update it later.

Also, do you need it committed?

Yes, please

On it, moment.

This revision was automatically updated to reflect the committed changes.
kristina reopened this revision.Nov 11 2018, 10:12 AM

This is causing some LLVM and LLD tests to fail on buildbots, since they rely on the old format.

I'll try to correct them myself, the culprits are:

Failing Tests (3):
    LLVM :: MC/ELF/common-redeclare.s
    LLVM :: Object/objdump-symbol-table.test
    lld :: ELF/relocation-common.s
This revision is now accepted and ready to land.Nov 11 2018, 10:12 AM

rL346611 for LLVM testsuite, and rLLD346612 for LLD testsuite should fix tests affected by this. Will close when/if llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast is happy.

rLLD346613 fixes MIPS LLD tests. Looking at build console, there is a bunch of other target specific tests that need updates after this.

kristina added a comment.EditedNov 11 2018, 11:03 AM

About to fail again:

  • Hexagon: llvm/test/MC/Hexagon/lcomm.s, MC/Hexagon/common-redeclare.s
  • Wasm: llvm/test/MC/WebAssembly/weak-alias.ll

Updating those two and that should be the last of it.

Last fixes in rL346614 and rL346617. Big sorry for the buildbot storm, I should have reverted it after the first set of failures and dissected it after, or better yet grepped for objdump usage by other targets when reviewing.

kristina closed this revision.Nov 11 2018, 11:46 AM

Grouped and linked all related commits. Closing.

Grouped and linked all related commits. Closing.

Sorry for that, I should have run 'test all' locally ... Sorry for bothering you too much! Thanks a lot!

Grouped and linked all related commits. Closing.

Sorry for that, I should have run 'test all' locally ... Sorry for bothering you too much! Thanks a lot!

Don't worry, it's not your fault, there were a lot of targets that used llvm-objdump in their tests
that are unlikely to be built by most locally (ie. Hexagon). I guess at the core a lesson for the future
is to grep around for uses of said tool in tests since without the target enabled, you will not be
eligible to run the test and while the majority of the tests work on X86 (which is usually enabled by
default for almost everyone), target specific tests often require said target backend being enabled.