Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[yaml2obj][XOFF] Update yaml2obj for XCOFF to create valid XCOFF files in more cases. #77620

Merged
merged 5 commits into from
Feb 9, 2024

Conversation

stephenpeckham
Copy link
Contributor

yaml2obj creates invalid object files even when the input was created by obj2yaml using a valid object file. On the other hand, yaml2obj is used to intentionally create invalid object files for testing purposes.

This update balances using specified input values when provided and computing file offsets and sizes if necessary.

yaml2obj creates invalid object files even when the input was
created by obj2yaml using a valid object file. On the other hand,
yaml2obj is used to intentionally create invalid object files for
testing purposes.

This update balances using specified input values when provided and
computing file offsets and sizes if necessary.
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 10, 2024

@llvm/pr-subscribers-llvm-binary-utilities

@llvm/pr-subscribers-objectyaml

Author: None (stephenpeckham)

Changes

yaml2obj creates invalid object files even when the input was created by obj2yaml using a valid object file. On the other hand, yaml2obj is used to intentionally create invalid object files for testing purposes.

This update balances using specified input values when provided and computing file offsets and sizes if necessary.


Patch is 25.62 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/77620.diff

11 Files Affected:

  • (modified) llvm/include/llvm/ObjectYAML/XCOFFYAML.h (+1-1)
  • (modified) llvm/lib/ObjectYAML/XCOFFEmitter.cpp (+116-85)
  • (modified) llvm/test/tools/llvm-objcopy/XCOFF/invalid-read.test (+4-2)
  • (modified) llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test (+1-1)
  • (modified) llvm/test/tools/llvm-objdump/XCOFF/section-headers.test (+1-1)
  • (modified) llvm/test/tools/llvm-readobj/XCOFF/file-header.test (+1-2)
  • (modified) llvm/test/tools/llvm-readobj/XCOFF/sections.test (+10-10)
  • (modified) llvm/test/tools/obj2yaml/XCOFF/aix.yaml (+2-2)
  • (modified) llvm/test/tools/obj2yaml/XCOFF/invalid-section.yaml (+2-1)
  • (modified) llvm/test/tools/yaml2obj/XCOFF/aux-hdr-defaults.yaml (+10-14)
  • (modified) llvm/test/tools/yaml2obj/XCOFF/basic-doc.yaml (+2-2)
diff --git a/llvm/include/llvm/ObjectYAML/XCOFFYAML.h b/llvm/include/llvm/ObjectYAML/XCOFFYAML.h
index f1e821fe5fa369..e4b6a5d2f8de17 100644
--- a/llvm/include/llvm/ObjectYAML/XCOFFYAML.h
+++ b/llvm/include/llvm/ObjectYAML/XCOFFYAML.h
@@ -26,7 +26,7 @@ struct FileHeader {
   int32_t TimeStamp;
   llvm::yaml::Hex64 SymbolTableOffset;
   int32_t NumberOfSymTableEntries;
-  uint16_t AuxHeaderSize;
+  std::optional<uint16_t> AuxHeaderSize;
   llvm::yaml::Hex16 Flags;
 };
 
diff --git a/llvm/lib/ObjectYAML/XCOFFEmitter.cpp b/llvm/lib/ObjectYAML/XCOFFEmitter.cpp
index ccf768c06aebfe..7f78aaf749dbcc 100644
--- a/llvm/lib/ObjectYAML/XCOFFEmitter.cpp
+++ b/llvm/lib/ObjectYAML/XCOFFEmitter.cpp
@@ -43,14 +43,14 @@ class XCOFFWriter {
   bool nameShouldBeInStringTable(StringRef SymbolName);
   bool initFileHeader(uint64_t CurrentOffset);
   void initAuxFileHeader();
-  bool initSectionHeader(uint64_t &CurrentOffset);
+  bool initSectionHeaders(uint64_t &CurrentOffset);
   bool initRelocations(uint64_t &CurrentOffset);
   bool initStringTable();
   bool assignAddressesAndIndices();
 
   void writeFileHeader();
   void writeAuxFileHeader();
-  void writeSectionHeader();
+  void writeSectionHeaders();
   bool writeSectionData();
   bool writeRelocations();
   bool writeSymbols();
@@ -98,11 +98,27 @@ bool XCOFFWriter::nameShouldBeInStringTable(StringRef SymbolName) {
 bool XCOFFWriter::initRelocations(uint64_t &CurrentOffset) {
   for (XCOFFYAML::Section &InitSection : InitSections) {
     if (!InitSection.Relocations.empty()) {
-      InitSection.NumberOfRelocations = InitSection.Relocations.size();
-      InitSection.FileOffsetToRelocations = CurrentOffset;
       uint64_t RelSize = Is64Bit ? XCOFF::RelocationSerializationSize64
                                  : XCOFF::RelocationSerializationSize32;
-      CurrentOffset += InitSection.NumberOfRelocations * RelSize;
+      const uint64_t UsedSize = RelSize * InitSection.Relocations.size();
+
+      // If NumberOfRelocations was specified, we use it, even if it's
+      // not consistent with the number of provided relocations
+      if (!InitSection.NumberOfRelocations)
+        InitSection.NumberOfRelocations = InitSection.Relocations.size();
+
+      // If the YAML file specified an offset to relocations, we use it.
+      if (InitSection.FileOffsetToRelocations) {
+        if (CurrentOffset > InitSection.FileOffsetToRelocations) {
+          ErrHandler("Specified FileOffsetToRelocations will overwrite"
+                     "existing data");
+          return false;
+        }
+        CurrentOffset = InitSection.FileOffsetToRelocations;
+      }
+      else
+        InitSection.FileOffsetToRelocations = CurrentOffset;
+      CurrentOffset += UsedSize;
       if (CurrentOffset > MaxRawDataSize) {
         ErrHandler("maximum object size of" + Twine(MaxRawDataSize) +
                    "exceeded when writing relocation data");
@@ -113,15 +129,10 @@ bool XCOFFWriter::initRelocations(uint64_t &CurrentOffset) {
   return true;
 }
 
-bool XCOFFWriter::initSectionHeader(uint64_t &CurrentOffset) {
-  uint64_t CurrentSecAddr = 0;
+bool XCOFFWriter::initSectionHeaders(uint64_t &CurrentOffset) {
+  uint64_t CurrentDataAddr = 0;
+  uint64_t CurrentTDataAddr = 0;
   for (uint16_t I = 0, E = InitSections.size(); I < E; ++I) {
-    if (CurrentOffset > MaxRawDataSize) {
-      ErrHandler("maximum object size of" + Twine(MaxRawDataSize) +
-                 "exceeded when writing section data");
-      return false;
-    }
-
     // Assign indices for sections.
     if (InitSections[I].SectionName.size() &&
         !SectionIndexMap[InitSections[I].SectionName]) {
@@ -134,23 +145,46 @@ bool XCOFFWriter::initSectionHeader(uint64_t &CurrentOffset) {
       }
     }
 
-    // Calculate the physical/virtual address. This field should contain 0 for
-    // all sections except the text, data and bss sections.
-    if (InitSections[I].Flags != XCOFF::STYP_TEXT &&
-        InitSections[I].Flags != XCOFF::STYP_DATA &&
-        InitSections[I].Flags != XCOFF::STYP_BSS)
-      InitSections[I].Address = 0;
-    else
-      InitSections[I].Address = CurrentSecAddr;
+    if (!InitSections[I].Size)
+      InitSections[I].Size = InitSections[I].SectionData.binary_size();
+
+    // We cannot compute section addresses in general. We only enforce
+    // the rule .data and .bss are consecutive, as are .tdata and .tbss.
+    switch(InitSections[I].Flags) {
+      case XCOFF::STYP_DATA:
+        CurrentDataAddr = InitSections[I].Address + InitSections[I].Size;
+        break;
+      case XCOFF::STYP_BSS:
+        if (!InitSections[I].Address)
+          InitSections[I].Address = CurrentDataAddr;
+        break;
+      case XCOFF::STYP_TDATA:
+        CurrentTDataAddr = InitSections[I].Address + InitSections[I].Size;
+        break;
+      case XCOFF::STYP_TBSS:
+        if (!InitSections[I].Address)
+          InitSections[I].Address = CurrentTDataAddr;
+        break;
+    }
 
-    // Calculate the FileOffsetToData and data size for sections.
     if (InitSections[I].SectionData.binary_size()) {
-      InitSections[I].FileOffsetToData = CurrentOffset;
+      if (InitSections[I].FileOffsetToData) {
+        // Use the providedFileOffsetToData.
+        if (CurrentOffset > InitSections[I].FileOffsetToData) {
+          ErrHandler("Specified FileOffsetToData will overwrite existing data");
+          return false;
+        }
+        CurrentOffset = InitSections[I].FileOffsetToData;
+      } else {
+         CurrentOffset = alignTo(CurrentOffset, DefaultSectionAlign);
+         InitSections[I].FileOffsetToData = CurrentOffset;
+      }
       CurrentOffset += InitSections[I].SectionData.binary_size();
-      // Ensure the offset is aligned to DefaultSectionAlign.
-      CurrentOffset = alignTo(CurrentOffset, DefaultSectionAlign);
-      InitSections[I].Size = CurrentOffset - InitSections[I].FileOffsetToData;
-      CurrentSecAddr += InitSections[I].Size;
+      if (CurrentOffset > MaxRawDataSize) {
+        ErrHandler("maximum object size of" + Twine(MaxRawDataSize) +
+                   "exceeded when writing section data");
+        return false;
+      }
     }
   }
   return initRelocations(CurrentOffset);
@@ -254,6 +288,13 @@ bool XCOFFWriter::initFileHeader(uint64_t CurrentOffset) {
 
   // Calculate SymbolTableOffset for the file header.
   if (InitFileHdr.NumberOfSymTableEntries) {
+    if (Obj.Header.SymbolTableOffset) {
+      if (CurrentOffset > Obj.Header.SymbolTableOffset) {
+        ErrHandler("symbol table offset overlaps existing data");
+        return false;
+      }
+      CurrentOffset = Obj.Header.SymbolTableOffset;
+    }
     InitFileHdr.SymbolTableOffset = CurrentOffset;
     CurrentOffset +=
         InitFileHdr.NumberOfSymTableEntries * XCOFF::SymbolTableEntrySize;
@@ -268,7 +309,8 @@ bool XCOFFWriter::initFileHeader(uint64_t CurrentOffset) {
 }
 
 void XCOFFWriter::initAuxFileHeader() {
-  InitAuxFileHdr = *Obj.AuxHeader;
+  if (Obj.AuxHeader)
+    InitAuxFileHdr = *Obj.AuxHeader;
   // In general, an object file might contain multiple sections of a given type,
   // but in a loadable module, there must be exactly one .text, .data, .bss, and
   // .loader section. A loadable object might also have one .tdata section and
@@ -320,30 +362,31 @@ void XCOFFWriter::initAuxFileHeader() {
 }
 
 bool XCOFFWriter::assignAddressesAndIndices() {
-  uint64_t FileHdrSize =
+  const uint64_t FileHdrSize =
       Is64Bit ? XCOFF::FileHeaderSize64 : XCOFF::FileHeaderSize32;
-  uint64_t AuxFileHdrSize = 0;
-  if (Obj.AuxHeader)
-    AuxFileHdrSize = Obj.Header.AuxHeaderSize
-                         ? Obj.Header.AuxHeaderSize
-                         : (Is64Bit ? XCOFF::AuxFileHeaderSize64
-                                    : XCOFF::AuxFileHeaderSize32);
-  uint64_t SecHdrSize =
+
+  // If AuxHeaderSize is specified in the YAML file, we construct
+  // an auxiliary header.
+  const uint64_t AuxFileHdrSize =
+      Obj.Header.AuxHeaderSize ? *Obj.Header.AuxHeaderSize :
+      !Obj.AuxHeader ? 0 :
+      (Is64Bit ? XCOFF::AuxFileHeaderSize64 : XCOFF::AuxFileHeaderSize32);
+  const uint64_t SecHdrSize =
       Is64Bit ? XCOFF::SectionHeaderSize64 : XCOFF::SectionHeaderSize32;
   uint64_t CurrentOffset =
       FileHdrSize + AuxFileHdrSize + InitSections.size() * SecHdrSize;
 
   // Calculate section header info.
-  if (!initSectionHeader(CurrentOffset))
+  if (!initSectionHeaders(CurrentOffset))
     return false;
-  InitFileHdr.AuxHeaderSize = AuxFileHdrSize;
 
   // Calculate file header info.
   if (!initFileHeader(CurrentOffset))
     return false;
+  InitFileHdr.AuxHeaderSize = AuxFileHdrSize;
 
   // Initialize the auxiliary file header.
-  if (Obj.AuxHeader)
+  if (AuxFileHdrSize)
     initAuxFileHeader();
 
   // Initialize the string table.
@@ -356,22 +399,18 @@ void XCOFFWriter::writeFileHeader() {
                                                 : InitFileHdr.NumberOfSections);
   W.write<int32_t>(Obj.Header.TimeStamp);
   if (Is64Bit) {
-    W.write<uint64_t>(Obj.Header.SymbolTableOffset
-                          ? Obj.Header.SymbolTableOffset
-                          : InitFileHdr.SymbolTableOffset);
-    W.write<uint16_t>(InitFileHdr.AuxHeaderSize);
+    W.write<uint64_t>(InitFileHdr.SymbolTableOffset);
+    W.write<uint16_t>(*InitFileHdr.AuxHeaderSize);
     W.write<uint16_t>(Obj.Header.Flags);
     W.write<int32_t>(Obj.Header.NumberOfSymTableEntries
                          ? Obj.Header.NumberOfSymTableEntries
                          : InitFileHdr.NumberOfSymTableEntries);
   } else {
-    W.write<uint32_t>(Obj.Header.SymbolTableOffset
-                          ? Obj.Header.SymbolTableOffset
-                          : InitFileHdr.SymbolTableOffset);
+    W.write<uint32_t>(InitFileHdr.SymbolTableOffset);
     W.write<int32_t>(Obj.Header.NumberOfSymTableEntries
                          ? Obj.Header.NumberOfSymTableEntries
                          : InitFileHdr.NumberOfSymTableEntries);
-    W.write<uint16_t>(InitFileHdr.AuxHeaderSize);
+    W.write<uint16_t>(*InitFileHdr.AuxHeaderSize);
     W.write<uint16_t>(Obj.Header.Flags);
   }
 }
@@ -391,6 +430,9 @@ void XCOFFWriter::writeAuxFileHeader() {
     W.write<uint32_t>(InitAuxFileHdr.EntryPointAddr.value_or(yaml::Hex64(0)));
     W.write<uint32_t>(InitAuxFileHdr.TextStartAddr.value_or(yaml::Hex64(0)));
     W.write<uint32_t>(InitAuxFileHdr.DataStartAddr.value_or(yaml::Hex64(0)));
+    // A short 32-bit auxiliary header ends here.
+    if (*InitFileHdr.AuxHeaderSize == XCOFF::AuxFileHeaderSizeShort)
+      return;
     W.write<uint32_t>(InitAuxFileHdr.TOCAnchorAddr.value_or(yaml::Hex64(0)));
   }
   W.write<uint16_t>(InitAuxFileHdr.SecNumOfEntryPoint.value_or(0));
@@ -432,50 +474,39 @@ void XCOFFWriter::writeAuxFileHeader() {
     W.write<uint16_t>(
         InitAuxFileHdr.Flag.value_or(yaml::Hex16(XCOFF::SHR_SYMTAB)));
     if (InitFileHdr.AuxHeaderSize > XCOFF::AuxFileHeaderSize64)
-      W.OS.write_zeros(InitFileHdr.AuxHeaderSize - XCOFF::AuxFileHeaderSize64);
-  } else if (InitFileHdr.AuxHeaderSize > XCOFF::AuxFileHeaderSize32) {
-    W.OS.write_zeros(InitFileHdr.AuxHeaderSize - XCOFF::AuxFileHeaderSize32);
+      W.OS.write_zeros(*InitFileHdr.AuxHeaderSize - XCOFF::AuxFileHeaderSize64);
+  } else {
+    if (InitFileHdr.AuxHeaderSize > XCOFF::AuxFileHeaderSize32)
+      W.OS.write_zeros(*InitFileHdr.AuxHeaderSize - XCOFF::AuxFileHeaderSize32);
   }
 }
 
-void XCOFFWriter::writeSectionHeader() {
+void XCOFFWriter::writeSectionHeaders() {
   for (uint16_t I = 0, E = Obj.Sections.size(); I < E; ++I) {
-    XCOFFYAML::Section YamlSec = Obj.Sections[I];
     XCOFFYAML::Section DerivedSec = InitSections[I];
-    writeName(YamlSec.SectionName, W);
-    // Virtual address is the same as physical address.
-    uint64_t SectionAddress =
-        YamlSec.Address ? YamlSec.Address : DerivedSec.Address;
+    writeName(DerivedSec.SectionName, W);
     if (Is64Bit) {
-      W.write<uint64_t>(SectionAddress); // Physical address
-      W.write<uint64_t>(SectionAddress); // Virtual address
-      W.write<uint64_t>(YamlSec.Size ? YamlSec.Size : DerivedSec.Size);
-      W.write<uint64_t>(YamlSec.FileOffsetToData ? YamlSec.FileOffsetToData
-                                                 : DerivedSec.FileOffsetToData);
-      W.write<uint64_t>(YamlSec.FileOffsetToRelocations
-                            ? YamlSec.FileOffsetToRelocations
-                            : DerivedSec.FileOffsetToRelocations);
-      W.write<uint64_t>(YamlSec.FileOffsetToLineNumbers);
-      W.write<uint32_t>(YamlSec.NumberOfRelocations
-                            ? YamlSec.NumberOfRelocations
-                            : DerivedSec.NumberOfRelocations);
-      W.write<uint32_t>(YamlSec.NumberOfLineNumbers);
+      // Virtual address is the same as physical address.
+      W.write<uint64_t>(DerivedSec.Address); // Physical address
+      W.write<uint64_t>(DerivedSec.Address); // Virtual address
+      W.write<uint64_t>(DerivedSec.Size);
+      W.write<uint64_t>(DerivedSec.FileOffsetToData);
+      W.write<uint64_t>(DerivedSec.FileOffsetToRelocations);
+      W.write<uint64_t>(DerivedSec.FileOffsetToLineNumbers);
+      W.write<uint32_t>(DerivedSec.NumberOfRelocations);
+      W.write<uint32_t>(DerivedSec.NumberOfLineNumbers);
       W.write<int32_t>(YamlSec.Flags);
       W.OS.write_zeros(4);
     } else {
-      W.write<uint32_t>(SectionAddress); // Physical address
-      W.write<uint32_t>(SectionAddress); // Virtual address
-      W.write<uint32_t>(YamlSec.Size ? YamlSec.Size : DerivedSec.Size);
-      W.write<uint32_t>(YamlSec.FileOffsetToData ? YamlSec.FileOffsetToData
-                                                 : DerivedSec.FileOffsetToData);
-      W.write<uint32_t>(YamlSec.FileOffsetToRelocations
-                            ? YamlSec.FileOffsetToRelocations
-                            : DerivedSec.FileOffsetToRelocations);
-      W.write<uint32_t>(YamlSec.FileOffsetToLineNumbers);
-      W.write<uint16_t>(YamlSec.NumberOfRelocations
-                            ? YamlSec.NumberOfRelocations
-                            : DerivedSec.NumberOfRelocations);
-      W.write<uint16_t>(YamlSec.NumberOfLineNumbers);
+      // Virtual address is the same as physical address.
+      W.write<uint32_t>(DerivedSec.Address); // Physical address
+      W.write<uint32_t>(DerivedSec.Address); // Virtual address
+      W.write<uint32_t>(DerivedSec.Size);
+      W.write<uint32_t>(DerivedSec.FileOffsetToData);
+      W.write<uint32_t>(DerivedSec.FileOffsetToRelocations);
+      W.write<uint32_t>(DerivedSec.FileOffsetToLineNumbers);
+      W.write<uint16_t>(DerivedSec.NumberOfRelocations);
+      W.write<uint16_t>(DerivedSec.NumberOfLineNumbers);
       W.write<int32_t>(YamlSec.Flags);
     }
   }
@@ -645,7 +676,7 @@ void XCOFFWriter::writeAuxSymbol(
 
 bool XCOFFWriter::writeSymbols() {
   int64_t PaddingSize =
-      (uint64_t)InitFileHdr.SymbolTableOffset - (W.OS.tell() - StartOffset);
+      InitFileHdr.SymbolTableOffset - (W.OS.tell() - StartOffset);
   if (PaddingSize < 0) {
     ErrHandler("redundant data was written before symbols");
     return false;
@@ -756,10 +787,10 @@ bool XCOFFWriter::writeXCOFF() {
     return false;
   StartOffset = W.OS.tell();
   writeFileHeader();
-  if (Obj.AuxHeader)
+  if (InitFileHdr.AuxHeaderSize.value_or(0))
     writeAuxFileHeader();
   if (!Obj.Sections.empty()) {
-    writeSectionHeader();
+    writeSectionHeaders();
     if (!writeSectionData())
       return false;
     if (!writeRelocations())
diff --git a/llvm/test/tools/llvm-objcopy/XCOFF/invalid-read.test b/llvm/test/tools/llvm-objcopy/XCOFF/invalid-read.test
index 1df63406a41361..96dcd72b672c24 100644
--- a/llvm/test/tools/llvm-objcopy/XCOFF/invalid-read.test
+++ b/llvm/test/tools/llvm-objcopy/XCOFF/invalid-read.test
@@ -5,7 +5,7 @@
 # RUN: yaml2obj %s --docnum=1 -o %t1
 # RUN: not llvm-objcopy %t1 %t1.out 2>&1 | FileCheck %s -DFILE=%t1 --check-prefix=ERROR1
 
-# ERROR1: error: '[[FILE]]': The end of the file was unexpectedly encountered: section data with offset 0x70 and size 0x4 goes past the end of the file
+# ERROR1: error: '[[FILE]]': The end of the file was unexpectedly encountered: section data with offset 0x70 and size 0x20 goes past the end of the file
 
 --- !XCOFF
 FileHeader:
@@ -13,6 +13,7 @@ FileHeader:
 Sections:
   - SectionData:      '00007400'
     FileOffsetToData: 0x70
+    Size: 0x20
 
 ## Failed to read relocations.
 # RUN: yaml2obj %s --docnum=2 -o %t2
@@ -35,12 +36,13 @@ Sections:
 # RUN: yaml2obj %s --docnum=3 -o %t3
 # RUN: not llvm-objcopy %t3 %t3.out 2>&1 | FileCheck %s -DFILE=%t3 --check-prefix=ERROR3
 
-# ERROR3: error: '[[FILE]]': The end of the file was unexpectedly encountered: symbol table with offset 0x15 and size 0x24 goes past the end of the file
+# ERROR3: error: '[[FILE]]': The end of the file was unexpectedly encountered: symbol table with offset 0x15 and size 0x36 goes past the end of the file
 
 --- !XCOFF
 FileHeader:
   MagicNumber:         0x01DF
   OffsetToSymbolTable: 0x15
+  EntriesInSymbolTable: 3
 Symbols:
   - Name:         foo
     AuxEntries:
diff --git a/llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test b/llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test
index 91354f5a951e0d..96cac6b849fd52 100644
--- a/llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test
+++ b/llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test
@@ -112,4 +112,4 @@ Symbols:
 # CHECK-NEXT:      70: 00 00 00 00  
 # CHECK-NEXT:        ...
 # CHECK-NEXT:      7c: 00 12 34 00  
-# CHECK-NEXT:      80: 00 00 00 00  
+# CHECK-NEXT:      80: 00 00 00     
diff --git a/llvm/test/tools/llvm-objdump/XCOFF/section-headers.test b/llvm/test/tools/llvm-objdump/XCOFF/section-headers.test
index e80d5f6a711b1a..1a110fb9642b85 100644
--- a/llvm/test/tools/llvm-objdump/XCOFF/section-headers.test
+++ b/llvm/test/tools/llvm-objdump/XCOFF/section-headers.test
@@ -10,7 +10,7 @@
 # CHECK-NEXT:   1 .data         00000004 00000000 DATA
 # CHECK-NEXT:   2 .bss          00000000 00000010 BSS
 # CHECK-NEXT:   3 .tdata        00000004 00000000 DATA
-# CHECK-NEXT:   4 .tbss         00000000 00000000 BSS
+# CHECK-NEXT:   4 .tbss         00000000 00000004 BSS
 # CHECK-NEXT:   5 .dwline       00000046 00000000 DEBUG
 # CHECK-NEXT:   6 .debug        00000046 00000000 DEBUG
 
diff --git a/llvm/test/tools/llvm-readobj/XCOFF/file-header.test b/llvm/test/tools/llvm-readobj/XCOFF/file-header.test
index 8cbd84749fd48c..2407aef7264849 100644
--- a/llvm/test/tools/llvm-readobj/XCOFF/file-header.test
+++ b/llvm/test/tools/llvm-readobj/XCOFF/file-header.test
@@ -23,7 +23,6 @@ FileHeader:
   CreationTime:         [[CREATTIME=1]]
   EntriesInSymbolTable: [[SYMBOLCOUNT=1]]
   NumberOfSections:     1
-  OffsetToSymbolTable:  0x3C
   AuxiliaryHeaderSize:  0
   Flags:                0x12
 Sections:
@@ -42,7 +41,7 @@ Symbols:
 # FILEHEADER64-NEXT:  Magic: 0x1F7
 # FILEHEADER64-NEXT:  NumberOfSections: 1
 # FILEHEADER64-NEXT:  TimeStamp: None (0x0)
-# FILEHEADER64-NEXT:  SymbolTableOffset: 0x3C
+# FILEHEADER64-NEXT:  SymbolTableOffset: 0x60
 # FILEHEADER64-NEXT:  SymbolTableEntries: 1
 # FILEHEADER64-NEXT:  OptionalHeaderSize: 0x0
 # FILEHEADER64-NEXT:  Flags: 0x12
diff --git a/llvm/test/tools/llvm-readobj/XCOFF/sections.test b/llvm/test/tools/llvm-readobj/XCOFF/sections.test
index be098939ce775a..36e85d6033652a 100644
--- a/llvm/test/tools/llvm-readobj/XCOFF/sections.test
+++ b/llvm/test/tools/llvm-readobj/XCOFF/sections.test
@@ -13,7 +13,7 @@
 # SEC32-NEXT:    Name: .text
 # SEC32-NEXT:    PhysicalAddress: 0x0
 # SEC32-NEXT:    VirtualAddress: 0x0
-# SEC32-NEXT:    Size: 0x4
+# SEC32-NEXT:    Size: 0x2
 # SEC32-NEXT:    RawDataOffset: 0x64
 # SEC32-NEXT:    RelocationPointer: 0x0
 # SEC32-NEXT:    LineNumberPointer: 0x0
@@ -24,11 +24,11 @@
 # SEC32-NEXT:  Section {
 # SEC32-NEXT:    Index: 2
 # SEC32-NEXT:    Name: .data
-# SEC32-NEXT:    PhysicalAddress: 0x4
-# SEC32-NEXT:    VirtualAddress: 0x4
-# SEC32-NEXT:    Size: 0x4
+# SEC32-NEXT:    PhysicalAddress: 0x0
+# SEC32-NEXT:    VirtualAddress: 0x0
+# SEC32-NEXT:    Size: 0x2
 # SEC32-NEXT:    RawDataOffset: 0x68
-# SEC32-NEXT:    RelocationPointer: 0x6C
+# SEC32-NEXT:    RelocationPointer: 0x6A
 # SEC32-NEXT:    LineNumberPointer: 0x0
 # SEC32-NEXT:    NumberOfRelocations: 1
 # SEC32-NEXT:    NumberOfLineNumbers: 0
@@ -65,7 +65,7 @@ Sections:
 # SEC64-...
[truncated]

Copy link

github-actions bot commented Jan 10, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think all the error cases have tests? Also you need to ensure that all your optional fields are tested (showing default behaviour when not specified).

llvm/lib/ObjectYAML/XCOFFEmitter.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjectYAML/XCOFFEmitter.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjectYAML/XCOFFEmitter.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjectYAML/XCOFFEmitter.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjectYAML/XCOFFEmitter.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjectYAML/XCOFFEmitter.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjectYAML/XCOFFEmitter.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjectYAML/XCOFFEmitter.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjectYAML/XCOFFEmitter.cpp Show resolved Hide resolved
Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also you need to ensure that all your optional fields are tested (showing default behaviour when not specified).

Have you addressed this comment?

llvm/lib/ObjectYAML/XCOFFEmitter.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjectYAML/XCOFFEmitter.cpp Outdated Show resolved Hide resolved
llvm/test/tools/yaml2obj/XCOFF/offset-check.yaml Outdated Show resolved Hide resolved
llvm/test/tools/yaml2obj/XCOFF/offset-check.yaml Outdated Show resolved Hide resolved
llvm/test/tools/yaml2obj/XCOFF/offset-check.yaml Outdated Show resolved Hide resolved
llvm/lib/ObjectYAML/XCOFFEmitter.cpp Show resolved Hide resolved
llvm/lib/ObjectYAML/XCOFFEmitter.cpp Show resolved Hide resolved
llvm/lib/ObjectYAML/XCOFFEmitter.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjectYAML/XCOFFEmitter.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjectYAML/XCOFFEmitter.cpp Outdated Show resolved Hide resolved
@stephenpeckham
Copy link
Contributor Author

Also you need to ensure that all your optional fields are tested (showing default behaviour when not specified).

Have you addressed this comment?
Adding tests for every optional field is beyond the scope of this PR.

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't attempted to trace things fully, but the AuxHeaderSize field is now a std::optional in this patch. Is that not something that is exposed to the user?

@stephenpeckham
Copy link
Contributor Author

I haven't attempted to trace things fully, but the AuxHeaderSize field is now a std::optional in this patch. Is that not something that is exposed to the user?

I thought I had a reason to make AuxHeaderSize optional , but I it looks like that specific change isn't necessary, at least for now. I'm not sure what you mean by exposing an optional field to the user. In any case, the XCOFF auxiliary header support needs some additional changes (in a future PR--the auxiliary fields can be read by yaml2obj, but are not currently printed by obj2yaml.

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't attempted to trace things fully, but the AuxHeaderSize field is now a std::optional in this patch. Is that not something that is exposed to the user?

I thought I had a reason to make AuxHeaderSize optional , but I it looks like that specific change isn't necessary, at least for now. I'm not sure what you mean by exposing an optional field to the user. In any case, the XCOFF auxiliary header support needs some additional changes (in a future PR--the auxiliary fields can be read by yaml2obj, but are not currently printed by obj2yaml.

I don't know why, but I had in my head that you'd changed code that might mean that people could omit a field that they couldn't before, in which case that should be tested. Looking again, I don't think there is, sorry for the fuss and LGTM.

@stephenpeckham stephenpeckham merged commit b5abaea into llvm:main Feb 9, 2024
4 checks passed
@stephenpeckham stephenpeckham deleted the peckham/yamlxcoff branch February 9, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants