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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

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

In D88635#2316265 <https://reviews.llvm.org/D88635#2316265>, @luqmana wrote:

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



================
Comment at: llvm/test/tools/llvm-readobj/COFF/tls-directory.test:13
+## .rdata
+## 004040000840400000204000000000000000000000003000
+
----------------
I am not sure it is a useful comment? It is not clear what this binary blob says probably.


================
Comment at: llvm/test/tools/llvm-readobj/COFF/tls-directory.test:81
+## .rdata
+## 00400040010000000840004001000000002000400100000000000000000000000000000000003000
+
----------------
The same.


================
Comment at: llvm/test/tools/llvm-readobj/COFF/tls-directory.test:111
+# RUN: yaml2obj %s --docnum=2 -o %t.no-tls1.exe -DTLSRVA=0 -DTLSSIZE=0
+# RUN: llvm-readobj --coff-tls-directory %t.no-tls1.exe | FileCheck %s --check-prefix NO-TLS1
+
----------------
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?


================
Comment at: llvm/test/tools/llvm-readobj/COFF/tls-directory.test:138
+  DLLCharacteristics: []
+  SizeOfStackReserve: 1048576
+  SizeOfStackCommit: 4096
----------------
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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88635/new/

https://reviews.llvm.org/D88635