Page MenuHomePhabricator

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

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

Diff Detail

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
9

Nit: Spurious blank line?

tools/llvm-objdump/ELFDump.cpp
162

printSymbolVersionRef -> printSymbolVersionRefs or printSymbolVersionReferences

164

Why not use using here?

171

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

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

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

Rather than having this big if statement, do

if (GnuVerDepSec != nullptr)
  return;
197–198

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.

202

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

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

241–261

I'm not sure I follow what you mean?

tools/llvm-objdump/llvm-objdump.cpp
2224

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

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

241–261

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

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
179

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);
  }
}
185

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

191

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

207

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
179

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
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
179

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.

194

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

204

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
204

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
194

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

204

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
194

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

217

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
5 ↗(On Diff #188074)

Looks good to me here :)

tools/llvm-objdump/ELFDump.cpp
183

Use PRIu16, not "u" here.

217

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

241–261

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.