Page MenuHomePhabricator

[Object] object::ELFObjectFile::symbol_begin(): skip symbol index 0
ClosedPublic

Authored by MaskRay on May 23 2019, 2:45 AM.

Details

Summary

For clients iterating the symbol table, none expects to handle index 0
(STN_UNDEF). Skip it to improve consistency with other binary formats.
Clients that need STN_UNDEF (e.g. lld) can use
getSectionContentsAsArray().

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.May 23 2019, 2:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2019, 2:45 AM
mtrent accepted this revision.May 23 2019, 8:05 AM
This revision is now accepted and ready to land.May 23 2019, 8:05 AM

Have you made sure that there aren't any uses which would result in an undesired change in behaviour because of this? I feel like it's something that people wouldn't expect to change, and therefore wouldn't test explicitly necessarily.

There should also be a test case somewhere (not that I have a good suggestion for where).

Have you made sure that there aren't any uses which would result in an undesired change in behaviour because of this? I feel like it's something that people wouldn't expect to change, and therefore wouldn't test explicitly necessarily.

There should also be a test case somewhere (not that I have a good suggestion for where).

I've checked the call sites:

In RuntimeDyld, if (Flags & SymbolRef::SF_Undefined) continue; filters out index 0.
In llvm-objdump, the filtering code is removed in this patch.
In lib/Object/SymbolSize.cpp, I think the code doesn't notice index 0 is included... but it doesn't seem to matter.
In llvm-nm.cpp, I'll leave the test to @chrisjackson in D62148

This revision was automatically updated to reflect the committed changes.

Have you made sure that there aren't any uses which would result in an undesired change in behaviour because of this? I feel like it's something that people wouldn't expect to change, and therefore wouldn't test explicitly necessarily.

There should also be a test case somewhere (not that I have a good suggestion for where).

I've checked the call sites:

In RuntimeDyld, if (Flags & SymbolRef::SF_Undefined) continue; filters out index 0.
In llvm-objdump, the filtering code is removed in this patch.
In lib/Object/SymbolSize.cpp, I think the code doesn't notice index 0 is included... but it doesn't seem to matter.
In llvm-nm.cpp, I'll leave the test to @chrisjackson in D62148

I have included the test as part of D62955.