DWARFDebugLine.cpp: Format unknown line number standard opcodes

[hubert.reinterpretcast]

Summary

This patch implements formatv() formatting for dwarf::LineNumberOps and makes use of it for the llvm-dwarfdump --debug-line dump.

Previously, unknown line number standard opcodes would lead to undefined behaviour. The code would attempt to format the data pointer of an empty StringRef (a null pointer) using %s. According to the description for format(), use of that interface carries the "risk of printf". Passing a null pointer in place of an array to a C library function results in undefined behaviour.

[jhenderson]

I think a better solution is to print something more meaningful. For example, "DW_LNS_unknown_<value>" or something similar. You can see examples of that with the DW_AT_unknown_* printing in debug-abbrev.s. I don't think that will be significantly more complex and will provide more user-friendly output. See also my inline comment about where to put the fix.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

697

If you put the fix in LNStandardString instead, this line will also print more user-friendly output in verbose mode (a test should be added for it, if you do).

[hubert.reinterpretcast]

DWARFDebugLine.cpp: Format unknown line number standard opcodes

[jhenderson]

I'd still really like to see a test that tests the other code path that tries to print DW_LNS_* (see the inline comment). The obvious thing to me to do would be to update the unit test "ParserPrintsStandardOpcodesWhenRequested", but it would require changes to the DwarfGenerator to create a custom prologue. If you don't want to do this, that's okay, as the code path is already untested.

[hubert.reinterpretcast]

In D72369#1809613, @jhenderson wrote:

I think a better solution is to print something more meaningful. For example, "DW_LNS_unknown_<value>" or something similar. You can see examples of that with the DW_AT_unknown_* printing in debug-abbrev.s. I don't think that will be significantly more complex and will provide more user-friendly output.

Thanks for the pointer. I have set up "DW_LNS_unknown_<value>" printing based on the DW_AT_unknown_* printing.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

697

I looked into this, and LNStandardString is part of a family of functions where returning an empty StringRef is the documented interface for unknown values.

operator<< is okay with empty StringRefs, and the opcode value is already printed. Changing the output here does not really fit with this patch.

[hubert.reinterpretcast]

In D72369#1812104, @jhenderson wrote:

I'd still really like to see a test that tests the other code path that tries to print DW_LNS_* (see the inline comment). The obvious thing to me to do would be to update the unit test "ParserPrintsStandardOpcodesWhenRequested", but it would require changes to the DwarfGenerator to create a custom prologue. If you don't want to do this, that's okay, as the code path is already untested.

Yes, that is rather out-of-the-way for what this patch is doing. My understanding is that you would be okay with letting this patch land?

[jhenderson]

In D72369#1812230, @hubert.reinterpretcast wrote:

In D72369#1812104, @jhenderson wrote:

I'd still really like to see a test that tests the other code path that tries to print DW_LNS_* (see the inline comment). The obvious thing to me to do would be to update the unit test "ParserPrintsStandardOpcodesWhenRequested", but it would require changes to the DwarfGenerator to create a custom prologue. If you don't want to do this, that's okay, as the code path is already untested.

Yes, that is rather out-of-the-way for what this patch is doing. My understanding is that you would be okay with letting this patch land?

Just wanted to suggest the idea, but what you've done is good on its own, thanks. LGTM.

llvm/include/llvm/BinaryFormat/Dwarf.h

template <> struct EnumTraits<Index> : public std::true_type {
   static constexpr char Type[4] = "IDX";
   static constexpr StringRef (*StringFn)(unsigned) = &IndexString;
 };
 
 template <> struct EnumTraits<Tag> : public std::true_type {
   static constexpr char Type[4] = "TAG";
   static constexpr StringRef (*StringFn)(unsigned) = &TagString;
 };
+
+template <> struct EnumTraits<LineNumberOps> : public std::true_type {
+  static constexpr char Type[4] = "LNS";
+  static constexpr StringRef (*StringFn)(unsigned) = &LNStandardString;
+};
 } // End of namespace dwarf
 
 /// Dwarf constants format_provider
 ///
 /// Specialization of the format_provider template for dwarf enums. Unlike the
 /// dumping functions above, these format unknown enumerator values as
 /// DW_TYPE_unknown_1234 (e.g. DW_TAG_unknown_ffff).
 template <typename Enum>

llvm/lib/BinaryFormat/Dwarf.cpp

bool llvm::dwarf::isValidFormForVersion(Form F, unsigned Version,
   }
   return ExtensionsOk;
 }
 
 constexpr char llvm::dwarf::EnumTraits<Attribute>::Type[];
 constexpr char llvm::dwarf::EnumTraits<Form>::Type[];
 constexpr char llvm::dwarf::EnumTraits<Index>::Type[];
 constexpr char llvm::dwarf::EnumTraits<Tag>::Type[];
+constexpr char llvm::dwarf::EnumTraits<LineNumberOps>::Type[];

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/DebugInfo/DWARF/DWARFFormValue.h"
 #include "llvm/DebugInfo/DWARF/DWARFRelocMap.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Format.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
 #include <cinttypes>
 #include <cstdint>
 #include <cstdio>
 #include <utility>
OS << format(" prologue_length: 0x%8.8" PRIx64 "\n", PrologueLength)
      << format(" min_inst_length: %u\n", MinInstLength)
      << format(getVersion() >= 4 ? "max_ops_per_inst: %u\n" : "", MaxOpsPerInst)
      << format(" default_is_stmt: %u\n", DefaultIsStmt)
      << format("       line_base: %i\n", LineBase)
      << format("      line_range: %u\n", LineRange)
      << format("     opcode_base: %u\n", OpcodeBase);
 
   for (uint32_t I = 0; I != StandardOpcodeLengths.size(); ++I)
-    OS << format("standard_opcode_lengths[%s] = %u\n",
-                 LNStandardString(I + 1).data(), StandardOpcodeLengths[I]);
+    OS << formatv("standard_opcode_lengths[{0}] = {1}\n",
+                  static_cast<dwarf::LineNumberOps>(I + 1),
+                  StandardOpcodeLengths[I]);
 
   if (!IncludeDirectories.empty()) {
     // DWARF v5 starts directory indexes at 0.
     uint32_t DirBase = getVersion() >= 5 ? 0 : 1;
     for (uint32_t I = 0; I != IncludeDirectories.size(); ++I) {
       OS << format("include_directories[%3u] = ", I + DirBase);
       IncludeDirectories[I].dump(OS, DumpOptions);
       OS << '\n';
if (Opcode == 0) {
       // Otherwise we have an unparseable line-number program.
       if (*OffsetPtr - ExtOffset != Len)
         return createStringError(errc::illegal_byte_sequence,
                            "unexpected line op length at offset 0x%8.8" PRIx64
                            " expected 0x%2.2" PRIx64 " found 0x%2.2" PRIx64,
                            ExtOffset, Len, *OffsetPtr - ExtOffset);
     } else if (Opcode < Prologue.OpcodeBase) {
       if (OS)
         *OS << LNStandardString(Opcode);
       switch (Opcode) {
       // Standard Opcodes
       case DW_LNS_copy:
         // Takes no arguments. Append a row to the matrix using the
         // current values of the state-machine registers.
         if (OS) {
           *OS << "\n";
           OS->indent(12);

llvm/test/tools/llvm-dwarfdump/X86/debug-line.s

 # CHECK-NEXT: standard_opcode_lengths[DW_LNS_set_column] = 1
 # CHECK-NEXT: standard_opcode_lengths[DW_LNS_negate_stmt] = 0
 # CHECK-NEXT: standard_opcode_lengths[DW_LNS_set_basic_block] = 0
 # CHECK-NEXT: standard_opcode_lengths[DW_LNS_const_add_pc] = 0
 # CHECK-NEXT: standard_opcode_lengths[DW_LNS_fixed_advance_pc] = 1
 # CHECK-NEXT: standard_opcode_lengths[DW_LNS_set_prologue_end] = 0
 # CHECK-NEXT: standard_opcode_lengths[DW_LNS_set_epilogue_begin] = 0
 # CHECK-NEXT: standard_opcode_lengths[DW_LNS_set_isa] = 1
-# CHECK-NEXT: standard_opcode_lengths[(null)] = 0
+# CHECK-NEXT: standard_opcode_lengths[DW_LNS_unknown_d] = 0
 # CHECK-NEXT: include_directories[  0] = "dir1/dir2"
 # CHECK-NEXT: file_names[  0]:
 # CHECK-NEXT:            name: "file1.c"
 # CHECK-NEXT:       dir_index: 2
 # CHECK-NEXT:        mod_time: 0x12345678
 # CHECK-NEXT:          length: 0x00000010
 # CHECK-EMPTY:
 # CHECK-NEXT: Address            Line   Column File   ISA Discriminator Flags