This is an archive of the discontinued LLVM Phabricator instance.

[Object, llvm-readelf] - Move the API for retrieving symbol versions to ELF.h
ClosedPublic

Authored by grimar on Jan 15 2021, 5:48 AM.

Details

Summary

ELFDumper.cpp implements the functionality that allows to get symbol versions.
It is used for dumping versioned symbols.

This helps to implement https://bugs.llvm.org/show_bug.cgi?id=48670 ("make llvm-nm -D print version names"):
we can move out and reuse the code from ELFDumper.cpp.
This is what this patch do: it moves the related functionality to ELFFile<ELFT>.

Diff Detail

Event Timeline

grimar created this revision.Jan 15 2021, 5:48 AM
grimar requested review of this revision.Jan 15 2021, 5:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2021, 5:48 AM
MaskRay accepted this revision.Jan 15 2021, 2:45 PM

Thanks for looking into llvm-nm -D😊

We have the https://bugs.llvm.org/show_bug.cgi?id=48670 bug which suggests that llvm-nm also should print version names for symbols.

This helps implement https://bugs.llvm.org/show_bug.cgi?id=48670 (make llvm-nm -D print version names).😊

llvm/include/llvm/Object/ELF.h
588
llvm/tools/llvm-readobj/ELFDumper.cpp
382

sizeof(VersionEntry)=40.

Inline size 16 is too large... perhaps just 0 is fine. The malloc cost is affordable.

This revision is now accepted and ready to land.Jan 15 2021, 2:45 PM

Mostly looks fine. It might be nice if we could avoid putting so much code in the ELF.h header file, but that would require more refactoring, so we probably want to leave that to another patch.

llvm/test/tools/llvm-readobj/ELF/verdef-invalid.test
277

What's happened to these error messages?

grimar added inline comments.Jan 18 2021, 12:49 AM
llvm/test/tools/llvm-readobj/ELF/verdef-invalid.test
277

It is because of changes in GNUELFDumper<ELFT>::printVersionSymbolSection.

Previoulsy, ELFDumper<ELFT>::getSymbolVersionByIndex loaded the version mapping internally
(ELFDumper<ELFT>::LoadVersionMap()), because it stored it in VersionMap member.

The new code moves getSymbolVersionByIndex to ELFFile<ELFT>. And now it hase VersionMap argument:

Expected<StringRef> getSymbolVersionByIndex(
    uint32_t SymbolVersionIndex, bool &IsDefault,
    SmallVector<Optional<VersionEntry>, 16> &VersionMap) const;

So now its up to caller to decide if VersionMap is stored and where. Doesn't seem like we want to add a
VersionMap member to ELFFile<ELFT>.

With that currently we might have an error when we call

Expected<SmallVector<Optional<VersionEntry>, 16> *> MapOrErr =
      getVersionMap();

which might occur a bit earlier in the code, i.e. before we start iterating over version entries.

Note it doesn't seem to degrade the diagnostics reported. The reason ("invalid SHT_GNU_verdef...") is still printed and remains the same.
It is probably even better as it general and reported once for all entries now.

grimar edited the summary of this revision. (Show Details)Jan 18 2021, 12:54 AM
grimar updated this revision to Diff 317277.Jan 18 2021, 12:58 AM
grimar marked 3 inline comments as done.
  • Addressed review comments.