[DWARF] Check for format mismatch between CU and Range List Table.

[ikudrin]

Summary

As stated in section 7.4, p. 198 of DWARF Standard Version 5, "The 32-bit and 64-bit DWARF format conventions must not be intermixed within a single compilation unit."

[Orlando]

lib/DebugInfo/DWARF/DWARFUnit.cpp

311 ↗

(On Diff #218449)

Is there a reason for OffsetIn to exist as opposed to just using Offset here?

[ikudrin]

lib/DebugInfo/DWARF/DWARFUnit.cpp

311 ↗

(On Diff #218449)

Offset is changed in Table.extractHeaderAndOffsets() (line 323), but we have to show the original value in the error message (line 326).

[aprantl]

lib/DebugInfo/DWARF/DWARFUnit.cpp

311 ↗

(On Diff #218449)

Perhaps: StartOffset?

[dblaikie]

I'm not sure if using "OffsetIn" in the diagnostic is the right thing to do.

Honestly the fact that DWARFv5 specifies that the "rnglist base address" (& loclist bast address, and str_offsets base) point to the beginning of the list rather than the start of the header is super quirky and I don't think it's helpful to users to highlight that. (though I doubt DWARFvNext would bother fixing this - it'sjust quirky, not super problematic for any practical reasons I can think of)

[ikudrin]

In D67097#1656268, @dblaikie wrote:

I'm not sure if using "OffsetIn" in the diagnostic is the right thing to do.

Maybe it will be better to use the adjusted offset which points to the start of the header?

[ikudrin]

• Print the adjusted offset which points to the start of the header.

@dblaikie, what about this variant?

[probinson]

In D67097#1656268, @dblaikie wrote:

I'm not sure if using "OffsetIn" in the diagnostic is the right thing to do.

Honestly the fact that DWARFv5 specifies that the "rnglist base address" (& loclist bast address, and str_offsets base) point to the beginning of the list rather than the start of the header is super quirky and I don't think it's helpful to users to highlight that. (though I doubt DWARFvNext would bother fixing this - it'sjust quirky, not super problematic for any practical reasons I can think of)

IIRC this is one of Cary's, and argued that it could be directly used to index into section, instead of you having to step past the header to use it. Anyway we let him get away with it, for better or worse. It would have to be essentially unusable or horribly inefficient, for the committee to be willing to change it now.

[dblaikie]

In D67097#1657964, @probinson wrote:

In D67097#1656268, @dblaikie wrote:

I'm not sure if using "OffsetIn" in the diagnostic is the right thing to do.

Honestly the fact that DWARFv5 specifies that the "rnglist base address" (& loclist bast address, and str_offsets base) point to the beginning of the list rather than the start of the header is super quirky and I don't think it's helpful to users to highlight that. (though I doubt DWARFvNext would bother fixing this - it'sjust quirky, not super problematic for any practical reasons I can think of)

IIRC this is one of Cary's, and argued that it could be directly used to index into section, instead of you having to step past the header to use it. Anyway we let him get away with it, for better or worse.

Meh, I can't imagine the (Start+Header)[N] would be too expensive (& Start+Header could be computed once/ahead of time easily enough).

It would have to be essentially unusable or horribly inefficient, for the committee to be willing to change it now.

Not sure what would be unusable or inefficient about it - could you explain more?

I'd figure if DWARFvNext changed the semantics of the various *_base attributes, then an implementation would look at the CU version to decide whether it should walk backwards to find the header, or forwards to find the list.

[dblaikie]

In D67097#1657093, @ikudrin wrote:

• Print the adjusted offset which points to the start of the header.

@dblaikie, what about this variant?

This update didn't seem to touch a test case - is this untested?

[ikudrin]

In D67097#1658364, @dblaikie wrote:

This update didn't seem to touch a test case - is this untested?

The test is for a DWO case, where we do not adjust the offset. I would not expect the check to trigger for regular files because we adjust the offset depending on the assumed header size.

[ikudrin]

• Rebase on the current tip.

[probinson]

In D67097#1658363, @dblaikie wrote:

In D67097#1657964, @probinson wrote:

In D67097#1656268, @dblaikie wrote:

I'm not sure if using "OffsetIn" in the diagnostic is the right thing to do.

Honestly the fact that DWARFv5 specifies that the "rnglist base address" (& loclist bast address, and str_offsets base) point to the beginning of the list rather than the start of the header is super quirky and I don't think it's helpful to users to highlight that. (though I doubt DWARFvNext would bother fixing this - it'sjust quirky, not super problematic for any practical reasons I can think of)

IIRC this is one of Cary's, and argued that it could be directly used to index into section, instead of you having to step past the header to use it. Anyway we let him get away with it, for better or worse.

Meh, I can't imagine the (Start+Header)[N] would be too expensive (& Start+Header could be computed once/ahead of time easily enough).

By that argument, computing (Start-Header) is not expensive and so why is it worth changing? For aesthetics? If it bugs you that much, you can write the proposal and post it to dwarfstd.org; it doesn't have to include the edit to the wording, just "these attributes should point here instead of there."

It would have to be essentially unusable or horribly inefficient, for the committee to be willing to change it now.

Not sure what would be unusable or inefficient about it - could you explain more?

(rewrite) There isn't anything terribly unusable or inefficient about it, so unless somebody really complains, the committee is unlikely to change it now. This isn't the C++ committee, after all.

[dblaikie]

In D67097#1659662, @probinson wrote:

In D67097#1658363, @dblaikie wrote:

In D67097#1657964, @probinson wrote:

In D67097#1656268, @dblaikie wrote:

I'm not sure if using "OffsetIn" in the diagnostic is the right thing to do.

Honestly the fact that DWARFv5 specifies that the "rnglist base address" (& loclist bast address, and str_offsets base) point to the beginning of the list rather than the start of the header is super quirky and I don't think it's helpful to users to highlight that. (though I doubt DWARFvNext would bother fixing this - it'sjust quirky, not super problematic for any practical reasons I can think of)

IIRC this is one of Cary's, and argued that it could be directly used to index into section, instead of you having to step past the header to use it. Anyway we let him get away with it, for better or worse.

Meh, I can't imagine the (Start+Header)[N] would be too expensive (& Start+Header could be computed once/ahead of time easily enough).

By that argument, computing (Start-Header) is not expensive and so why is it worth changing? For aesthetics? If it bugs you that much, you can write the proposal and post it to dwarfstd.org; it doesn't have to include the edit to the wording, just "these attributes should point here instead of there."

Yep, that's roughly what I was getting at when I said "(though I doubt DWARFvNext would bother fixing this - it'sjust quirky, not super problematic for any practical reasons I can think of)".

It would have to be essentially unusable or horribly inefficient, for the committee to be willing to change it now.

Not sure what would be unusable or inefficient about it - could you explain more?

(rewrite) There isn't anything terribly unusable or inefficient about it, so unless somebody really complains, the committee is unlikely to change it now. This isn't the C++ committee, after all.

Not sure I understand the snark at the C++ committee, but anyway.

[probinson]

In D67097#1659917, @dblaikie wrote:

Not sure I understand the snark at the C++ committee, but anyway.

Oh, because of their perennial willingness to change a released standard, with no way to identify revisions. The C++11 of today is not the same as the C++11 of 2011, for example. This is basic revision control, everybody (else) understands this. I mean, if we found a bug in Clang 8.0.0, we wouldn't overwrite the archived 8.0.0 to fix it. But that's effectively what the C++ committee does with its "defect repair."
The DWARF committee has the decency to provide a new version number when the interpretation of some attribute changes.

[dblaikie]

In D67097#1661014, @probinson wrote:

In D67097#1659917, @dblaikie wrote:

Not sure I understand the snark at the C++ committee, but anyway.

Oh, because of their perennial willingness to change a released standard, with no way to identify revisions. The C++11 of today is not the same as the C++11 of 2011, for example. This is basic revision control, everybody (else) understands this. I mean, if we found a bug in Clang 8.0.0, we wouldn't overwrite the archived 8.0.0 to fix it. But that's effectively what the C++ committee does with its "defect repair."
The DWARF committee has the decency to provide a new version number when the interpretation of some attribute changes.

Ah, you're referring to the C++ standard defect report process. Fair enough. Though it seems a bit out of place/unrelated to this discussion. I didn't intend to suggest that DWARFv4 should be retroactively changed - sorry if it came off that way.

Happy to have a separate discussion about the C++ committee process some time.

[ikudrin]

Sorry to intervene, but what about the patch itself? Is it OK to land with the latest changes or should I improve it somehow?

[probinson]

Ah right, the actual patch. I think it would be better to also report the base of the unit that referenced the range list, if that's not too inconvenient.
What you have here is fine with me, apart from that question.

[probinson]

FTR, I have also been thinking that perhaps the dumper should be more tolerant of this kind of thing, attempting to report more of what is present rather than throwing a hissy fit when things aren't perfect. But I'm just noting that for the record. What this patch does is consistent with how the rest of lib/DebugInfo/DWARF behaves, and revisiting that behavior is well beyond the scope of this patch.

As I said above, the patch so far is fine with me, with the one open question about reporting the referencing unit.

[ikudrin]

• Rebase to the tip.
• Add reporting the offset of a compilation unit which format is taken as a reference.

[dblaikie]

yeah, as @probinson said, at some point someone could add a callback error filter here to decide what's a fatal error and what isn't - but this seems reasonable for now

[ikudrin]

Actually, I've discovered that we parse range list table headers for every unit, including type units, even if they do not reference them. Thus, this patch creates more problems than solves; it should be postponed until we make extracting range list tables lazy and do it only for compilation units which actually reference the tables.

llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp

bool DWARFUnitHeader::extract(DWARFContext &Context,
   return true;
 }
 
 // Parse the rangelist table header, including the optional array of offsets
 // following it (DWARF v5 and later).
 template<typename ListTableType>
 static Expected<ListTableType>
 parseListTableHeader(DWARFDataExtractor &DA, uint64_t Offset,
-                        DwarfFormat Format) {
+                        DwarfFormat Format, uint64_t CUOffset) {
   // We are expected to be called with Offset 0 or pointing just past the table
   // header. Correct Offset in the latter case so that it points to the start
   // of the header.
   if (Offset > 0) {
     uint64_t HeaderSize = DWARFListTableHeader::getHeaderSize(Format);
     if (Offset < HeaderSize)
       return createStringError(errc::invalid_argument, "did not detect a valid"
                                " list table with base = 0x%" PRIx64 "\n",
                                Offset);
     Offset -= HeaderSize;
   }
   ListTableType Table;
+  uint64_t StartOffset = Offset;
   if (Error E = Table.extractHeaderAndOffsets(DA, &Offset))
     return std::move(E);
+  if (Format != Table.getFormat())
+    return createStringError(errc::invalid_argument,
+                             "mismatched DWARF formats between "
+                             "CU at offset 0x%" PRIx64 " "
+                             "and list table with base = 0x%" PRIx64 "\n",
+                             CUOffset, StartOffset);
   return Table;
 }
 
 Error DWARFUnit::extractRangeList(uint64_t RangeListOffset,
                                   DWARFDebugRangeList &RangeList) const {
   // Require that compile unit is extracted.
   assert(!DieArray.empty());
   DWARFDataExtractor RangesData(Context.getDWARFObj(), *RangeSection,
else
       setRangesSection(&Context.getDWARFObj().getRnglistsSection(),
                        toSectionOffset(UnitDie.find(DW_AT_rnglists_base), 0));
     if (RangeSection->Data.size()) {
       // Parse the range list table header. Individual range lists are
       // extracted lazily.
       DWARFDataExtractor RangesDA(Context.getDWARFObj(), *RangeSection,
                                   isLittleEndian, 0);
       auto TableOrError = parseListTableHeader<DWARFDebugRnglistTable>(
-          RangesDA, RangeSectionBase, Header.getFormat());
+          RangesDA, RangeSectionBase, Header.getFormat(), Header.getOffset());
       if (!TableOrError)
         return createStringError(errc::invalid_argument,
                                  "parsing a range list table: " +
                                      toString(TableOrError.takeError()));
 
       RngListTable = TableOrError.get();
 
       // In a split dwarf unit, there is no DW_AT_rnglists_base attribute.
bool DWARFUnit::parseDWO() {
   // Share .debug_addr and .debug_ranges section with compile unit in .dwo
   if (AddrOffsetSectionBase)
     DWO->setAddrOffsetSection(AddrOffsetSection, *AddrOffsetSectionBase);
   if (getVersion() >= 5) {
     DWO->setRangesSection(&Context.getDWARFObj().getRnglistsDWOSection(), 0);
     DWARFDataExtractor RangesDA(Context.getDWARFObj(), *RangeSection,
                                 isLittleEndian, 0);
     if (auto TableOrError = parseListTableHeader<DWARFDebugRnglistTable>(
-            RangesDA, RangeSectionBase, Header.getFormat()))
+            RangesDA, RangeSectionBase, Header.getFormat(), Header.getOffset()))
       DWO->RngListTable = TableOrError.get();
     else
       WithColor::error() << "parsing a range list table: "
                          << toString(TableOrError.takeError())
                          << '\n';
     if (DWO->RngListTable)
       DWO->RangeSectionBase = DWO->RngListTable->getHeaderSize();
   } else {

llvm/test/DebugInfo/X86/dwarfdump-rnglists-format-mismatch.s

+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o - | \
+# RUN:   llvm-dwarfdump -debug-info - 2>&1 > /dev/null | \
+# RUN:   FileCheck %s
+
+        .section .debug_abbrev.dwo,"",@progbits
+        .byte 0x01      # Abbrev code
+        .byte 0x11      # DW_TAG_compile_unit
+        .byte 0x00      # DW_CHILDREN_no
+        .byte 0x55      # DW_AT_ranges
+        .byte 0x23      # DW_FORM_rnglistx
+        .byte 0x00      # EOM(1)
+        .byte 0x00      # EOM(2)
+        .byte 0x00      # EOM(3)
+
+        .section .debug_info.dwo,"",@progbits
+        .long .LCUend - .LCUversion     # Length of Unit
+.LCUversion:
+        .short 5                        # DWARF version number
+        .byte 5                         # DW_UT_split_compile
+        .byte 4                         # Address Size (in bytes)
+        .long 0                         # Offset Into Abbrev Section
+        .quad 0                         # DWO Id
+        .byte 1                         # Abbreviation code
+        .uleb128 0                      # DW_AT_ranges
+.LCUend:
+
+        .section .debug_rnglists.dwo,"",@progbits
+        .long 0xffffffff                # DWARF64 mark
+        .quad .LRTend - .LRTversion     # table length
+.LRTversion:
+        .short 5                        # version
+        .byte 4                         # address size
+        .byte 0                         # segment selector size
+        .long 1                         # offset entry count
+.LRTbase:
+        .quad .LRTlist0 - .LRTbase
+.LRTlist0:
+        .byte 0                         # DW_RLE_end_of_list
+.LRTend:
+
+# CHECK: error: parsing a range list table: mismatched DWARF formats between
+# CHECK-SAME: CU at offset 0x0 and list table with base = 0x0