Page MenuHomePhabricator

[llvm-nm] Fix for BZ41711 - Class character for a symbol with undefined binding does not match class assigned by GNU nm

Authored by chrisjackson on Jun 14 2019, 9:33 AM.



A symbol with a binding of 0x5 that is present in a .text section is assigned the class character 'T'. However, gnu-nm assigns the character '?'. I have added logic that mimics that present in GNU nm's bfd_decode_symclass() from bfd\symc.c.

See 41711.

The associated test was XFailing, but I also had to make a slight correction of the symbol names.

Diff Detail

Event Timeline

chrisjackson created this revision.Jun 14 2019, 9:33 AM
chrisjackson edited the summary of this revision. (Show Details)Jun 14 2019, 9:41 AM
chrisjackson edited the summary of this revision. (Show Details)Jun 14 2019, 10:05 AM
chrisjackson added reviewers: MaskRay, mtrent.
mtrent accepted this revision.Jun 15 2019, 12:30 AM

Respectfully, I am not a good reviewer for ELF-specific file format changes. I assume you added me as a proxy for 'enderby', who touched some of the lines of this routine. Looking at the SVN history I believe Kevin Enderby's involvement was limited to promulgating Lang Hames' "Expected<>" as the preferred error handling idiom in libObject and friends. So, again, respectfully, neither Kevin nor I are what I would consider good, authoritative reviewers for ELF.

The changes here look reasonable to me from a generic programming / code flow point-of-view. If you need something domain specific, I suggest you look elsewhere?

This revision is now accepted and ready to land.Jun 15 2019, 12:30 AM
MaskRay accepted this revision.Jun 16 2019, 2:04 AM

LG, but you may want to wait for other reviewers' opinions.

jhenderson added inline comments.Jun 17 2019, 4:02 AM

My immediate thought when looking at the code here was "what about STB_WEAK"? I know this is handled elsewhere, so maybe it's worth an assert that the binding isn't STB_WEAK, along with a comment explaining why it can't be here.

I also wonder about symbols in the STB_LOOS/STB_HIOS ranges STB_LOPROC/STB_HIPROC. Does GNU in these cases always emit '?'? Please add test-cases for these.

Tidied up the test and added some additional cases. Added an assert explaining the tested binding values.

chrisjackson closed this revision.Sep 3 2019, 2:53 AM

This was closed with commit 41e20d210, svn r364559.