This is an archive of the discontinued LLVM Phabricator instance.

[obj2yaml] - Do not miss section index for special symbols.
ClosedPublic

Authored by grimar on Feb 21 2019, 3:30 AM.

Details

Summary

This fixes https://bugs.llvm.org/show_bug.cgi?id=40786
("obj2yaml symbol output missing section index for SHN_ABS and SHN_COMMON symbols")

Since SHN_ABS and SHN_COMMON symbols are special, we should preserve
the st_shndx for them. The patch does this for them and the other special symbols.

The test case is based on the test provided by James Henderson at the bug page!

Diff Detail

Event Timeline

grimar created this revision.Feb 21 2019, 3:30 AM
grimar edited the summary of this revision. (Show Details)Feb 21 2019, 3:31 AM
jhenderson added inline comments.Feb 21 2019, 3:51 AM
tools/obj2yaml/elf2yaml.cpp
279

I'm thinking that we should probably actually preserve all values >= SHN_LORESERVE, since these aren't normal section indexes. What do you think?

SHN_XINDEX is possibly a special case though. Not sure about that one, but it's possible it just works.

grimar updated this revision to Diff 187773.Feb 21 2019, 5:36 AM
grimar marked an inline comment as done.
  • Addressed review comment.
tools/obj2yaml/elf2yaml.cpp
279

Yes, I think you're right. I used 0xff01 value which maps to SHN_HEXAGON_SCOMMON_1 atm.
We actually map/print this value for EM_X86_64 yamls too, and it does not seem right to me,
I would expect to see just a hex value in output for that case.
So I switched the test to EM_HEXAGON, that allows to test it and makes the test case to be correct.

About SHN_XINDEX: it is a bit more complicated.
We do not support them atm:
https://github.com/llvm-mirror/llvm/blob/master/lib/ObjectYAML/ELFYAML.cpp#L833
So yaml2obj reports "Large indexes are not supported".

I created a little binary using debugger magic to test it. Btw, --no-validate would help here, so I would perhaps
do a patch for it after landing this one. It should allow getting rid of this binary as proof of usefulness.

jhenderson added inline comments.Feb 21 2019, 6:39 AM
test/tools/obj2yaml/abs-common-symbols.yaml
1 ↗(On Diff #187773)

The test needs renaming to show its more general purpose now. Also maybe worth a comment to describe the test ;-)

10 ↗(On Diff #187773)

The point of the original test was to show that both SHN_ABS and 0xfff1 can be handled. I don't think it matters too much whether both are tested or not, but there's not much point in having duplicates.

21 ↗(On Diff #187773)

How about testing another arbitrary value in the reserved range that doesn't necessarily correspond to any known value?

grimar updated this revision to Diff 187791.Feb 21 2019, 7:01 AM
grimar marked 3 inline comments as done.
grimar retitled this revision from [obj2yaml] - Do not miss section index for SHN_ABS and SHN_COMMON symbols. to [obj2yaml] - Do not miss section index for special symbols..
grimar edited the summary of this revision. (Show Details)
  • Addressed review comments.
jhenderson added inline comments.Feb 21 2019, 7:30 AM
test/tools/obj2yaml/special-symbols-indices.yaml
1 ↗(On Diff #187791)

Nit: test name should be special-symbol-indices.test (you do not need to make "symbol" plural).

This revision is now accepted and ready to land.Feb 21 2019, 7:30 AM
grimar marked an inline comment as done.Feb 21 2019, 11:56 PM
grimar added inline comments.
test/tools/obj2yaml/special-symbols-indices.yaml
1 ↗(On Diff #187791)

Thanks :)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2019, 12:44 AM