Page MenuHomePhabricator

[llvm-objdump] Add `Version References` dumper (WIP) (PR30241)
Needs ReviewPublic

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

Details

Summary

Add symbol version dumper for #30241

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
300–320

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

300–320

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.

300–320

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.Wed, Dec 19, 1:31 AM

(WIP) This is a backup.