[PATCH] D88635: [llvm-readobj][LLD] Add --coff-tls-table flag to print TLS Table & test.

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: lld/test/COFF/tls-alignment.ll:1
+; RUN: llc -filetype=obj %s -o %t.obj
+; RUN: lld-link %t.obj -entry:main -nodefaultlib -out:%t.exe
----------------
It would be helpful to have a comment at the top of the file explaining what this test is intended to test.


================
Comment at: lld/test/COFF/tls-alignment.ll:39
+}
\ No newline at end of file

----------------
Nit: add the missing new line at EOF.


================
Comment at: llvm/lib/Object/COFFObjectFile.cpp:662
+
+  uint64_t tableSize =
+      is64() ? sizeof(coff_tls_directory64) : sizeof(coff_tls_directory32);
----------------
LLVM style uses UpperCamelCase for variable names.


================
Comment at: llvm/lib/Object/COFFObjectFile.cpp:665
+
+  // Check that the size is correct
+  if (DataEntry->Size != tableSize)
----------------
Nit: missing trailing full stop.


================
Comment at: llvm/lib/Object/COFFObjectFile.cpp:667
+  if (DataEntry->Size != tableSize)
+    return errorCodeToError(object_error::parse_failed);
+
----------------
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>").


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:2047
+}
\ No newline at end of file

----------------
Nit: add missing new line at EOF.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88635