This is an archive of the discontinued LLVM Phabricator instance.

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

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

Diff Detail

Event Timeline

Higuoxing created this revision.Nov 19 2018, 4:33 AM
Higuoxing updated this revision to Diff 174605.Nov 19 2018, 6:51 AM

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

formatted

Higuoxing added inline comments.Nov 19 2018, 7:00 AM
tools/llvm-objdump/ELFDump.cpp
343–363

Seems that these codes are redundant, can we simplify this?

Higuoxing updated this revision to Diff 174716.Nov 19 2018, 5:19 PM

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

Rename some variables

Higuoxing updated this revision to Diff 174737.Nov 20 2018, 2:17 AM

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

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
271

printSymbolVersionRef -> printSymbolVersionRefs or printSymbolVersionReferences

273

Why not use using here?

280

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).

288–290

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.

296

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).

304

Rather than having this big if statement, do

if (GnuVerDepSec != nullptr)
  return;
306–307

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.

311

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.

317

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

343–363

I'm not sure I follow what you mean?

tools/llvm-objdump/llvm-objdump.cpp
1887

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
273

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

343–363

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
296

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

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

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

300

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

316

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
6

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

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.Feb 25 2019, 12:47 AM
test/tools/llvm-objdump/verneed-wrong-info.test
6

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

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

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

313

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.Feb 25 2019, 12:49 AM
tools/llvm-objdump/ELFDump.cpp
313

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

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

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

Higuoxing marked 3 inline comments as done.Feb 25 2019, 1:18 AM
Higuoxing added inline comments.
tools/llvm-objdump/ELFDump.cpp
303

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

313

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

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

326

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

jhenderson added inline comments.Feb 25 2019, 1:51 AM
test/tools/llvm-objdump/verneed-wrong-info.test
6

Looks good to me here :)

tools/llvm-objdump/ELFDump.cpp
292

Use PRIu16, not "u" here.

326

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

343–363

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

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

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

Thanks a lot!

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

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

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

LGTM.

This revision was automatically updated to reflect the committed changes.