[ObjectYAML][DWARF] Let writeVariableSizedInteger() return Error.

[Higuoxing]

Summary

This patch helps change the return type of writeVariableSizedInteger() from void to Error.

[Higuoxing]

I think the error message "invalid integer write size" makes little sense here. If multiple sections are involved, it's hard to tell which section contains the invalid value. I'm still thinking about improving it.

[jhenderson]

llvm/lib/ObjectYAML/DWARFEmitter.cpp

57

This probably wants to be not_supported because strictly, there's nothing stopping us writing a 3/5/7 etc byte value.

133

Perhaps here and at similar points, it might be worth adding additional context to the message, to address your problem. There are examples of this in various places, such as llvm-readobj, I believe.

llvm/test/tools/yaml2obj/ELF/DWARF/debug-addr.yaml

597

perhaps entry -> list

llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml

411

Similarly, entry -> list here and below.

[Higuoxing]

Thanks for reviewing!

I added a helper function errorWithContext() which helps add additional context information to the error message.

llvm/lib/ObjectYAML/DWARFEmitter.cpp

133

I'm not sure if I'm doing it in the right way. I added one helper function errorWithContext() which helps add additional information to the error message.

[jhenderson]

llvm/lib/ObjectYAML/DWARFEmitter.cpp

39

This isn't too far off what I had in mind, but is probably more than what you actually need.

Take a look at ELFDumper.cpp in llvm-readobj. You'll see several instances where the function toString is used to convert an error into a string and then this string is used as part of a new error message. For example, in printRelocation you have the following:

Expected<std::pair<const typename ELFT::Sym *, std::string>> Target =
    this->dumper()->getRelocationTarget(SymTab, R);
if (!Target)
  this->reportUniqueWarning(createError(
      "unable to print relocation " + Twine(RelIndex) + " in section " +
      Twine(SecIndex) + ": " + toString(Target.takeError())));

When the Target contains an error, the toString method is used, combined with other context information into a new error message. Consequently, you'd explicitly build up your message with the lower-level error message and higher level things. For example:

if (Error Err = writeVariableSizedInteger(
        Descriptor.Address, Range.AddrSize, OS, DI.IsLittleEndian))
  return createStringError(errc::not_supported, "unable to write debug_aranges address: " + toString(std::move(Err)));

57–60

I'd slightly modify this error to include the size that has been requested.

[Higuoxing]

Address comments.

llvm/lib/ObjectYAML/DWARFEmitter.cpp

39

Thanks a lot! TIL, toString(Err) marks the error checked!

[jhenderson]

llvm/lib/ObjectYAML/DWARFEmitter.cpp

58

createStringError follows printf style formatting rules, so this can be:

return createStringError(errc::not_supported,
                         "invalid integer write size: %zu", Size);

and then no need to include the header.

134–135

Similarly, consider using %s:

return createStringError(errc::not_supported,
                         "unable to write debug_aranges address: %s",
                             toString(std::move(Err)).c_str());

Same goes below.

[Higuoxing]

Address comments.

[jhenderson]

LGTM!

llvm/lib/ObjectYAML/DWARFEmitter.cpp

 #include <cstddef>
 #include <cstdint>
 #include <memory>
 #include <string>
 #include <vector>
 
 using namespace llvm;
 
 template <typename T>
 static void writeInteger(T Integer, raw_ostream &OS, bool IsLittleEndian) {
   if (IsLittleEndian != sys::IsLittleEndianHost)
     sys::swapByteOrder(Integer);
   OS.write(reinterpret_cast<char *>(&Integer), sizeof(T));
 }
 
-static void writeVariableSizedInteger(uint64_t Integer, size_t Size,
+static Error writeVariableSizedInteger(uint64_t Integer, size_t Size,
                                        raw_ostream &OS, bool IsLittleEndian) {
   if (8 == Size)
     writeInteger((uint64_t)Integer, OS, IsLittleEndian);
   else if (4 == Size)
     writeInteger((uint32_t)Integer, OS, IsLittleEndian);
   else if (2 == Size)
     writeInteger((uint16_t)Integer, OS, IsLittleEndian);
   else if (1 == Size)
     writeInteger((uint8_t)Integer, OS, IsLittleEndian);
   else
-    assert(false && "Invalid integer write size.");
+    return createStringError(errc::invalid_argument,
+                             "invalid integer write size");
+
+  return Error::success();
 }
 
 static void ZeroFillBytes(raw_ostream &OS, size_t Size) {
   std::vector<uint8_t> FillData;
   FillData.insert(FillData.begin(), Size, 0);
   OS.write(reinterpret_cast<char *>(FillData.data()), Size);
 }
 
 static void writeInitialLength(const DWARFYAML::InitialLength &Length,
                                raw_ostream &OS, bool IsLittleEndian) {
   writeInteger((uint32_t)Length.TotalLength, OS, IsLittleEndian);
   if (Length.isDWARF64())
     writeInteger((uint64_t)Length.TotalLength64, OS, IsLittleEndian);
 }
 
 static void writeInitialLength(const dwarf::DwarfFormat Format,
                                const uint64_t Length, raw_ostream &OS,
                                bool IsLittleEndian) {
   bool IsDWARF64 = Format == dwarf::DWARF64;
   if (IsDWARF64)
-    writeVariableSizedInteger(dwarf::DW_LENGTH_DWARF64, 4, OS, IsLittleEndian);
-  writeVariableSizedInteger(Length, IsDWARF64 ? 8 : 4, OS, IsLittleEndian);
+    cantFail(writeVariableSizedInteger(dwarf::DW_LENGTH_DWARF64, 4, OS,
+                                       IsLittleEndian));
+  cantFail(
+      writeVariableSizedInteger(Length, IsDWARF64 ? 8 : 4, OS, IsLittleEndian));
 }
 
 Error DWARFYAML::emitDebugStr(raw_ostream &OS, const DWARFYAML::Data &DI) {
   for (auto Str : DI.DebugStrings) {
     OS.write(Str.data(), Str.size());
     OS.write('\0');
   }
 
for (auto Range : DI.ARanges) {
     writeInteger((uint8_t)Range.AddrSize, OS, DI.IsLittleEndian);
     writeInteger((uint8_t)Range.SegSize, OS, DI.IsLittleEndian);
 
     auto HeaderSize = OS.tell() - HeaderStart;
     auto FirstDescriptor = alignTo(HeaderSize, Range.AddrSize * 2);
     ZeroFillBytes(OS, FirstDescriptor - HeaderSize);
 
     for (auto Descriptor : Range.Descriptors) {
-      writeVariableSizedInteger(Descriptor.Address, Range.AddrSize, OS,
-                                DI.IsLittleEndian);
-      writeVariableSizedInteger(Descriptor.Length, Range.AddrSize, OS,
-                                DI.IsLittleEndian);
+      if (Error Err = writeVariableSizedInteger(
+              Descriptor.Address, Range.AddrSize, OS, DI.IsLittleEndian))
+        return Err;
+      cantFail(writeVariableSizedInteger(Descriptor.Length, Range.AddrSize, OS,
+                                         DI.IsLittleEndian));
     }
     ZeroFillBytes(OS, Range.AddrSize * 2);
   }
 
   return Error::success();
 }
 
 Error DWARFYAML::emitDebugRanges(raw_ostream &OS, const DWARFYAML::Data &DI) {
if (DebugRanges.Offset)
       ZeroFillBytes(OS, *DebugRanges.Offset - CurrOffset);
 
     uint8_t AddrSize;
     if (DebugRanges.AddrSize)
       AddrSize = *DebugRanges.AddrSize;
     else
       AddrSize = DI.Is64bit ? 8 : 4;
     for (auto Entry : DebugRanges.Entries) {
-      writeVariableSizedInteger(Entry.LowOffset, AddrSize, OS,
-                                DI.IsLittleEndian);
-      writeVariableSizedInteger(Entry.HighOffset, AddrSize, OS,
-                                DI.IsLittleEndian);
+      if (Error Err = writeVariableSizedInteger(Entry.LowOffset, AddrSize, OS,
+                                                DI.IsLittleEndian))
+        return Err;
+      cantFail(writeVariableSizedInteger(Entry.HighOffset, AddrSize, OS,
+                                         DI.IsLittleEndian));
     }
     ZeroFillBytes(OS, AddrSize * 2);
     ++EntryIndex;
   }
 
   return Error::success();
 }
 

 }
 
 Error DWARFYAML::emitDebugLine(raw_ostream &OS, const DWARFYAML::Data &DI) {
   for (const auto &LineTable : DI.DebugLines) {
     writeInitialLength(LineTable.Format, LineTable.Length, OS,
                        DI.IsLittleEndian);
     uint64_t SizeOfPrologueLength = LineTable.Format == dwarf::DWARF64 ? 8 : 4;
     writeInteger((uint16_t)LineTable.Version, OS, DI.IsLittleEndian);
-    writeVariableSizedInteger(LineTable.PrologueLength, SizeOfPrologueLength,
-                              OS, DI.IsLittleEndian);
+    cantFail(writeVariableSizedInteger(
+        LineTable.PrologueLength, SizeOfPrologueLength, OS, DI.IsLittleEndian));
     writeInteger((uint8_t)LineTable.MinInstLength, OS, DI.IsLittleEndian);
     if (LineTable.Version >= 4)
       writeInteger((uint8_t)LineTable.MaxOpsPerInst, OS, DI.IsLittleEndian);
     writeInteger((uint8_t)LineTable.DefaultIsStmt, OS, DI.IsLittleEndian);
     writeInteger((uint8_t)LineTable.LineBase, OS, DI.IsLittleEndian);
     writeInteger((uint8_t)LineTable.LineRange, OS, DI.IsLittleEndian);
     writeInteger((uint8_t)LineTable.OpcodeBase, OS, DI.IsLittleEndian);
 
for (const auto &LineTable : DI.DebugLines) {
     for (auto Op : LineTable.Opcodes) {
       writeInteger((uint8_t)Op.Opcode, OS, DI.IsLittleEndian);
       if (Op.Opcode == 0) {
         encodeULEB128(Op.ExtLen, OS);
         writeInteger((uint8_t)Op.SubOpcode, OS, DI.IsLittleEndian);
         switch (Op.SubOpcode) {
         case dwarf::DW_LNE_set_address:
         case dwarf::DW_LNE_set_discriminator:
-          writeVariableSizedInteger(Op.Data, DI.CompileUnits[0].AddrSize, OS,
-                                    DI.IsLittleEndian);
+          // TODO: Test this error.
+          if (Error Err = writeVariableSizedInteger(
+                  Op.Data, DI.CompileUnits[0].AddrSize, OS, DI.IsLittleEndian))
+            return Err;
           break;
         case dwarf::DW_LNE_define_file:
           emitFileEntry(OS, Op.FileEntry);
           break;
         case dwarf::DW_LNE_end_sequence:
           break;
         default:
           for (auto OpByte : Op.UnknownOpcodeData)
for (const AddrTableEntry &TableEntry : DI.DebugAddr) {
 
     writeInitialLength(TableEntry.Format, Length, OS, DI.IsLittleEndian);
     writeInteger((uint16_t)TableEntry.Version, OS, DI.IsLittleEndian);
     writeInteger((uint8_t)AddrSize, OS, DI.IsLittleEndian);
     writeInteger((uint8_t)TableEntry.SegSelectorSize, OS, DI.IsLittleEndian);
 
     for (const SegAddrPair &Pair : TableEntry.SegAddrPairs) {
       if (TableEntry.SegSelectorSize != 0)
-        writeVariableSizedInteger(Pair.Segment, TableEntry.SegSelectorSize, OS,
-                                  DI.IsLittleEndian);
+        if (Error Err = writeVariableSizedInteger(Pair.Segment,
+                                                  TableEntry.SegSelectorSize,
+                                                  OS, DI.IsLittleEndian))
+          return Err;
       if (AddrSize != 0)
-        writeVariableSizedInteger(Pair.Address, AddrSize, OS,
-                                  DI.IsLittleEndian);
+        if (Error Err = writeVariableSizedInteger(Pair.Address, AddrSize, OS,
+                                                  DI.IsLittleEndian))
+          return Err;
     }
   }
 
   return Error::success();
 }
 
 using EmitFuncType = Error (*)(raw_ostream &, const DWARFYAML::Data &);
 

llvm/test/tools/yaml2obj/ELF/DWARF/debug-addr.yaml

- Name:         .debug_addr
     Address:      0x2020      ## 0x00 by default.
     Offset:       0x50        ## 0x40 for the first section.
   - Name:         .sec        ## Linked by .debug_addr.
     Type:         SHT_STRTAB
 DWARF:
   debug_addr:
     - Version: 5
       Entries: []
+
+## l) Test that yaml2obj still generates a .debug_addr section if we assign an invalid value
+## to 'AddressSize' or 'SegmentSelectorSize' when the 'Entries' entry is empty.
+
+# RUN: yaml2obj --docnum=10 -DSIZENAME=AddressSize %s -o %t10.addr.o
+# RUN: llvm-readelf --hex-dump=.debug_addr %t10.addr.o | \
+# RUN:   FileCheck -DADDRSIZE=03 -DSEGSIZE=00 %s --check-prefix=ADDR-SEG-SIZE
+
+# RUN: yaml2obj --docnum=10 -DSIZENAME=SegmentSelectorSize %s -o %t10.seg.o
+# RUN: llvm-readelf --hex-dump=.debug_addr %t10.seg.o | \
+# RUN:   FileCheck -DADDRSIZE=08 -DSEGSIZE=03 %s --check-prefix=ADDR-SEG-SIZE
+
+#      ADDR-SEG-SIZE: Hex dump of section '.debug_addr':
+# ADDR-SEG-SIZE-NEXT: 0x00000000 04000000 0500[[ADDRSIZE]][[SEGSIZE]]
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+DWARF:
+  debug_addr:
+    - Version:      5
+      [[SIZENAME]]: 3
+      Entries:      []
+
+## m) Test that yaml2obj emits an error message if we try to assign an invalid value to
+## 'AddressSize' or 'SegmentSelectorSize' when the 'Entries' entry isn't empty.
+
+# RUN: not yaml2obj --docnum=11 -DSIZENAME=AddressSize %s 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=INVALID-SIZE
+
+# RUN: not yaml2obj --docnum=11 -DSIZENAME=SegmentSelectorSize %s 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=INVALID-SIZE
+
+# INVALID-SIZE: yaml2obj: error: invalid integer write size
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+DWARF:
+  debug_addr:
+    - Version:      5
+      [[SIZENAME]]: 3
+      Entries:
+        - Address: 0x1234

llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml

debug_aranges:
     - Length:   0x2c
       Version:  2
       CuOffset: 0
       AddrSize: 0x08
       SegSize:  0x00
       Descriptors:
         - Address: 0x0000000000001234
           Length:  0x20
+
+## h) Test that yaml2obj still generates a .debug_aranges section if we assign an invalid value
+## to 'AddrSize' when the 'Descriptors' entry is empty.
+
+# RUN: yaml2obj --docnum=8 %s -o %t8.o
+# RUN: llvm-readelf --hex-dump=.debug_aranges %t8.o | \
+# RUN:   FileCheck %s --check-prefix=ADDR-SIZE
+
+#      ADDR-SIZE: Hex dump of section '.debug_aranges':
+# ADDR-SIZE-NEXT: 0x00000000 2c000000 02000000 00000700 00000000
+##                                                 ^~ address_size (1-byte) 0x07
+# ADDR-SIZE-NEXT: 0x00000010 00000000 00000000 00000000
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+DWARF:
+  debug_aranges:
+    - Length:      0x2c
+      Version:     2
+      CuOffset:    0
+      AddrSize:    0x07 ## Invalid address_size.
+      SegSize:     0
+      Descriptors: []
+
+## i) Test that yaml2obj emits an error message if we try to assign an invalid value to
+## 'AddrSize' when the 'Descriptors' entry isn't empty.
+
+# RUN: not yaml2obj --docnum=9 %s 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=INVALID-SIZE
+
+# INVALID-SIZE: yaml2obj: error: invalid integer write size
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+DWARF:
+  debug_aranges:
+    - Length:      0x2c
+      Version:     2
+      CuOffset:    0
+      AddrSize:    0x07 ## Invalid address_size.
+      SegSize:     0
+      Descriptors:
+        - Address: 0x1234
+          Length:  0x1000

llvm/test/tools/yaml2obj/ELF/DWARF/debug-ranges.yaml

DWARF:
   debug_ranges:
     - Entries:
         - LowOffset:  0x01
           HighOffset: 0x02
     - Offset:   0x1F ## Must be greater than or equal to 0x20.
       Entries:
         - LowOffset:  0x01
           HighOffset: 0x02
+
+## j) Test that yaml2obj still generates a .debug_ranges section if we assign an invalid value
+## to 'AddrSize' when the 'Entries' entry is empty.
+
+# RUN: yaml2obj --docnum=10 %s -o %t10.o
+# RUN: llvm-readelf --hex-dump=.debug_ranges %t10.o | \
+# RUN:   FileCheck %s --check-prefix=ADDRSIZE
+
+#      ADDRSIZE: Hex dump of section '.debug_ranges':
+# ADDRSIZE-NEXT: 0x00000000 00000000 00000000 00000000 0000
+##                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ terminating entry (2*AddrSize=14-byte)
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+DWARF:
+  debug_ranges:
+    - AddrSize: 0x07
+      Entries:  []
+
+## k) Test that yaml2obj emits an error message if we try to assign an invalid value to
+## 'AddrSize' when the 'Entries' entry isn't empty.
+
+# RUN: not yaml2obj --docnum=11 %s 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=INVALID-SIZE
+
+# INVALID-SIZE: yaml2obj: error: invalid integer write size
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+DWARF:
+  debug_ranges:
+    - AddrSize: 0x07
+      Entries:
+        - LowOffset:  0x1234
+          HighOffset: 0x5678