Page MenuHomePhabricator

[llvm-readobj] Add --coff-tls-directory flag to print TLS Directory & test.
ClosedPublic

Authored by luqmana on Oct 1 2020, 1:02 AM.

Details

Summary

Akin to dumpbin's /TLS option, this will print out the TLS directory, if
present, in the image.

Example output:

> llvm-readobj --coff-tls-directory test.exe
File: test.exe
Format: COFF-x86-64
Arch: x86_64
AddressSize: 64bit
TLSDirectory {
  StartAddressOfRawData: 0x140004000
  EndAddressOfRawData: 0x140004040
  AddressOfIndex: 0x140002000
  AddressOfCallBacks: 0x0
  SizeOfZeroFill: 0x0
  Characteristics [ (0x0)
  ]
}

Diff Detail

Event Timeline

luqmana created this revision.Oct 1 2020, 1:02 AM
luqmana requested review of this revision.Oct 1 2020, 1:02 AM
luqmana edited the summary of this revision. (Show Details)Oct 1 2020, 1:04 AM
jhenderson requested changes to this revision.Oct 1 2020, 1:14 AM

Thanks for the patch. It looks to me like this patch is intended to add a new option to llvm-readobj. This should be tested in the llvm-readobj tests, and not in the LLD tests. If you want a new test for LLD, you'll probably want a separate patch for using this new option. You'll want to use a different method to create your input for llvm-readobj, since the LLVM tools should be testable without it. This might require you to use something like yaml2obj to create your test input (I'm assuming you need a "linked" image, rather than a relocatable COFF object - I'm not particularly familiar with COFF).

You also need to update the documentation located at llvm\docs\CommandGuide\llvm-readobj.rst with the new option.

I can't comment on the functional logic of the patch, due to lack of expertise with COFF. Please make sure to add some reviewers who have touched/reviewed the COFF dumping code recently.

lld/test/COFF/tls-alignment.ll
1 ↗(On Diff #295480)

It would be helpful to have a comment at the top of the file explaining what this test is intended to test.

39 ↗(On Diff #295480)

Nit: add the missing new line at EOF.

llvm/lib/Object/COFFObjectFile.cpp
662

LLVM style uses UpperCamelCase for variable names.

665

Nit: missing trailing full stop.

667

It would be better to use createStringError to provide a more meaningful failure message (something like "TLS table size <size> is not the expected size <expected size>").

llvm/tools/llvm-readobj/COFFDumper.cpp
2048

Nit: add missing new line at EOF.

This revision now requires changes to proceed.Oct 1 2020, 1:14 AM
luqmana updated this revision to Diff 295498.Oct 1 2020, 2:23 AM

Add llvm-readobj test and address comments.

Added a test specifically for llvm-readobj and removed the LLD test from this
revision. Also updated the naming from TLS table to TLS directory to match
the actual underlying structure name (IMAGE_TLS_DIRECTORY).

luqmana retitled this revision from [llvm-readobj][LLD] Add --coff-tls-table flag to print TLS Table & test. to [llvm-readobj] Add --coff-tls-directory flag to print TLS Directory & test..Oct 1 2020, 2:27 AM
luqmana edited the summary of this revision. (Show Details)
luqmana updated this revision to Diff 295501.Oct 1 2020, 2:29 AM

Add newline at the end of new test.

Thanks for the patch. It looks to me like this patch is intended to add a new option to llvm-readobj. This should be tested in the llvm-readobj tests, and not in the LLD tests. If you want a new test for LLD, you'll probably want a separate patch for using this new option. You'll want to use a different method to create your input for llvm-readobj, since the LLVM tools should be testable without it. This might require you to use something like yaml2obj to create your test input (I'm assuming you need a "linked" image, rather than a relocatable COFF object - I'm not particularly familiar with COFF).

You also need to update the documentation located at llvm\docs\CommandGuide\llvm-readobj.rst with the new option.

I can't comment on the functional logic of the patch, due to lack of expertise with COFF. Please make sure to add some reviewers who have touched/reviewed the COFF dumping code recently.

Yea, I'll just split out that LLD test into a separate revision. For the llvm-readobj specifc test, I looked around the existing tests and kinda copied the same setup by using a locally generated .exe (with instructions how in the test).

The code is still missing quite a bit of testing. Here are cases that I can see don't have any coverage:

  1. No TLS directory means nothing is printed.
  2. 32 versus 64 bit versions (I don't know which one is currently being tested, but they both should be).
  3. The various different branches in initTLSDirectoryPtr, including, but not limited to, the error case.

I've also added several people who have recently touched or reviewed code in llvm-readobj as reviewers.

grimar added inline comments.Oct 6 2020, 3:36 AM
llvm/test/tools/llvm-readobj/COFF/tls-directory.test
5

Is it possible to use yaml2obj and a YAML description instead of using a precompiled binary?
(I'd expect that you should be able to use obj2yaml tool on has-tls.exe and then reduce its output to prepare a little YAML test input).

luqmana updated this revision to Diff 296574.Oct 6 2020, 6:29 PM

Add more tests and use YAML instead of precompiled binary.

luqmana marked 4 inline comments as done.Oct 6 2020, 7:57 PM
grimar added a comment.Oct 7 2020, 1:12 AM

I think you can:

  1. Merge 7 test cases you have into one (use yaml2obj's --docnum option, see examples in llvm-readobj\ELF\)
  2. Perhaps you can reuse YAMLs with use of yaml2obj's macros feature (-D<macro>=<value>). See llvm-readobj\ELF\reloc-addends.test and many other tests.
  3. It feels that you should be able to reduce YAML descriptions, e.g. remove excessive sections? (so, try to keep parts that are related/needed only)
llvm/test/tools/llvm-readobj/COFF/tls-directory-64.test
17 ↗(On Diff #296574)

use CHECK-NEXT (and in other tests).

luqmana updated this revision to Diff 296626.Oct 7 2020, 2:33 AM
  • Consolidate and simplify tests.
luqmana marked an inline comment as done.Oct 7 2020, 2:37 AM

I think you can:

  1. Merge 7 test cases you have into one (use yaml2obj's --docnum option, see examples in llvm-readobj\ELF\)
  2. Perhaps you can reuse YAMLs with use of yaml2obj's macros feature (-D<macro>=<value>). See llvm-readobj\ELF\reloc-addends.test and many other tests.
  3. It feels that you should be able to reduce YAML descriptions, e.g. remove excessive sections? (so, try to keep parts that are related/needed only)

Done but I just left the 32bit and 64bit yaml separate as I don't think it's worth combining them. But I did get it down to just 3 docs overall. Also removed a lot of the stuff unnecessary for the tests. They won't run now but that's not important for the test I guess.

The updated version looks good overall to me. Few suggestions/questions are inlined.

They won't run now but that's not important for the test I guess.

Yeah, for llvm-readelf/obj tests we don't care.

llvm/test/tools/llvm-readobj/COFF/tls-directory.test
14

I am not sure it is a useful comment? It is not clear what this binary blob says probably.

82

The same.

112

I wonder if in this case an empty scope should be emitted. I.e:

TLSDirectory {
}

This is what we do for ELF sometimes, e.g:

template <typename ELFT> void ELFDumper<ELFT>::printHashTable() {
  DictScope D(W, "HashTable");
  if (!HashTable)
    return;

but I am not familar with llvm-readelf COFF to say what is the preferred style.
Could you check?

139

Perhaps this and few other fields are not valuable and could be set to 0?
(otherwise it looks like them are used, though I guess them are not).

jhenderson added inline comments.Oct 8 2020, 12:52 AM
llvm/test/tools/llvm-readobj/COFF/tls-directory.test
4
8
13–14

I'm not sure I understand the purpose of this comment.

76
81–82

Same comment as above - what is this comment giving us?

108–114

I'd move this to be with the other no-tls case below, so that malformed input cases appear next to each other.

117

No need to repeast this line - it's just a duplicate of the earlier comment.

luqmana updated this revision to Diff 296883.Oct 8 2020, 1:07 AM

Address review comments.

  • Emit empty scope if no TLS Directory.
  • Remove unclear comments.
  • Zero out unused fields in test.
luqmana updated this revision to Diff 296886.Oct 8 2020, 1:15 AM
Fix typos and put related test cases together.
luqmana marked 11 inline comments as done.Oct 8 2020, 1:19 AM
luqmana added inline comments.
llvm/test/tools/llvm-readobj/COFF/tls-directory.test
13–14

That's the binary form of the TLSDirectory struct we're printing out. I had the comment there before so it'd be closer to the actual CHECK lines. But the file is yaml has only one section now and is small enough anyways so I'll just remove that.

112

Looked at a couple methods and they seem to do the same. Updated to follow that style.

jhenderson accepted this revision.Oct 8 2020, 1:38 AM

LGTM, with one outstanding nit.

llvm/test/tools/llvm-readobj/COFF/tls-directory.test
4

This hasn't been addressed.

This revision is now accepted and ready to land.Oct 8 2020, 1:38 AM
grimar accepted this revision.Oct 8 2020, 1:40 AM

LGTM with a nit addressed.

llvm/test/tools/llvm-readobj/COFF/tls-directory.test
160

You don't need to have NO-TLS2 and NO-TLS1. You can just have a single NO-TLS and reuse it.

luqmana updated this revision to Diff 296893.Oct 8 2020, 1:44 AM
luqmana marked 2 inline comments as done.
Use same check statements for empty TLS directory tests and fix nits.
luqmana marked 2 inline comments as done.Oct 8 2020, 1:45 AM
luqmana added inline comments.
llvm/test/tools/llvm-readobj/COFF/tls-directory.test
4

Whoops, fixed that and the same case below.

This revision was landed with ongoing or failed builds.Oct 8 2020, 1:57 AM
This revision was automatically updated to reflect the committed changes.
luqmana marked an inline comment as done.