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

[luqmana]

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)
  ]
}

[jhenderson]

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

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

39

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

2047

Nit: add missing new line at EOF.

[luqmana]

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]

Add newline at the end of new test.

[luqmana]

In D88635#2305377, @jhenderson wrote:

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

[luqmana]

Update.

[jhenderson]

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]

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

4 ↗

(On Diff #295519)

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]

Add more tests and use YAML instead of precompiled binary.

[grimar]

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]

• Consolidate and simplify tests.

[luqmana]

In D88635#2316128, @grimar wrote:

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.

[grimar]

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

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

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

13 ↗

(On Diff #296626)

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

81 ↗

(On Diff #296626)

The same.

111 ↗

(On Diff #296626)

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?

138 ↗

(On Diff #296626)

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]

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

3 ↗

(On Diff #296626)

7 ↗

(On Diff #296626)

12–13 ↗

(On Diff #296626)

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

75 ↗

(On Diff #296626)

80–81 ↗

(On Diff #296626)

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

107–113 ↗

(On Diff #296626)

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

116 ↗

(On Diff #296626)

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

[luqmana]

Address review comments.

• Emit empty scope if no TLS Directory.
• Remove unclear comments.
• Zero out unused fields in test.

[luqmana]

Fix typos and put related test cases together.

[luqmana]

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

12–13 ↗

(On Diff #296626)

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.

111 ↗

(On Diff #296626)

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

[jhenderson]

LGTM, with one outstanding nit.

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

3 ↗

(On Diff #296626)

This hasn't been addressed.

[grimar]

LGTM with a nit addressed.

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

159 ↗

(On Diff #296886)

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

[luqmana]

Use same check statements for empty TLS directory tests and fix nits.

[luqmana]

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

3 ↗

(On Diff #296626)

Whoops, fixed that and the same case below.

lld/test/COFF/tls-alignment.ll

+; RUN: llc -filetype=obj %s -o %t.obj
+; RUN: lld-link %t.obj -entry:main -nodefaultlib -out:%t.exe
+; RUN: llvm-readobj --coff-tls-table %t.exe | FileCheck %s
+
+; CHECK: Characteristics [ (0x0)
+
+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+%IMAGE_TLS_DIRECTORY = type {
+    i64,    ; StartAddressOfRawData
+    i64,    ; EndAddressOfRawData
+    i64,    ; AddressOfIndex
+    i64,    ; AddressOfCallBacks
+    i32,    ; SizeOfZeroFill
+    i32     ; Characteristics
+}
+
+@_tls_start = global i8 zeroinitializer, section ".tls"
+@_tls_end = global i8 zeroinitializer, section ".tls$ZZZ"
+@_tls_index = global i64 1
+@_tls_used = global %IMAGE_TLS_DIRECTORY {
+    i64 ptrtoint (i8* @_tls_start to i64),
+    i64 ptrtoint (i8* @_tls_end to i64),
+    i64 ptrtoint (i64* @_tls_index to i64),
+    i64 0,
+    i32 0,
+    i32 0
+}, section ".rdata$T"
+
+@_fltused = global i8 zeroinitializer
+
+@aligned_thread_local = thread_local global [32 x i8] zeroinitializer, align 32
+
+define i32 @main() #0 {
+  store <8 x float> zeroinitializer, <8 x float>* bitcast ([32 x i8]* @aligned_thread_local to <8 x float>*), align 32
+  ret i32 0
+}
 No newline at end of file

llvm/include/llvm/BinaryFormat/COFF.h

enum SectionCharacteristics : uint32_t {
   IMAGE_SCN_ALIGN_64BYTES = 0x00700000,
   IMAGE_SCN_ALIGN_128BYTES = 0x00800000,
   IMAGE_SCN_ALIGN_256BYTES = 0x00900000,
   IMAGE_SCN_ALIGN_512BYTES = 0x00A00000,
   IMAGE_SCN_ALIGN_1024BYTES = 0x00B00000,
   IMAGE_SCN_ALIGN_2048BYTES = 0x00C00000,
   IMAGE_SCN_ALIGN_4096BYTES = 0x00D00000,
   IMAGE_SCN_ALIGN_8192BYTES = 0x00E00000,
+  IMAGE_SCN_ALIGN_MASK = 0x00F00000,
   IMAGE_SCN_LNK_NRELOC_OVFL = 0x01000000,
   IMAGE_SCN_MEM_DISCARDABLE = 0x02000000,
   IMAGE_SCN_MEM_NOT_CACHED = 0x04000000,
   IMAGE_SCN_MEM_NOT_PAGED = 0x08000000,
   IMAGE_SCN_MEM_SHARED = 0x10000000,
   IMAGE_SCN_MEM_EXECUTE = 0x20000000,
   IMAGE_SCN_MEM_READ = 0x40000000,
   IMAGE_SCN_MEM_WRITE = 0x80000000

llvm/include/llvm/Object/COFF.h

private:
   const coff_import_directory_table_entry *ImportDirectory;
   const delay_import_directory_table_entry *DelayImportDirectory;
   uint32_t NumberOfDelayImportDirectory;
   const export_directory_table_entry *ExportDirectory;
   const coff_base_reloc_block_header *BaseRelocHeader;
   const coff_base_reloc_block_header *BaseRelocEnd;
   const debug_directory *DebugDirectoryBegin;
   const debug_directory *DebugDirectoryEnd;
+  const coff_tls_directory32 *TlsTable32;
+  const coff_tls_directory64 *TlsTable64;
   // Either coff_load_configuration32 or coff_load_configuration64.
   const void *LoadConfig = nullptr;
 
   Expected<StringRef> getString(uint32_t offset) const;
 
   template <typename coff_symbol_type>
   const coff_symbol_type *toSymb(DataRefImpl Symb) const;
   const coff_section *toSec(DataRefImpl Sec) const;
   const coff_relocation *toRel(DataRefImpl Rel) const;
 
   // Finish initializing the object and return success or an error.
   Error initialize();
 
   Error initSymbolTablePtr();
   Error initImportTablePtr();
   Error initDelayImportTablePtr();
   Error initExportTablePtr();
   Error initBaseRelocPtr();
   Error initDebugDirectoryPtr();
+  Error initTlsTablePtr();
   Error initLoadConfigPtr();
 
 public:
   static Expected<std::unique_ptr<COFFObjectFile>>
   create(MemoryBufferRef Object);
 
   uintptr_t getSymbolTable() const {
     if (SymbolTable16)
public:
   iterator_range<delay_import_directory_iterator>
       delay_import_directories() const;
   iterator_range<export_directory_iterator> export_directories() const;
   iterator_range<base_reloc_iterator> base_relocs() const;
   iterator_range<const debug_directory *> debug_directories() const {
     return make_range(debug_directory_begin(), debug_directory_end());
   }
 
+  const coff_tls_directory32 *getTlsTable32() const { return TlsTable32; }
+  const coff_tls_directory64 *getTlsTable64() const { return TlsTable64; }
+
   const dos_header *getDOSHeader() const {
     if (!PE32Header && !PE32PlusHeader)
       return nullptr;
     return reinterpret_cast<const dos_header *>(base());
   }
 
   const coff_file_header *getCOFFHeader() const { return COFFHeader; }
   const coff_bigobj_file_header *getCOFFBigObjHeader() const {

llvm/lib/Object/COFFObjectFile.cpp

Error COFFObjectFile::initDebugDirectoryPtr() {
   DebugDirectoryBegin = reinterpret_cast<const debug_directory *>(IntPtr);
   DebugDirectoryEnd = reinterpret_cast<const debug_directory *>(
       IntPtr + DataEntry->Size);
   // FIXME: Verify the section containing DebugDirectoryBegin has at least
   // DataEntry->Size bytes after DataEntry->RelativeVirtualAddress.
   return Error::success();
 }
 
+Error COFFObjectFile::initTlsTablePtr() {
+  // Get the RVA of the TLS table. Do nothing if it does not exist.
+  const data_directory *DataEntry = getDataDirectory(COFF::TLS_TABLE);
+  if (!DataEntry)
+    return Error::success();
+
+  // Do nothing if the RVA is NULL.
+  if (DataEntry->RelativeVirtualAddress == 0)
+    return Error::success();
+
+  uint64_t tableSize =
+      is64() ? sizeof(coff_tls_directory64) : sizeof(coff_tls_directory32);
+
+  // Check that the size is correct
+  if (DataEntry->Size != tableSize)
+    return errorCodeToError(object_error::parse_failed);
+
+  uintptr_t IntPtr = 0;
+  if (Error E = getRvaPtr(DataEntry->RelativeVirtualAddress, IntPtr))
+    return E;
+
+  if (is64())
+    TlsTable64 = reinterpret_cast<const coff_tls_directory64 *>(IntPtr);
+  else
+    TlsTable32 = reinterpret_cast<const coff_tls_directory32 *>(IntPtr);
+
+  return Error::success();
+}
+
 Error COFFObjectFile::initLoadConfigPtr() {
   // Get the RVA of the debug directory. Do nothing if it does not exist.
   const data_directory *DataEntry = getDataDirectory(COFF::LOAD_CONFIG_TABLE);
   if (!DataEntry)
     return Error::success();
 
   // Do nothing if the RVA is NULL.
   if (DataEntry->RelativeVirtualAddress == 0)

 COFFObjectFile::COFFObjectFile(MemoryBufferRef Object)
     : ObjectFile(Binary::ID_COFF, Object), COFFHeader(nullptr),
       COFFBigObjHeader(nullptr), PE32Header(nullptr), PE32PlusHeader(nullptr),
       DataDirectory(nullptr), SectionTable(nullptr), SymbolTable16(nullptr),
       SymbolTable32(nullptr), StringTable(nullptr), StringTableSize(0),
       ImportDirectory(nullptr), DelayImportDirectory(nullptr),
       NumberOfDelayImportDirectory(0), ExportDirectory(nullptr),
       BaseRelocHeader(nullptr), BaseRelocEnd(nullptr),
-      DebugDirectoryBegin(nullptr), DebugDirectoryEnd(nullptr) {}
+      DebugDirectoryBegin(nullptr), DebugDirectoryEnd(nullptr),
+      TlsTable32(nullptr), TlsTable64(nullptr) {}
 
 Error COFFObjectFile::initialize() {
   // Check that we at least have enough room for a header.
   std::error_code EC;
   if (!checkSize(Data, EC, sizeof(coff_file_header)))
     return errorCodeToError(EC);
 
   // The current location in the file where we are looking at.
Error COFFObjectFile::initialize() {
   // Initialize the pointer to the export table.
   if (Error E = initExportTablePtr())
     return E;
 
   // Initialize the pointer to the base relocation table.
   if (Error E = initBaseRelocPtr())
     return E;
 
-  // Initialize the pointer to the export table.
+  // Initialize the pointer to the debug directory.
   if (Error E = initDebugDirectoryPtr())
     return E;
 
+  // Initialize the pointer to the TLS table.
+  if (Error E = initTlsTablePtr())
+    return E;
+
   if (Error E = initLoadConfigPtr())
     return E;
 
   return Error::success();
 }
 
 basic_symbol_iterator COFFObjectFile::symbol_begin() const {
   DataRefImpl Ret;

llvm/tools/llvm-readobj/COFFDumper.cpp

public:
 
   void printNeededLibraries() override;
 
   void printCOFFImports() override;
   void printCOFFExports() override;
   void printCOFFDirectives() override;
   void printCOFFBaseReloc() override;
   void printCOFFDebugDirectory() override;
+  void printTlsTable() override;
   void printCOFFResources() override;
   void printCOFFLoadConfig() override;
   void printCodeViewDebugInfo() override;
   void mergeCodeViewTypes(llvm::codeview::MergingTypeTableBuilder &CVIDs,
                           llvm::codeview::MergingTypeTableBuilder &CVTypes,
                           llvm::codeview::GlobalTypeTableBuilder &GlobalCVIDs,
                           llvm::codeview::GlobalTypeTableBuilder &GlobalCVTypes,
                           bool GHash) override;
private:
   void printDataDirectory(uint32_t Index, const std::string &FieldName);
 
   void printDOSHeader(const dos_header *DH);
   template <class PEHeader> void printPEHeader(const PEHeader *Hdr);
   void printBaseOfDataField(const pe32_header *Hdr);
   void printBaseOfDataField(const pe32plus_header *Hdr);
   template <typename T>
   void printCOFFLoadConfig(const T *Conf, LoadConfigTables &Tables);
+  template <typename IntTy>
+  void printTlsTable(const coff_tls_directory<IntTy> *TlsTable);
   typedef void (*PrintExtraCB)(raw_ostream &, const uint8_t *);
   void printRVATable(uint64_t TableVA, uint64_t Count, uint64_t EntrySize,
                      PrintExtraCB PrintExtra = 0);
 
   void printCodeViewSymbolSection(StringRef SectionName, const SectionRef &Section);
   void printCodeViewTypeSection(StringRef SectionName, const SectionRef &Section);
   StringRef getTypeName(TypeIndex Ty);
   StringRef getFileNameForFileOffset(uint32_t FileOffset);
TypeTableCollection IpiTypes(IpiRecords);
     ListScope S(Writer, "MergedIDStream");
     TypeDumpVisitor TDV(TpiTypes, &Writer, opts::CodeViewSubsectionBytes);
     TDV.setIpiTypes(IpiTypes);
     if (Error Err = codeview::visitTypeStream(IpiTypes, TDV))
       reportError(std::move(Err), "<?>");
     Writer.flush();
   }
 }
+
+void COFFDumper::printTlsTable() {
+  if (Obj->is64())
+    printTlsTable(Obj->getTlsTable64());
+  else
+    printTlsTable(Obj->getTlsTable32());
+}
+
+template <typename IntTy>
+void COFFDumper::printTlsTable(const coff_tls_directory<IntTy> *TlsTable) {
+  if (!TlsTable)
+    return;
+
+  DictScope D(W, "TLSTable");
+  W.printHex("StartAddressOfRawData", TlsTable->StartAddressOfRawData);
+  W.printHex("EndAddressOfRawData", TlsTable->EndAddressOfRawData);
+  W.printHex("AddressOfIndex", TlsTable->AddressOfIndex);
+  W.printHex("AddressOfCallBacks", TlsTable->AddressOfCallBacks);
+  W.printHex("SizeOfZeroFill", TlsTable->SizeOfZeroFill);
+  W.printFlags("Characteristics", TlsTable->Characteristics,
+               makeArrayRef(ImageSectionCharacteristics),
+               COFF::SectionCharacteristics(COFF::IMAGE_SCN_ALIGN_MASK));
+}
 No newline at end of file

llvm/tools/llvm-readobj/ObjDumper.h

public:
   virtual void printArchSpecificInfo() { }
 
   // Only implemented for PE/COFF.
   virtual void printCOFFImports() { }
   virtual void printCOFFExports() { }
   virtual void printCOFFDirectives() { }
   virtual void printCOFFBaseReloc() { }
   virtual void printCOFFDebugDirectory() { }
+  virtual void printTlsTable() {}
   virtual void printCOFFResources() {}
   virtual void printCOFFLoadConfig() { }
   virtual void printCodeViewDebugInfo() { }
   virtual void
   mergeCodeViewTypes(llvm::codeview::MergingTypeTableBuilder &CVIDs,
                      llvm::codeview::MergingTypeTableBuilder &CVTypes,
                      llvm::codeview::GlobalTypeTableBuilder &GlobalCVIDs,
                      llvm::codeview::GlobalTypeTableBuilder &GlobalCVTypes,

llvm/tools/llvm-readobj/llvm-readobj.cpp

namespace opts {
   COFFBaseRelocs("coff-basereloc",
                  cl::desc("Display the PE/COFF .reloc section"));
 
   // --coff-debug-directory
   cl::opt<bool>
   COFFDebugDirectory("coff-debug-directory",
                      cl::desc("Display the PE/COFF debug directory"));
 
+  // --coff-tls-table
+  cl::opt<bool> COFFTlsTable("coff-tls-table",
+                             cl::desc("Display the PE/COFF TLS table"));
+
   // --coff-resources
   cl::opt<bool> COFFResources("coff-resources",
                               cl::desc("Display the PE/COFF .rsrc section"));
 
   // --coff-load-config
   cl::opt<bool>
   COFFLoadConfig("coff-load-config",
                  cl::desc("Display the PE/COFF load config"));
if (Obj.isCOFF()) {
     if (opts::COFFExports)
       Dumper->printCOFFExports();
     if (opts::COFFDirectives)
       Dumper->printCOFFDirectives();
     if (opts::COFFBaseRelocs)
       Dumper->printCOFFBaseReloc();
     if (opts::COFFDebugDirectory)
       Dumper->printCOFFDebugDirectory();
+    if (opts::COFFTlsTable)
+      Dumper->printTlsTable();
     if (opts::COFFResources)
       Dumper->printCOFFResources();
     if (opts::COFFLoadConfig)
       Dumper->printCOFFLoadConfig();
     if (opts::CGProfile)
       Dumper->printCGProfile();
     if (opts::Addrsig)
       Dumper->printAddrsig();