This is an archive of the discontinued LLVM Phabricator instance.

[elfabi] Add support for reading DT_NEEDED from binaries
ClosedPublic

Authored by amontanez on Dec 18 2018, 1:55 PM.

Details

Summary

This patch gives elfabi the ability to read DT_NEEDED entries from ELF binaries to populate NeededLibs in TextAPI's ELFStub.

Diff Detail

Repository
rL LLVM

Event Timeline

amontanez created this revision.Dec 18 2018, 1:55 PM

Other than me realizing I made a mistake in previous reviews and a minor little issue (NeededLibCount), this LGTM.

llvm/tools/llvm-elfabi/ELFObjHandler.cpp
122 ↗(On Diff #178779)

You never use the NeededLibCount. Why do you have it?

llvm/tools/llvm-elfabi/ELFObjHandler.h
31 ↗(On Diff #178779)

ditto (below)

37 ↗(On Diff #178779)

ditto (below)

45 ↗(On Diff #178779)

ditto (below)

56 ↗(On Diff #178779)

Just realized something I should have caught in previous reviews. You don't need these in the headers. You can make them static template functions in just the file they're used in. That reduces .o size and improves the compilers ability to perform optimizations.

69 ↗(On Diff #178779)

Ditto here.

ruiu added a subscriber: ruiu.Dec 18 2018, 5:11 PM
ruiu added inline comments.
llvm/test/tools/llvm-elfabi/binary-read-neededlibs.test
35 ↗(On Diff #178779)

nit: remove extraneous space characters?

llvm/tools/llvm-elfabi/ELFObjHandler.cpp
130 ↗(On Diff #178779)

Is NeededLibs empty on function entry? If so, you can return TargetStub.NeededLibs.count() and remove NeededLibCount variable.

Rebase onto previous patch resolves most of the comments for this patch.

amontanez marked 8 inline comments as done.Dec 20 2018, 11:37 AM
ruiu added inline comments.Dec 20 2018, 11:46 AM
llvm/tools/llvm-elfabi/ELFObjHandler.cpp
126 ↗(On Diff #179109)

nit: I believe you don't need std::string as std::string has an implicit constructor from const char *.

amontanez marked an inline comment as done.

Rebase and use terminatedSubstr() to fetch DT_NEEDED strings from .dynstr string table.

ruiu added inline comments.Dec 28 2018, 2:02 PM
llvm/tools/llvm-elfabi/ELFObjHandler.cpp
45–51 ↗(On Diff #179311)

Indentation should be 2 spaces.

148 ↗(On Diff #179311)

I think all strings in a string table should be terminated with '\0' in any ELF file. If the last string "overruns" the string table, it is an error, and the last string should not automatically be terminated at the end of the string table.

amontanez updated this revision to Diff 180174.Jan 3 2019, 4:29 PM

Rebase on D55629 change that addresses strings that overrun string table bounds.

amontanez marked 2 inline comments as done.Jan 3 2019, 4:31 PM

Rebase again.

Rebase, update error message for DT_NEEDED entries that are outside the string table, and add another test.

This revision is now accepted and ready to land.Jan 18 2019, 11:45 AM
This revision was automatically updated to reflect the committed changes.