[yaml2obj] Add support for writing the long symbol name.

[Esme]

Summary

This patch, as a follow-up of D95505, adds support for writing the long symbol name by implementing StringTable.

[Higuoxing]

llvm/test/tools/yaml2obj/XCOFF/long-symbol-name.yaml

2

Nit: Usually, we use '##' for comments that are not RUN lines.

19

1. Can you add a symbol with short symbol name to illustrate that long names are stored in the string table and short names are stored in the .debug section?

2. I think it's hard to tell whether a symbol name is from the string table or the .debug section in your test since llvm-readobj only prints the symbol name without mentioning where it's from. Are there any options for llvm-readobj that is able to tell us where the symbol name is from?

[jhenderson]

llvm/lib/ObjectYAML/XCOFFEmitter.cpp

277–281

Am I right in thinking that we don't yet have XCOFF64 support in yaml2obj by the point of this patch? If not, we should probably just change this to W.write<int32_t>(0); and add the Is64Bit check later (optionally add an assert(!Is64Bit && "64-bit format not yet implemented") as a reminder.

Additionally, the comment suggests that the string can be in one of two places, but as far as this code is concerned, it is only written in one place (the string table), right? I think talking about both here can be confusing, without making it clear which is actually used.

llvm/test/tools/yaml2obj/XCOFF/long-symbol-name.yaml

19

+1 to @Higuoxing's comments. In particular, for testing we need to be sure that we don't accidentally start writing short names in the string table. I don't have a clear suggestion on how to do this, but one option would be to ensure we have the ability to dump the raw string table itself, and then look at the contents (similar to how you could do llvm-readobj -p=.strtab foo.elf to get a list of strings).

[Esme]

Addressed comments except dumping the raw string table.
It seems llvm-readobj can't dump the raw string table for XCOFF, I have to find another tool to do this.

[Esme]

Rebase.

[jhenderson]

In D103455#2805561, @Esme wrote:

Addressed comments except dumping the raw string table.
It seems llvm-readobj can't dump the raw string table for XCOFF, I have to find another tool to do this.

I think it would make a lot of sense to add string table dumping to llvm-readobj. It's generic enough that you could probably get away with implementing it for multiple file formats too (e.g. for ELF it would dump all SHT_STRTAB sections), but initially, you can just implement it for XCOFF. I'd use a switch name as simple as --string-table or possibly --string-tables.

llvm/test/tools/yaml2obj/XCOFF/long-symbol-name.yaml

31–34

You can remove the excessive whitespace between the key and value.

I'd also make the names just long enough/not quite long enough to need the string table, to test the boundary conditions of the name length check.

[Esme]

In D103455#2807169, @jhenderson wrote:

In D103455#2805561, @Esme wrote:

Addressed comments except dumping the raw string table.
It seems llvm-readobj can't dump the raw string table for XCOFF, I have to find another tool to do this.

I think it would make a lot of sense to add string table dumping to llvm-readobj. It's generic enough that you could probably get away with implementing it for multiple file formats too (e.g. for ELF it would dump all SHT_STRTAB sections), but initially, you can just implement it for XCOFF. I'd use a switch name as simple as --string-table or possibly --string-tables.

Thanks, it's a good idea to implement the option for llvm-readobj. I will do this in the future.
As far as I know, there is a system tool dump under AIX (https://www.ibm.com/docs/en/aix/7.1?topic=d-dump-command), which can print the raw string table.

bash-5.0$ yaml2obj llvm/test/tools/yaml2obj/XCOFF/long-symbol-name.yaml -o test.o
bash-5.0$ dump -cv test.o 

test.o:

                        ***String Table Information***
         Offset     Name

             4   .longname

However, the test in this patch can't dump the string table yet unless llvm-readobj --string-table is implemented.

[shchenz]

Yes, it is better to implement string table dump for XCOFF, then we should not rely on the system tool to verify this change.

I think the correctness should also be guaranteed by AIX system tool dump.

So LGTM. Thanks for doing this.

FYI. I can get the correct string table and symbol table for long/short name symbols on AIX:

$cat t.yaml 
--- !XCOFF
FileHeader:
  MagicNumber: 0x1DF
Symbols:
  - Name: .symname
  - Name: .longname
  - Name: .longname3456
  - Name: .longname345

$dump -X32 -cv test.o

test.o:

                        ***String Table Information***
         Offset     Name

             4   .longname
            14   .longname3456
            28   .longname345

$dump -X32 -t test.o

[0]     m   0x00000000         0     0    0x00         0x0000     .symname
[1]     m   0x00000000         0     0    0x00         0x0000     .longname
[2]     m   0x00000000         0     0    0x00         0x0000     .longname3456
[3]     m   0x00000000         0     0    0x00         0x0000     .longname345

llvm/lib/ObjectYAML/XCOFFEmitter.cpp

78

Maybe we need to explicitly describe that in the issue summary this is only for 32-bit XCOFF?

[jhenderson]

LGTM too. Please remember to update the test once the string table printing has landed.

llvm/test/tools/yaml2obj/XCOFF/long-symbol-name.yaml

20

Not for this change, but perhaps a future llvm-readobj enhancement. For ELF, the sh_name/st_name field printing includes the raw value as well as the interpreted name value, with the raw value representing the offset into the string table. This sometimes might be helpful for debugging, and I think could be implemented for XCOFF too.

llvm/lib/ObjectYAML/XCOFFEmitter.cpp

 //===- yaml2xcoff - Convert YAML to a xcoff object file -------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// The xcoff component of yaml2obj.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/BinaryFormat/XCOFF.h"
+#include "llvm/MC/StringTableBuilder.h"
 #include "llvm/Object/XCOFFObjectFile.h"
 #include "llvm/ObjectYAML/ObjectYAML.h"
 #include "llvm/ObjectYAML/yaml2obj.h"
 #include "llvm/Support/EndianStream.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/LEB128.h"
 
 using namespace llvm;
 
 namespace {
 
 constexpr unsigned DefaultSectionAlign = 4;
 constexpr int16_t MaxSectionIndex = INT16_MAX;
 constexpr uint32_t MaxRawDataSize = UINT32_MAX;
 
 class XCOFFWriter {
 public:
   XCOFFWriter(XCOFFYAML::Object &Obj, raw_ostream &OS, yaml::ErrorHandler EH)
-      : Obj(Obj), W(OS, support::big), ErrHandler(EH) {
+      : Obj(Obj), W(OS, support::big), ErrHandler(EH),
+        Strings(StringTableBuilder::XCOFF) {
     Is64Bit = Obj.Header.Magic == (llvm::yaml::Hex16)XCOFF::XCOFF64;
   }
   bool writeXCOFF();
 
 private:
+  bool nameShouldBeInStringTable(StringRef SymbolName);
   bool initFileHeader(uint64_t CurrentOffset);
   bool initSectionHeader(uint64_t &CurrentOffset);
   bool initRelocations(uint64_t &CurrentOffset);
   bool assignAddressesAndIndices();
   void writeFileHeader();
   void writeSectionHeader();
   bool writeSectionData();
   bool writeRelocations();
   bool writeSymbols();
 
   XCOFFYAML::Object &Obj;
   bool Is64Bit = false;
   support::endian::Writer W;
   yaml::ErrorHandler ErrHandler;
+  StringTableBuilder Strings;
   uint64_t StartOffset;
   // Map the section name to its corrresponding section index.
   DenseMap<StringRef, int16_t> SectionIndexMap = {
       {StringRef("N_DEBUG"), XCOFF::N_DEBUG},
       {StringRef("N_ABS"), XCOFF::N_ABS},
       {StringRef("N_UNDEF"), XCOFF::N_UNDEF}};
   XCOFFYAML::FileHeader InitFileHdr = Obj.Header;
   std::vector<XCOFFYAML::Section> InitSections = Obj.Sections;
 };
 
 static void writeName(StringRef StrName, support::endian::Writer W) {
   char Name[XCOFF::NameSize];
   memset(Name, 0, XCOFF::NameSize);
   char SrcName[] = "";
   memcpy(Name, StrName.size() ? StrName.data() : SrcName, StrName.size());
   ArrayRef<char> NameRef(Name, XCOFF::NameSize);
   W.write(NameRef);
 }
 
+bool XCOFFWriter::nameShouldBeInStringTable(StringRef SymbolName) {
+  return SymbolName.size() > XCOFF::NameSize;
+}
+
 bool XCOFFWriter::initRelocations(uint64_t &CurrentOffset) {
   for (uint16_t I = 0, E = InitSections.size(); I < E; ++I) {
     if (!InitSections[I].Relocations.empty()) {
       InitSections[I].NumberOfRelocations = InitSections[I].Relocations.size();
       InitSections[I].FileOffsetToRelocations = CurrentOffset;
       CurrentOffset += InitSections[I].NumberOfRelocations *
                        XCOFF::RelocationSerializationSize32;
       if (CurrentOffset > MaxRawDataSize) {
bool XCOFFWriter::initFileHeader(uint64_t CurrentOffset) {
   // The default format of the object file is XCOFF32.
   InitFileHdr.Magic = XCOFF::XCOFF32;
   InitFileHdr.NumberOfSections = Obj.Sections.size();
   InitFileHdr.NumberOfSymTableEntries = Obj.Symbols.size();
 
   for (const XCOFFYAML::Symbol &YamlSym : Obj.Symbols) {
     // Add the number of auxiliary symbols to the total number.
     InitFileHdr.NumberOfSymTableEntries += YamlSym.NumberOfAuxEntries;
+    if (nameShouldBeInStringTable(YamlSym.SymbolName))
+      Strings.add(YamlSym.SymbolName);
   }
+  // Finalize the string table.
+  Strings.finalize();
 
   // Calculate SymbolTableOffset for the file header.
   if (InitFileHdr.NumberOfSymTableEntries) {
     InitFileHdr.SymbolTableOffset = CurrentOffset;
     CurrentOffset +=
         InitFileHdr.NumberOfSymTableEntries * XCOFF::SymbolTableEntrySize;
     if (CurrentOffset > MaxRawDataSize) {
       ErrHandler("maximum object size of" + Twine(MaxRawDataSize) +
                  "exceeded when writing symbols");
       return false;
     }
   }
   // TODO: Calculate FileOffsetToLineNumbers when line number supported.
   return true;
 }
 
 bool XCOFFWriter::assignAddressesAndIndices() {
+  Strings.clear();
   uint64_t CurrentOffset =
       XCOFF::FileHeaderSize32 /* TODO: + auxiliaryHeaderSize() */ +
       InitSections.size() * XCOFF::SectionHeaderSize32;
 
   // Calculate section header info.
   if (!initSectionHeader(CurrentOffset))
     return false;
   // Calculate file header info.
int64_t PaddingSize =
       (uint64_t)InitFileHdr.SymbolTableOffset - (W.OS.tell() - StartOffset);
   if (PaddingSize < 0) {
     ErrHandler("redundant data was written before symbols");
     return false;
   }
   if (PaddingSize > 0)
     W.OS.write_zeros(PaddingSize);
   for (const XCOFFYAML::Symbol &YamlSym : Obj.Symbols) {
+    if (nameShouldBeInStringTable(YamlSym.SymbolName)) {
+      // For XCOFF32: A value of 0 indicates that the symbol name is in the
+      // string table.
+      W.write<int32_t>(0);
+      W.write<uint32_t>(Strings.getOffset(YamlSym.SymbolName));
+    } else {
       writeName(YamlSym.SymbolName, W);
+    }
     W.write<uint32_t>(YamlSym.Value);
     W.write<int16_t>(
         YamlSym.SectionName.size() ? SectionIndexMap[YamlSym.SectionName] : 0);
     W.write<uint16_t>(YamlSym.Type);
     W.write<uint8_t>(YamlSym.StorageClass);
     W.write<uint8_t>(YamlSym.NumberOfAuxEntries);
 
     // Now output the auxiliary entry.
bool XCOFFWriter::writeXCOFF() {
   writeFileHeader();
   if (!Obj.Sections.empty()) {
     writeSectionHeader();
     if (!writeSectionData())
       return false;
     if (!writeRelocations())
       return false;
   }
-  if (!Obj.Symbols.empty())
-    return writeSymbols();
+  if (!Obj.Symbols.empty()) {
+    if (!writeSymbols())
+      return false;
+    // Write the string table.
+    Strings.write(W.OS);
+  }
   return true;
 }
 
 } // end anonymous namespace
 
 namespace llvm {
 namespace yaml {
 
 bool yaml2xcoff(XCOFFYAML::Object &Doc, raw_ostream &Out, ErrorHandler EH) {
   XCOFFWriter Writer(Doc, Out, EH);
   return Writer.writeXCOFF();
 }
 
 } // namespace yaml
 } // namespace llvm

llvm/test/tools/yaml2obj/XCOFF/long-symbol-name.yaml

+## Test that the string table works well for long symbol names.
+## TODO: Dump the raw string table and check the contents.
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readobj --symbols %t | FileCheck %s
+
+# CHECK:      AddressSize: 32bit
+# CHECK-NEXT: Symbols [
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Index: 0
+# CHECK-NEXT:     Name: .symname
+# CHECK-NEXT:     Value: 0x0
+# CHECK-NEXT:     Section: N_UNDEF
+# CHECK-NEXT:     Type: 0x0
+# CHECK-NEXT:     StorageClass: C_NULL (0x0)
+# CHECK-NEXT:     NumberOfAuxEntries: 0
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Index: 1
+# CHECK-NEXT:     Name: .longname
+# CHECK-NEXT:     Value: 0x0
+# CHECK-NEXT:     Section: N_UNDEF
+# CHECK-NEXT:     Type: 0x0
+# CHECK-NEXT:     StorageClass: C_NULL (0x0)
+# CHECK-NEXT:     NumberOfAuxEntries: 0
+# CHECK-NEXT:   }
+# CHECK-NEXT: ]
+
+--- !XCOFF
+FileHeader:
+  MagicNumber: 0x1DF
+Symbols:
+  - Name: .symname
+  - Name: .longname