[DWARFYAML] Make the opcode_base and the standard_opcode_lengths fields optional.

[Higuoxing]

Summary

This patch makes the opcode_base and the standard_opcode_lengths fields
of the line table optional. When both of them are not specified,
yaml2obj emits them according to the line table's version.

[Higuoxing]

Update.

[jhenderson]

To summarise my inline suggestions, I think the behaviour should be:

1. Neither field specified: values are derived from the version. If the version is unsupported, return max(? - maybe v4 instead, to stop test behaviour changing in the future unexpectedly) supported version values.
2. Both fields specified: do as requested.
3. Just opcode_base specified, same as 1, except truncate the standard opcode lengths vector to the right length. If 0, use an empty standard opcode lengths vector.
4. Just standard opcode lengths specified: use length of specified lengths vector to derive opcode_base.

llvm/lib/ObjectYAML/DWARFEmitter.cpp

556–560

You don't use the Version parameter, it looks like? I think you should - define the values based on the specified version, perhaps defaulting to v4 if the version is unknown (actually, I'd probably default to whatever the latest is, but that's academic, as v4 and v5 have the same standard opcodes).

557–558

I actually think the logic should be to truncate whatever the default vector is, rather than specifying a zero-filled vector.

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

716

729

[Higuoxing]

• Make getStandardOpcodeLengths() return the standard_opcode_lengths according to the latest DWARF spec by default.
• Add one test case where the opcode_base is greater than 13.
• Address some inline comments.

llvm/lib/ObjectYAML/DWARFEmitter.cpp

556–560

I like the latter approach. When I generate the DWARFv2 line table using clang -gdwarf-2 a.c, the opcode_base field of the generated line table is 13 as well. I think making getStandardOpcodeLengths return the standard_opcode_lengths according to the latest DWARF spec is fine here.

[jhenderson]

llvm/lib/ObjectYAML/DWARFEmitter.cpp

557–564

I think you either misunderstood my comment or something else. You're still not using the Version parameter, and I think you should use that in determining what the standard opcode lengths should be if otherwise unspecified. See my previous out-of-line commentary, in particular point 1.

[Higuoxing]

Use Version to determine the default opcode_base and standard_opcode_lengths fields.

[jhenderson]

llvm/lib/ObjectYAML/DWARFEmitter.cpp

561–562

This probably deserves a comment saying something like "DWARF v2 uses the same first 9 standard opcodes as v3-5."

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

785–786

I suggest you do this for each of the other versions too, assuming the test is straightforward to write using FileCheck and yaml2obj -D options.

[Higuoxing]

• Test the default opcode_base field and the standard_opcode_lengths array of DWARFv2-DWARFv4 (DWARFv5 isn't supported yet).
• Add missing comment.

[jhenderson]

LGTM.

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

706

I'm rather surprised that works, if I'm honest!

[Higuoxing]

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

706

Yeah, the preprocessor of yaml2obj simply replaces strings that quoted by [[]].

llvm/include/llvm/ObjectYAML/DWARFYAML.h

struct LineTable {
   Optional<uint64_t> Length;
   uint16_t Version;
   Optional<uint64_t> PrologueLength;
   uint8_t MinInstLength;
   uint8_t MaxOpsPerInst;
   uint8_t DefaultIsStmt;
   uint8_t LineBase;
   uint8_t LineRange;
-  uint8_t OpcodeBase;
-  std::vector<uint8_t> StandardOpcodeLengths;
+  Optional<uint8_t> OpcodeBase;
+  Optional<std::vector<uint8_t>> StandardOpcodeLengths;
   std::vector<StringRef> IncludeDirs;
   std::vector<File> Files;
   std::vector<LineTableOpcode> Opcodes;
 };
 
 struct SegAddrPair {
   yaml::Hex64 Segment;
   yaml::Hex64 Address;

llvm/lib/ObjectYAML/DWARFEmitter.cpp

if (Op.Opcode == 0) {
     default:
       for (auto OpData : Op.StandardOpcodeData) {
         encodeULEB128(OpData, OS);
       }
     }
   }
 }
 
+static std::vector<uint8_t>
+getStandardOpcodeLengths(uint16_t Version, Optional<uint8_t> OpcodeBase) {
+  if (OpcodeBase)
+    return std::vector<uint8_t>(*OpcodeBase > 0 ? *OpcodeBase - 1 : 0);
+  return std::vector<uint8_t>{0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1};
+}
+
 Error DWARFYAML::emitDebugLine(raw_ostream &OS, const DWARFYAML::Data &DI) {
   for (const DWARFYAML::LineTable &LineTable : DI.DebugLines) {
     // Buffer holds the bytes following the header_length (or prologue_length in
     // DWARFv2) field to the end of the line number program itself.
     std::string Buffer;
     raw_string_ostream BufferOS(Buffer);
 
     writeInteger(LineTable.MinInstLength, BufferOS, DI.IsLittleEndian);
     // TODO: Add support for emitting DWARFv5 line table.
     if (LineTable.Version >= 4)
       writeInteger(LineTable.MaxOpsPerInst, BufferOS, DI.IsLittleEndian);
     writeInteger(LineTable.DefaultIsStmt, BufferOS, DI.IsLittleEndian);
     writeInteger(LineTable.LineBase, BufferOS, DI.IsLittleEndian);
     writeInteger(LineTable.LineRange, BufferOS, DI.IsLittleEndian);
-    writeInteger(LineTable.OpcodeBase, BufferOS, DI.IsLittleEndian);
 
-    for (uint8_t OpcodeLength : LineTable.StandardOpcodeLengths)
+    std::vector<uint8_t> StandardOpcodeLengths =
+        LineTable.StandardOpcodeLengths.getValueOr(
+            getStandardOpcodeLengths(LineTable.Version, LineTable.OpcodeBase));
+    uint8_t OpcodeBase = LineTable.OpcodeBase
+                             ? *LineTable.OpcodeBase
+                             : StandardOpcodeLengths.size() + 1;
+    writeInteger(OpcodeBase, BufferOS, DI.IsLittleEndian);
+    for (uint8_t OpcodeLength : StandardOpcodeLengths)
       writeInteger(OpcodeLength, BufferOS, DI.IsLittleEndian);
 
     for (StringRef IncludeDir : LineTable.IncludeDirs) {
       BufferOS.write(IncludeDir.data(), IncludeDir.size());
       BufferOS.write('\0');
     }
     BufferOS.write('\0');
 
     for (const DWARFYAML::File &File : LineTable.Files)
       emitFileEntry(BufferOS, File);
     BufferOS.write('\0');
 
     uint64_t HeaderLength =
         LineTable.PrologueLength ? *LineTable.PrologueLength : Buffer.size();
 
     for (const DWARFYAML::LineTableOpcode &Op : LineTable.Opcodes)
-      writeLineTableOpcode(Op, LineTable.OpcodeBase, DI.Is64BitAddrSize ? 8 : 4,
-                           BufferOS, DI.IsLittleEndian);
+      writeLineTableOpcode(Op, OpcodeBase, DI.Is64BitAddrSize ? 8 : 4, BufferOS,
+                           DI.IsLittleEndian);
 
     uint64_t Length;
     if (LineTable.Length) {
       Length = *LineTable.Length;
     } else {
       Length = 2; // sizeof(version)
       Length +=
           (LineTable.Format == dwarf::DWARF64 ? 8 : 4); // sizeof(header_length)

llvm/lib/ObjectYAML/DWARFYAML.cpp

void MappingTraits<DWARFYAML::LineTable>::mapping(
   IO.mapRequired("Version", LineTable.Version);
   IO.mapOptional("PrologueLength", LineTable.PrologueLength);
   IO.mapRequired("MinInstLength", LineTable.MinInstLength);
   if(LineTable.Version >= 4)
     IO.mapRequired("MaxOpsPerInst", LineTable.MaxOpsPerInst);
   IO.mapRequired("DefaultIsStmt", LineTable.DefaultIsStmt);
   IO.mapRequired("LineBase", LineTable.LineBase);
   IO.mapRequired("LineRange", LineTable.LineRange);
-  IO.mapRequired("OpcodeBase", LineTable.OpcodeBase);
-  IO.mapRequired("StandardOpcodeLengths", LineTable.StandardOpcodeLengths);
+  IO.mapOptional("OpcodeBase", LineTable.OpcodeBase);
+  IO.mapOptional("StandardOpcodeLengths", LineTable.StandardOpcodeLengths);
   IO.mapOptional("IncludeDirs", LineTable.IncludeDirs);
   IO.mapOptional("Files", LineTable.Files);
   IO.mapOptional("Opcodes", LineTable.Opcodes);
 }
 
 void MappingTraits<DWARFYAML::SegAddrPair>::mapping(
     IO &IO, DWARFYAML::SegAddrPair &SegAddrPair) {
   IO.mapOptional("Segment", SegAddrPair.Segment, 0);

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

- Version:               4
             Name:    abcd
             DirIdx:  0x1234
             ModTime: 0x1234
             Length:  0x1234
         - Opcode:    DW_LNS_extended_op
           ## Specify the ExtLen field.
           ExtLen:    0x1234
           SubOpcode: DW_LNE_end_sequence
+
+## m) Test how yaml2obj generates the opcode_base and the
+## standard_opcode_lengths fields.
+
+## Both the opcode_base and the standard_opcode_lengths fields are not
+## specified.
+
+# RUN: yaml2obj --docnum=13 %s -o %t13.o
+# RUN: llvm-readelf --hex-dump=.debug_line %t13.o | \
+# RUN:   FileCheck %s --check-prefix=OPCODEBASE1
+
+#      OPCODEBASE1: Hex dump of section '.debug_line':
+# OPCODEBASE1-NEXT: 0x00000000 1a000000 04001400 00000100 0100010d ................
+##                                                              ^- opcode_base (13)
+# OPCODEBASE1-NEXT: 0x00000010 00010101 01000000 01000001 0000     ..............
+##                             ^-                                  standard_opcode_lengths[DW_LNS_copy] = 0
+##                               ^-                                standard_opcode_lengths[DW_LNS_advance_pc] = 1
+##                                 ^-                              standard_opcode_lengths[DW_LNS_advance_line] = 1
+##                                   ^-                            standard_opcode_lengths[DW_LNS_set_file] = 1
+##                                      ^-                         standard_opcode_lengths[DW_LNS_set_column] = 1
+##                                        ^-                       standard_opcode_lengths[DW_LNS_negate_stmt] = 0
+##                                          ^-                     standard_opcode_lengths[DW_LNS_set_basic_block] = 0
+##                                            ^-                   standard_opcode_lengths[DW_LNS_const_add_pc] = 0
+##                                               ^-                standard_opcode_lengths[DW_LNS_fixed_advance_pc] = 1
+##                                                 ^-              standard_opcode_lengths[DW_LNS_set_prologue_end] = 0
+##                                                   ^-            standard_opcode_lengths[DW_LNS_set_epilogue_begin] = 0
+##                                                     ^-          standard_opcode_lengths[DW_LNS_set_isa] = 1
+##                                                        ^---     terminators for include_directories and file_names
+
+--- !ELF
+FileHeader:
+  Class: ELFCLASS64
+  Data:  ELFDATA2LSB
+  Type:  ET_EXEC
+DWARF:
+  debug_line:
+    - Version:               4
+      MinInstLength:         1
+      MaxOpsPerInst:         0
+      DefaultIsStmt:         1
+      LineBase:              0
+      LineRange:             1
+      OpcodeBase:            [[OPCODEBASE=<none>]]
+      StandardOpcodeLengths: [[STANDARDOPCODELENGTHS=<none>]]
+
+## Specify the opcode_base field (opcode_base!=0).
+
+# RUN: yaml2obj --docnum=13 -DOPCODEBASE=4 %s -o %t14.o
+# RUN: llvm-readelf --hex-dump=.debug_line %t14.o | \
+# RUN:   FileCheck %s --check-prefix=OPCODEBASE2
+
+#      OPCODEBASE2: Hex dump of section '.debug_line':
+# OPCODEBASE2-NEXT: 0x00000000 11000000 04000b00 00000100 01000104 ................
+##                                                              ^- opcode_base (4)
+# OPCODEBASE2-NEXT: 0x00000010 00000000 00                         .....
+##                             ^-----                              standard_opcode_lengths (3-byte)
+##                                   ^----                         terminators for include_directories and file_names
+
+## Specify the opcode_base field (opcode_base=0).
+
+# RUN: yaml2obj --docnum=13 -DOPCODEBASE=0 %s -o %t15.o
+# RUN: llvm-readelf --hex-dump=.debug_line %t15.o | \
+# RUN:   FileCheck %s --check-prefix=OPCODEBASE3
+
+#      OPCODEBASE3: Hex dump of section '.debug_line':
+# OPCODEBASE3-NEXT: 0x00000000 0e000000 04000800 00000100 01000100 ................
+##                                                              ^- opcode_base (0)
+# OPCODEBASE3-NEXT: 0x00000010 0000                                ..
+##                             ^---                                terminators for include_directories and file_names
+
+## Specify the standard_opcode_lengths field.
+
+# RUN: yaml2obj --docnum=13 -DSTANDARDOPCODELENGTHS=[0,1,0] %s -o %t16.o
+# RUN: llvm-readelf --hex-dump=.debug_line %t16.o | \
+# RUN:   FileCheck %s --check-prefix=OPCODEBASE4
+
+#      OPCODEBASE4: Hex dump of section '.debug_line':
+# OPCODEBASE4-NEXT: 0x00000000 11000000 04000b00 00000100 01000104 ................
+##                                                              ^- opcode_base (4)
+# OPCODEBASE4-NEXT: 0x00000010 00010000 00                         .....
+##                             ^-----                              standard_opcode_lengths (3-byte)
+##                                   ^----                         terminators for include_directories and file_names
+
+## Specify both the opcode_base and the standard_opcode_lengths fields.
+
+# RUN: yaml2obj --docnum=13 -DOPCODEBASE=2 -DSTANDARDOPCODELENGTHS=[0,1,0] %s -o %t17.o
+# RUN: llvm-readelf --hex-dump=.debug_line %t17.o | \
+# RUN:   FileCheck %s --check-prefix=OPCODEBASE5
+
+#      OPCODEBASE5: Hex dump of section '.debug_line':
+# OPCODEBASE5-NEXT: 0x00000000 11000000 04000b00 00000100 01000102 ................
+##                                                              ^- opcode_base (2)
+# OPCODEBASE5-NEXT: 0x00000010 00010000 00                         .....
+##                             ^-----                              standard_opcode_lengths (3-byte)
+##                                   ^----                         terminators for include_directories and file_names

llvm/tools/obj2yaml/dwarf2yaml.cpp

if (auto StmtOffset =
       DebugLines.MinInstLength = LineData.getU8(&Offset);
       if (DebugLines.Version >= 4)
         DebugLines.MaxOpsPerInst = LineData.getU8(&Offset);
       DebugLines.DefaultIsStmt = LineData.getU8(&Offset);
       DebugLines.LineBase = LineData.getU8(&Offset);
       DebugLines.LineRange = LineData.getU8(&Offset);
       DebugLines.OpcodeBase = LineData.getU8(&Offset);
 
-      DebugLines.StandardOpcodeLengths.reserve(DebugLines.OpcodeBase - 1);
+      DebugLines.StandardOpcodeLengths.emplace();
       for (uint8_t i = 1; i < DebugLines.OpcodeBase; ++i)
-        DebugLines.StandardOpcodeLengths.push_back(LineData.getU8(&Offset));
+        DebugLines.StandardOpcodeLengths->push_back(LineData.getU8(&Offset));
 
       while (Offset < EndPrologue) {
         StringRef Dir = LineData.getCStr(&Offset);
         if (!Dir.empty())
           DebugLines.IncludeDirs.push_back(Dir);
         else
           break;
       }
if (auto StmtOffset =
             dumpFileEntry(LineData, Offset, NewOp.FileEntry);
             break;
           case dwarf::DW_LNE_end_sequence:
             break;
           default:
             while (Offset < StartExt + *NewOp.ExtLen)
               NewOp.UnknownOpcodeData.push_back(LineData.getU8(&Offset));
           }
-        } else if (NewOp.Opcode < DebugLines.OpcodeBase) {
+        } else if (NewOp.Opcode < *DebugLines.OpcodeBase) {
           switch (NewOp.Opcode) {
           case dwarf::DW_LNS_copy:
           case dwarf::DW_LNS_negate_stmt:
           case dwarf::DW_LNS_set_basic_block:
           case dwarf::DW_LNS_const_add_pc:
           case dwarf::DW_LNS_set_prologue_end:
           case dwarf::DW_LNS_set_epilogue_begin:
             break;
if (auto StmtOffset =
             break;
 
           case dwarf::DW_LNS_fixed_advance_pc:
             NewOp.Data = LineData.getU16(&Offset);
             break;
 
           default:
             for (uint8_t i = 0;
-                 i < DebugLines.StandardOpcodeLengths[NewOp.Opcode - 1]; ++i)
+                 i <
+                 DebugLines.StandardOpcodeLengths.getValue()[NewOp.Opcode - 1];
+                 ++i)
               NewOp.StandardOpcodeData.push_back(LineData.getULEB128(&Offset));
           }
         }
         DebugLines.Opcodes.push_back(NewOp);
       }
       Y.DebugLines.push_back(DebugLines);
     }
   }
 }