Page MenuHomePhabricator

[llvm-objdump] Add `Version References` dumper
ClosedPublic

Authored by Higuoxing on Nov 19 2018, 4:33 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Could you add tests for the cases where a) there is no DT_VERNEEDNUM tag, and b) where it has a value of 0, please.

Could you also point me at some documentation describing the VerNeed structs etc.

test/tools/llvm-objdump/private-headers-symbol-version.test
8 ↗(On Diff #174605)

Nit: Spurious blank line?

tools/llvm-objdump/ELFDump.cpp
197–198 ↗(On Diff #174716)

I'm trying to understand the structure of the VerNeed stuff. Is it a variable size block, and hence the need to iterate over bytes rather than struct instances?

Also, prefer reinterpret_cast instead of C-style, here and elsewhere in this function.

162 ↗(On Diff #174605)

printSymbolVersionRef -> printSymbolVersionRefs or printSymbolVersionReferences

164 ↗(On Diff #174605)

Why not use using here?

239–259 ↗(On Diff #174605)

I'm not sure I follow what you mean?

171 ↗(On Diff #174737)

d_val's type is different in 32 bit and 64-bit ELF, so this should be a type large enough to hold either version (i.e. uin64_t).

179–181 ↗(On Diff #174737)

Does GNU objdump print the header if there is a DT_VERNEEDNUM tag with a value of 0? I feel like it would be reasonable to do so in our case.

187 ↗(On Diff #174737)

You can use ELFT::Elf_Shdr here and elsewhere directly, and avoid the need for the typename.

I'm also not sure what "GNUVerDepSec" means, so that probably needs expanding a bit more (especially the "Dep" part).

195 ↗(On Diff #174737)

Rather than having this big if statement, do

if (GnuVerDepSec != nullptr)
  return;
202 ↗(On Diff #174737)

unsigned -> uint64_t to match my earlier comment.

Maybe worth replacing I with VerNeedIndex or something, and J below similarly, just to disambiguate them a bit more.

208 ↗(On Diff #174737)

unsigned is probably the wrong type. Use the type of vn_cnt or just the largest uint*_t type needed.

tools/llvm-objdump/llvm-objdump.cpp
2224 ↗(On Diff #174716)

put the return on a new line, since printELFSymbolVersionRef doesn't return anything.

Higuoxing added a comment.EditedNov 20 2018, 3:56 AM

Could you add tests for the cases where a) there is no DT_VERNEEDNUM tag, and b) where it has a value of 0, please.

Sure, I will add them

Could you also point me at some documentation describing the VerNeed structs etc.

https://refspecs.linuxfoundation.org/LSB_2.1.0/LSB-Core-generic/LSB-Core-generic/symverdefs.html
https://refspecs.linuxfoundation.org/LSB_2.1.0/LSB-Core-generic/LSB-Core-generic/symverrqmts.html

PS. Thanks for your kind reviewing, I will cover them one by one :)

Higuoxing updated this revision to Diff 174884.Nov 21 2018, 1:36 AM

Updating D54697: [llvm-objdump] Add Version References dumper (PR30241)
Partially refactored some codes

Higuoxing updated this revision to Diff 174885.Nov 21 2018, 1:38 AM
Higuoxing marked an inline comment as done.

Updating D54697: [llvm-objdump] Add Version References dumper (PR30241)

delete #include "llvm/Object/ELFTypes.h"

Higuoxing marked 7 inline comments as done.Nov 21 2018, 1:45 AM
Higuoxing added inline comments.
tools/llvm-objdump/ELFDump.cpp
164 ↗(On Diff #174605)

I use typedef here, just like what they do in other functions.

239–259 ↗(On Diff #174605)

I mean, these functions are similar (they all cast ELF<32|64><BE|LE> to ElfObj), can we simplify these functions?

printELFFileHeader
printELFDynamicSection
printELFSymbolVersionRefs
Higuoxing added inline comments.Nov 21 2018, 1:47 AM
tools/llvm-objdump/ELFDump.cpp
187 ↗(On Diff #174737)

I change it to VerNeedSec

Higuoxing retitled this revision from [llvm-objdump] Add `Version References` dumper (PR30241) to [llvm-objdump] Add `Version References` dumper (WIP) (PR30241).Nov 21 2018, 2:00 AM

I want to use YAML2Obj to generate the test files, but there are something wrong with that tool. So, I have to write a patch for that. This patch have to be suspended for some time, until that one landed ...

Higuoxing planned changes to this revision.Nov 23 2018, 11:38 PM
Higuoxing updated this revision to Diff 176276.Dec 2 2018, 2:52 AM

Add dumper for symbol version definitions.

Higuoxing planned changes to this revision.Dec 2 2018, 2:52 AM
Higuoxing planned changes to this revision.Dec 15 2018, 1:49 AM
Higuoxing updated this revision to Diff 178347.
Higuoxing added a reviewer: emaste.
jhenderson added a comment.EditedDec 17 2018, 4:40 AM

Hi @Higuoxing. Is this ready for review again? It is currently marked as WIP/changes planned.

Hi @Higuoxing. Is this ready for review again? It is currently marked as WIP/changes planned.

This works, but there are some little refactors needed. Besides, I cannot give enough test cases right now ...

Higuoxing updated this revision to Diff 178847.Dec 19 2018, 1:31 AM

(WIP) This is a backup.

Higuoxing updated this revision to Diff 187899.Feb 21 2019, 9:48 PM

Sorry for not responding on this patch for 2 months. Could anyone review this? Thanks a lot.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2019, 9:48 PM
Herald added a subscriber: rupprecht. · View Herald Transcript
Higuoxing retitled this revision from [llvm-objdump] Add `Version References` dumper (WIP) (PR30241) to [llvm-objdump] Add `Version References` dumper.Feb 21 2019, 9:49 PM

Updating D54697: [llvm-objdump] Add Version References dumper

I'll take a look at this next week, if nobody else does in the meantime.

Higuoxing updated this revision to Diff 187942.Feb 22 2019, 8:36 AM

Updating D54697: [llvm-objdump] Add Version References dumper

grimar added a subscriber: grimar.EditedFeb 23 2019, 2:49 AM

I did not see this patch earlier and so worked on something very close to this for some time. I stopped my work
and shared the ideas/code I have in the comments here :)

tools/llvm-objdump/ELFDump.cpp
288 ↗(On Diff #187942)

If you move this loop to printSymbolVersionInfo and change the printSymbolVersionDependency signature
to printSymbolVersionDependency(const typename ELFT::Shdr &Shdr, ArrayRef<uint8_t> Contents)
that probably will be better, because for printSymbolVersionDefinition implementation you would also
need to iterate over all sections and take the content. That can be done in a single place.

I mean the code in printSymbolVersionInfo can be like:

template <class ELFT>
static void printSymbolVersionInfo(const ELFFile<ELFT> *Elf) {
  typedef typename ELFT::Shdr Elf_Shdr;

  auto SecOrErr = Elf->sections();
  if (!SecOrErr)
    report_fatal_error(errorToErrorCode(SecOrErr.takeError()).message());

  for (const Elf_Shdr &Shdr : *SecOrErr) {
    if (Shdr.sh_type != ELF::SHT_GNU_verdef &&
        Shdr.sh_type != ELF::SHT_GNU_verneed)
      continue;

    auto ContentsOrErr = Elf->getSectionContents(&Shdr);
    if (!ContentsOrErr)
      report_fatal_error(errorToErrorCode(ContentsOrErr.takeError()).message());

    if (Shdr.sh_type == ELF::SHT_GNU_verdef)
      printSymbolVersionDefinition<ELFT>(Shdr, *ContentsOrErr);
    else if (Shdr.sh_type == ELF::SHT_GNU_verneed)
      printSymbolVersionDependency<ELFT>(Shdr, *ContentsOrErr);
  }
}
294 ↗(On Diff #187942)

Actually, you do not need this I think.
You can simply iterate over Elf_Verneed entries until vn_next != 0.

300 ↗(On Diff #187942)

I would use getSectionContents (see sample above), because it does more checks.

316 ↗(On Diff #187942)

The same, I think, you can simply iterate until vna_next != 0.

grimar added inline comments.Feb 23 2019, 3:01 AM
test/tools/llvm-objdump/verneed-wrong-info.test
5 ↗(On Diff #187942)

The case looks simple, so I would remove the link and just describe it.
i.e. something like:

"We have a SHT_GNU_verneed section with a broken sh_info field that says the section
contains more entries that it actually have"

Higuoxing marked an inline comment as done.
Higuoxing added inline comments.
tools/llvm-objdump/ELFDump.cpp
288 ↗(On Diff #187942)

Oh, thank you @grimar , I am wandering that whether the dumping order of verneed and verdef sections is important? Sometimes, the verneed section is before verdef and sometimes verneed is after verdef.

I found that elf2yaml codes could be used here, so I just do some copy and paste work.

I don't know if we need some checks on sh_info field and DT_VERNEED, just like what gnu-objdump do.

grimar added inline comments.Mon, Feb 25, 12:47 AM
test/tools/llvm-objdump/verneed-wrong-info.test
5 ↗(On Diff #188074)

that->than, have->has (sorry, did these mistakes/mistypes when suggested it).

I hope @jhenderson will re-check it though :)

tools/llvm-objdump/ELFDump.cpp
288 ↗(On Diff #187942)

I did not notice that, but if so then I guess GNU objdump just print them in the same order as they are in a section table probably.
i.e. the same is done in this patch.

303 ↗(On Diff #188074)

Shouldn't you be able to get Filename from Elf?

313 ↗(On Diff #188074)

If you're not going to implement SHT_GNU_verneed in this patch then I think you should
get rid of Shdr.sh_type != ELF::SHT_GNU_verneed.

grimar added inline comments.Mon, Feb 25, 12:49 AM
tools/llvm-objdump/ELFDump.cpp
313 ↗(On Diff #188074)

I meant "if you're not going to implement ELF::SHT_GNU_verdef" of course..

Higuoxing updated this revision to Diff 188105.Mon, Feb 25, 1:14 AM

Updating D54697: [llvm-objdump] Add Version References dumper

Higuoxing marked 3 inline comments as done.Mon, Feb 25, 1:18 AM
Higuoxing added inline comments.
tools/llvm-objdump/ELFDump.cpp
303 ↗(On Diff #188074)

Sorry, I cannot find a getFileName() method in Elf or something else like that.

313 ↗(On Diff #188074)

Thanks for reviewing, I would like to implement it in next patch :)

This looks fine, thanks! (with a minor nit addressed)
Let's see if anyone else has comments.

tools/llvm-objdump/ELFDump.cpp
303 ↗(On Diff #188074)

Yeah, I see now. Nevermind, I looked at the wrong class I think.

326 ↗(On Diff #188105)

You do not need this if, this condition is always true now.

jhenderson added inline comments.Mon, Feb 25, 1:51 AM
test/tools/llvm-objdump/verneed-wrong-info.test
5 ↗(On Diff #188074)

Looks good to me here :)

tools/llvm-objdump/ELFDump.cpp
239–259 ↗(On Diff #174605)

You're welcome to try some sort of refactor, but do it in another patch.

292 ↗(On Diff #188105)

Use PRIu16, not "u" here.

326 ↗(On Diff #188105)

You don't need this if as the early continue above means that this if will always be true.

Higuoxing updated this revision to Diff 188124.Mon, Feb 25, 3:45 AM

Updating D54697: [llvm-objdump] Add Version References dumper

Thanks a lot!

jhenderson accepted this revision.Mon, Feb 25, 3:57 AM

LGTM, but please wait for @grimar to confirm he is happy.

This revision is now accepted and ready to land.Mon, Feb 25, 3:57 AM
grimar accepted this revision.Mon, Feb 25, 4:50 AM

LGTM.

This revision was automatically updated to reflect the committed changes.