[DWARFYAML] Add support for emitting custom operands for range list entry.

[Higuoxing]

Summary

This patch adds support for emitting custom operands for range list.

[Higuoxing]

Remove an unused header.

[Higuoxing]

Suppress clang-tidy warning.

[jhenderson]

In general terms, I think this is a good solution. Let's see what @grimar and @labath think.

llvm/include/llvm/ObjectYAML/DWARFYAML.h

188

enum class?

439–445

It might be nice to call these U8, U16 etc, to make them nice and concise.

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

655–661

Maybe we should just treat this case as having no operands automatically?

676

I think you can just delete "their" here.

[grimar]

Looks fine to me either.

llvm/lib/ObjectYAML/DWARFEmitter.cpp

478

Does it make sense to print the number of types and values in the message?
e.g.

"the number of operands (5) should match the number of their types (4)"

531

Do you need a cast here?

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

650

What about adding a test with mismatching types/values.
e.g

Types:    [ Unsigned8 ]
Values:   [ 0x1234567890abcdef ]

[labath]

In D84386#2168813, @jhenderson wrote:

In general terms, I think this is a good solution. Let's see what @grimar and @labath think.

Pretty much what I said already. This might be a nice way to test parsing of unknown/invalid DW_LLE constants, but there's only so many of those kinds of tests that we need, so it seems unnecessary.

Furthermore, once we're down in the invalid input land, it becomes very important to know how the bytes are laid out exactly in order to understand what the parser will do/has done. This becomes a (small) obstacle in that because I have to reason about the actual byte sequence that will be produced by this yaml -- I would prefer if I just had the bytes directly.

But if you do want to have this, I'm not going to stop you...

llvm/include/llvm/ObjectYAML/DWARFYAML.h

439–445

+1

[Higuoxing]

llvm/lib/ObjectYAML/DWARFEmitter.cpp

531

I'm not sure about it here. Seems that some build bots are not happy if we do not add a cast here since we have covered all enumeration values in this switch.

[jhenderson]

In D84386#2168864, @labath wrote:

In D84386#2168813, @jhenderson wrote:

In general terms, I think this is a good solution. Let's see what @grimar and @labath think.

Pretty much what I said already. This might be a nice way to test parsing of unknown/invalid DW_LLE constants, but there's only so many of those kinds of tests that we need, so it seems unnecessary.

Furthermore, once we're down in the invalid input land, it becomes very important to know how the bytes are laid out exactly in order to understand what the parser will do/has done. This becomes a (small) obstacle in that because I have to reason about the actual byte sequence that will be produced by this yaml -- I would prefer if I just had the bytes directly.

But if you do want to have this, I'm not going to stop you...

@Higuoxing, could you put up a patch that uses the hex approach @labath is suggesting instead of this approach, please? We can then better compare the complexity etc of the two approaches.

[Higuoxing]

In D84386#2168871, @jhenderson wrote:

In D84386#2168864, @labath wrote:

In D84386#2168813, @jhenderson wrote:

In general terms, I think this is a good solution. Let's see what @grimar and @labath think.

Pretty much what I said already. This might be a nice way to test parsing of unknown/invalid DW_LLE constants, but there's only so many of those kinds of tests that we need, so it seems unnecessary.

Furthermore, once we're down in the invalid input land, it becomes very important to know how the bytes are laid out exactly in order to understand what the parser will do/has done. This becomes a (small) obstacle in that because I have to reason about the actual byte sequence that will be produced by this yaml -- I would prefer if I just had the bytes directly.

But if you do want to have this, I'm not going to stop you...

@Higuoxing, could you put up a patch that uses the hex approach @labath is suggesting instead of this approach, please? We can then better compare the complexity etc of the two approaches.

Yeah, no problem!

[labath]

In D84386#2168872, @Higuoxing wrote:

In D84386#2168871, @jhenderson wrote:

In D84386#2168864, @labath wrote:

In D84386#2168813, @jhenderson wrote:

In general terms, I think this is a good solution. Let's see what @grimar and @labath think.

Pretty much what I said already. This might be a nice way to test parsing of unknown/invalid DW_LLE constants, but there's only so many of those kinds of tests that we need, so it seems unnecessary.

Furthermore, once we're down in the invalid input land, it becomes very important to know how the bytes are laid out exactly in order to understand what the parser will do/has done. This becomes a (small) obstacle in that because I have to reason about the actual byte sequence that will be produced by this yaml -- I would prefer if I just had the bytes directly.

But if you do want to have this, I'm not going to stop you...

@Higuoxing, could you put up a patch that uses the hex approach @labath is suggesting instead of this approach, please? We can then better compare the complexity etc of the two approaches.

Yeah, no problem!

The biggest advantage I see in that approach is that it would enable obj2yaml to describe (chunks of) invalid debug_rnglists sections. So the way I would do this is to approach this from the other end -- look at how obj2yaml does its parsing. The point where it would need to "give up" because of a parsing error is a good place to insert a Contents attribute with a hex string. Here, one good such place seems to be the level which describes a single range list -- if one range list doesn't parse, we just insert a binary blob and then try again with the next list.

llvm/lib/ObjectYAML/DWARFEmitter.cpp

531

This looks like the "default label in a fully covered switch" warning, except that technically the switch is *not* fully covered without the default case. C++ is weird...

[Higuoxing]

In D84386#2168871, @jhenderson wrote:

In D84386#2168864, @labath wrote:

In D84386#2168813, @jhenderson wrote:

In general terms, I think this is a good solution. Let's see what @grimar and @labath think.

Pretty much what I said already. This might be a nice way to test parsing of unknown/invalid DW_LLE constants, but there's only so many of those kinds of tests that we need, so it seems unnecessary.

Furthermore, once we're down in the invalid input land, it becomes very important to know how the bytes are laid out exactly in order to understand what the parser will do/has done. This becomes a (small) obstacle in that because I have to reason about the actual byte sequence that will be produced by this yaml -- I would prefer if I just had the bytes directly.

But if you do want to have this, I'm not going to stop you...

@Higuoxing, could you put up a patch that uses the hex approach @labath is suggesting instead of this approach, please? We can then better compare the complexity etc of the two approaches.

I've implemented one in D84618 :-) Any thoughts?

[jhenderson]

I think the other version is better, so you can probably abandon this change.

[Higuoxing]

This patch is abandoned in favor of D84618.

llvm/include/llvm/ObjectYAML/DWARFYAML.h

 struct StringOffsetsTable {
   dwarf::DwarfFormat Format;
   Optional<yaml::Hex64> Length;
   yaml::Hex16 Version;
   yaml::Hex16 Padding;
   std::vector<yaml::Hex64> Offsets;
 };
 
+enum ListEntryOperandTypes {
+  Unsigned8,
+  Unsigned16,
+  Unsigned32,
+  Unsigned64,
+  ULEB128,
+  SLEB128,
+};
+
 struct RnglistEntry {
   dwarf::RnglistEntries Operator;
+  Optional<std::vector<ListEntryOperandTypes>> Types;
   std::vector<yaml::Hex64> Values;
 };
 
 template <typename EntryType> struct ListEntries {
   std::vector<EntryType> Entries;
 };
 
 template <typename EntryType> struct ListTable {

 LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::DWARFYAML::SegAddrPair)
 LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::DWARFYAML::AddrTableEntry)
 LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::DWARFYAML::StringOffsetsTable)
 LLVM_YAML_IS_SEQUENCE_VECTOR(
     llvm::DWARFYAML::ListTable<DWARFYAML::RnglistEntry>)
 LLVM_YAML_IS_SEQUENCE_VECTOR(
     llvm::DWARFYAML::ListEntries<DWARFYAML::RnglistEntry>)
 LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::DWARFYAML::RnglistEntry)
+LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::DWARFYAML::ListEntryOperandTypes)
 
 namespace llvm {
 namespace yaml {
 
 template <> struct MappingTraits<DWARFYAML::Data> {
   static void mapping(IO &IO, DWARFYAML::Data &DWARF);
 };
 

 template <> struct ScalarEnumerationTraits<dwarf::Constants> {
   static void enumeration(IO &io, dwarf::Constants &value) {
     io.enumCase(value, "DW_CHILDREN_no", dwarf::DW_CHILDREN_no);
     io.enumCase(value, "DW_CHILDREN_yes", dwarf::DW_CHILDREN_yes);
     io.enumFallback<Hex16>(value);
   }
 };
 
+template <> struct ScalarEnumerationTraits<DWARFYAML::ListEntryOperandTypes> {
+  static void enumeration(IO &io, DWARFYAML::ListEntryOperandTypes &value) {
+    io.enumCase(value, "Unsigned8",
+                DWARFYAML::ListEntryOperandTypes::Unsigned8);
+    io.enumCase(value, "Unsigned16",
+                DWARFYAML::ListEntryOperandTypes::Unsigned16);
+    io.enumCase(value, "Unsigned32",
+                DWARFYAML::ListEntryOperandTypes::Unsigned32);
+    io.enumCase(value, "Unsigned64",
+                DWARFYAML::ListEntryOperandTypes::Unsigned64);
+    io.enumCase(value, "ULEB128", DWARFYAML::ListEntryOperandTypes::ULEB128);
+    io.enumCase(value, "SLEB128", DWARFYAML::ListEntryOperandTypes::SLEB128);
+  }
+};
+
 #define HANDLE_DW_RLE(unused, name)                                            \
   io.enumCase(value, "DW_RLE_" #name, dwarf::DW_RLE_##name);
 
 template <> struct ScalarEnumerationTraits<dwarf::RnglistEntries> {
   static void enumeration(IO &io, dwarf::RnglistEntries &value) {
 #include "llvm/BinaryFormat/Dwarf.def"
+    io.enumFallback<Hex8>(value);
   }
 };
 
 } // end namespace yaml
 } // end namespace llvm
 
 #endif // LLVM_OBJECTYAML_DWARFYAML_H

llvm/lib/ObjectYAML/DWARFEmitter.cpp

if (Error Err = writeVariableSizedInteger(Addr, AddrSize, OS, IsLittleEndian))
     return createStringError(errc::invalid_argument,
                              "unable to write address for the operator %s: %s",
                              EncodingName.str().c_str(),
                              toString(std::move(Err)).c_str());
 
   return Error::success();
 }
 
+static Error
+writeCustomListEntryOperands(ArrayRef<DWARFYAML::ListEntryOperandTypes> Types,
+                             ArrayRef<yaml::Hex64> Values, raw_ostream &OS,
+                             bool IsLittleEndian) {
+  if (Types.size() != Values.size())
+    return createStringError(
+        errc::invalid_argument,
+        "the number of operands should match the number of their types");
+
+  for (size_t I = 0; I < Types.size(); ++I) {
+    switch (Types[I]) {
+    case DWARFYAML::ListEntryOperandTypes::Unsigned8:
+      writeInteger((uint8_t)Values[I], OS, IsLittleEndian);
+      break;
+    case DWARFYAML::ListEntryOperandTypes::Unsigned16:
+      writeInteger((uint16_t)Values[I], OS, IsLittleEndian);
+      break;
+    case DWARFYAML::ListEntryOperandTypes::Unsigned32:
+      writeInteger((uint32_t)Values[I], OS, IsLittleEndian);
+      break;
+    case DWARFYAML::ListEntryOperandTypes::Unsigned64:
+      writeInteger((uint64_t)Values[I], OS, IsLittleEndian);
+      break;
+    case DWARFYAML::ListEntryOperandTypes::ULEB128:
+      encodeULEB128((uint64_t)Values[I], OS);
+      break;
+    case DWARFYAML::ListEntryOperandTypes::SLEB128:
+      encodeSLEB128((int64_t)Values[I], OS);
+      break;
+    }
+  }
+
+  return Error::success();
+}
+
 static Expected<uint64_t> writeListEntry(raw_ostream &OS,
                                          const DWARFYAML::RnglistEntry &Entry,
                                          uint8_t AddrSize,
                                          bool IsLittleEndian) {
   uint64_t BeginOffset = OS.tell();
   writeInteger((uint8_t)Entry.Operator, OS, IsLittleEndian);
 
   StringRef EncodingName = dwarf::RangeListEncodingString(Entry.Operator);
 
   auto CheckOperands = [&](uint64_t ExpectedOperands) -> Error {
     return checkListEntryOperands(EncodingName, Entry.Values, ExpectedOperands);
   };
 
   auto WriteAddress = [&](uint64_t Addr) -> Error {
     return writeListEntryAddress(EncodingName, OS, Addr, AddrSize,
                                  IsLittleEndian);
   };
 
-  switch (Entry.Operator) {
+  if (Entry.Types) {
+    if (Error Err = writeCustomListEntryOperands(*Entry.Types, Entry.Values, OS,
+                                                 IsLittleEndian))
+      return std::move(Err);
+    return OS.tell() - BeginOffset;
+  }
+
+  switch ((uint8_t)Entry.Operator) {
   case dwarf::DW_RLE_end_of_list:
     if (Error Err = CheckOperands(0))
       return std::move(Err);
     break;
   case dwarf::DW_RLE_base_addressx:
     if (Error Err = CheckOperands(1))
       return std::move(Err);
     encodeULEB128(Entry.Values[0], OS);
case dwarf::DW_RLE_start_end:
     break;
   case dwarf::DW_RLE_start_length:
     if (Error Err = CheckOperands(2))
       return std::move(Err);
     if (Error Err = WriteAddress(Entry.Values[0]))
       return std::move(Err);
     encodeULEB128(Entry.Values[1], OS);
     break;
+  default:
+    return createStringError(errc::invalid_argument,
+                             "unrecognized operator: 0x" +
+                                 utohexstr(Entry.Operator));
   }
 
   return OS.tell() - BeginOffset;
 }
 
 template <typename EntryType>
 Error writeDWARFLists(raw_ostream &OS,
                       ArrayRef<DWARFYAML::ListTable<EntryType>> Tables,

llvm/lib/ObjectYAML/DWARFYAML.cpp

void MappingTraits<DWARFYAML::StringOffsetsTable>::mapping(
   IO.mapOptional("Version", StrOffsetsTable.Version, 5);
   IO.mapOptional("Padding", StrOffsetsTable.Padding, 0);
   IO.mapOptional("Offsets", StrOffsetsTable.Offsets);
 }
 
 void MappingTraits<DWARFYAML::RnglistEntry>::mapping(
     IO &IO, DWARFYAML::RnglistEntry &RnglistEntry) {
   IO.mapRequired("Operator", RnglistEntry.Operator);
+  IO.mapOptional("Types", RnglistEntry.Types);
   IO.mapOptional("Values", RnglistEntry.Values);
 }
 
 template <typename EntryType>
 void MappingTraits<DWARFYAML::ListEntries<EntryType>>::mapping(
     IO &IO, DWARFYAML::ListEntries<EntryType> &ListEntries) {
   IO.mapOptional("Entries", ListEntries.Entries);
 }

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

 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 DWARF:
   debug_rnglists: []
+
+## s) Test that yaml2obj is able to emit custom operands for range list entries.
+
+# RUN: yaml2obj --docnum=17 %s -o %t17.o
+# RUN: llvm-readelf --hex-dump=.debug_rnglists %t17.o | \
+# RUN:   FileCheck %s --check-prefix=CUSTOM-OPERANDS
+
+#      CUSTOM-OPERANDS: Hex dump of section '.debug_rnglists':
+# CUSTOM-OPERANDS-NEXT: 0x00000000 21000000 05000800 01000000 04000000 !...............
+##                                 ^-------                            unit_length (4-byte)
+##                                          ^---                       version (2-byte)
+##                                              ^-                     address_size (1-byte)
+##                                                ^-                   segment_selector_size (1-byte)
+##                                                   ^-------          offset_entry_count (4-byte)
+##                                                            ^------- offsets[0] (4-byte)
+# CUSTOM-OPERANDS-NEXT: 0x00000010 00013412 78563412 efcdab90 78563412 ..4.xV4.....xV4.
+##                                 ^-                                  DW_RLE_end_of_list
+##                                   ^-                                operands[0] (uint8_t)
+##                                     ^---                            operands[1] (uint16_t)
+##                                          ^-------                   operands[2] (uint32_t)
+##                                                   ^---------------- operands[3] (uint64_t)
+# CUSTOM-OPERANDS-NEXT: 0x00000020 b4247dff 12                         .$}..
+##                                 ^---                                operands[4] (ULEB128)
+##                                     ^-                              operands[5] (SLEB128) -3
+##                                       ^-                            operator: 0xff
+##                                          ^-                         operands[0] (uint8_t)
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+DWARF:
+  debug_rnglists:
+    - Lists:
+        - Entries:
+            - Operator: DW_RLE_end_of_list
+              Types:    [ Unsigned8, Unsigned16, Unsigned32, Unsigned64,         ULEB128, SLEB128 ]
+              Values:   [ 0x01,      0x1234,     0x12345678, 0x1234567890abcdef, 0x1234,  0xfffffffffffffffd ] ## 0xfffffffffffffffd is -3
+            - Operator: 0xff
+              Types:    [ Unsigned8 ]
+              Values:   [ 0x12 ]
+
+## t) Test that yaml2obj emits an error message if we specify an unrecognized operator but don't
+## specify the type(s) for its operand(s).
+
+# RUN: not yaml2obj --docnum=18 %s 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=UNRECOGNIZED
+
+# UNRECOGNIZED: yaml2obj: error: unrecognized operator: 0xFF
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+DWARF:
+  debug_rnglists:
+    - Lists:
+        - Entries:
+            - Operator: 0xff
+
+## u) Test that yaml2obj emits an error message if the number of operands doesn't match
+## the number of their types.
+
+# RUN: not yaml2obj --docnum=19 %s 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=UNMATCH-TYPES
+
+# UNMATCH-TYPES: yaml2obj: error: the number of operands should match the number of their types
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+DWARF:
+  debug_rnglists:
+    - Lists:
+        - Entries:
+            - Operator: DW_RLE_end_of_list
+              Types:    [ ULEB128 ]
+              Values:   [ 0x01, 0x02 ]