[DWARF] Make llvm-dwarfdump display the .debug_loc.dwo section (fixes PR38991)

[wolfgangp]

Summary

A trivial fix. When creating the DebugLocDWO context, look at the number of DWO compile units.

[dblaikie]

Should the address size be required/used for parsing the debug_loc.dwo
section? I would think there shouldn't be any addresses, or otherwise
address-size-dependent there?

[wolfgangp]

In D52444#1244247, @dblaikie wrote:

Should the address size be required/used for parsing the debug_loc.dwo
section? I would think there shouldn't be any addresses, or otherwise
address-size-dependent there?

Yes, but currently DWARFExpression is oblivious to DWO and insists on having a valid Address Size. This would have to be fixed before we can omit the address size.

[dblaikie]

In D52444#1245403, @wolfgangp wrote:

In D52444#1244247, @dblaikie wrote:

Should the address size be required/used for parsing the debug_loc.dwo
section? I would think there shouldn't be any addresses, or otherwise
address-size-dependent there?

Yes, but currently DWARFExpression is oblivious to DWO and insists on having a valid Address Size. This would have to be fixed before we can omit the address size.

Is that sufficiently expensive to be worth doing this workaround in the interim?

What if the address size was hardcoded to 4 here, for example? What would go wrong in that case?

(could leave a fixit that this shouldn't be needed - maybe use Optional for the address size, so it asserts if someone doesn't provide an address size but it ends up being needed down the line)

[wolfgangp]

In D52444#1245696, @dblaikie wrote:

In D52444#1245403, @wolfgangp wrote:

In D52444#1244247, @dblaikie wrote:

Should the address size be required/used for parsing the debug_loc.dwo
section? I would think there shouldn't be any addresses, or otherwise
address-size-dependent there?

Yes, but currently DWARFExpression is oblivious to DWO and insists on having a valid Address Size. This would have to be fixed before we can omit the address size.

Is that sufficiently expensive to be worth doing this workaround in the interim?

The question is whether to bring it up to DWARF5 level right away or to just have it deal with DWO with what's currently supported. The former would certainly be more expensive.

What if the address size was hardcoded to 4 here, for example? What would go wrong in that case?

A DW_OP_addr or DW_OP_call_ref opcode with an 8-byte operand would cause the rest of the expression to be parsed incorrectly. That's unlikely to occur in a location list, but it would definitely fail.
As a matter of fact, DW_OP_call_ref uses AddrSize to decide whether to get 32 or 64 bits, when this should be dependent on the DWARF format (32 vs. 64). So instead of addrsize we should pass the format instead in a DWO context.

(could leave a fixit that this shouldn't be needed - maybe use Optional for the address size, so it asserts if someone doesn't provide an address size but it ends up being needed down the line)

I had originally thought to get location lists up to DWARF 5 level (i.e. just the table and range aspect), but deal with expressions later. Perhaps in the interim it's best to have what's currently there deal with DWO correctly so we don't have anything half-working out there.

[wolfgangp]

One reason why we may still want addrsize is so we can skip the correct amount of bytes during parsing if we do find a DW_OP_addr in a DWO (and report an error).

[dblaikie]

In D52444#1245744, @wolfgangp wrote:

In D52444#1245696, @dblaikie wrote:

In D52444#1245403, @wolfgangp wrote:

In D52444#1244247, @dblaikie wrote:

Should the address size be required/used for parsing the debug_loc.dwo
section? I would think there shouldn't be any addresses, or otherwise
address-size-dependent there?

Yes, but currently DWARFExpression is oblivious to DWO and insists on having a valid Address Size. This would have to be fixed before we can omit the address size.

Is that sufficiently expensive to be worth doing this workaround in the interim?

The question is whether to bring it up to DWARF5 level right away or to just have it deal with DWO with what's currently supported. The former would certainly be more expensive.

Not sure I follow what you mean by that - what it has to do with DWARF5 or not. The pre-DWARF5 version of split-DWARF wouldn't've used addresses in the .dwo file - the addresses aren't known at that time, I think?

What if the address size was hardcoded to 4 here, for example? What would go wrong in that case?

A DW_OP_addr or DW_OP_call_ref opcode with an 8-byte operand would cause the rest of the expression to be parsed incorrectly. That's unlikely to occur in a location list, but it would definitely fail.

Those would be invalid OP kinds in location list in debug_loc.dwo though, right?

As a matter of fact, DW_OP_call_ref uses AddrSize to decide whether to get 32 or 64 bits, when this should be dependent on the DWARF format (32 vs. 64). So instead of addrsize we should pass the format instead in a DWO context.

(could leave a fixit that this shouldn't be needed - maybe use Optional for the address size, so it asserts if someone doesn't provide an address size but it ends up being needed down the line)

I had originally thought to get location lists up to DWARF 5 level (i.e. just the table and range aspect), but deal with expressions later. Perhaps in the interim it's best to have what's currently there deal with DWO correctly so we don't have anything half-working out there.

I think my general question is: What behavior of llvm-dwarfdump would be wrong if the address size was hardcoded to 4 (or 3? or any other number) for parsing debug_loc.dwo?

[dblaikie]

In D52444#1245772, @wolfgangp wrote:

One reason why we may still want addrsize is so we can skip the correct amount of bytes during parsing if we do find a DW_OP_addr in a DWO (and report an error).

I think it'd be reasonable to report an error and stop parsing - skipping the rest of the location list? The value is out of place in the DWO section, so I'm not sure it's reasonable to conclude what length it should be, etc.

[wolfgangp]

In D52444#1245773, @dblaikie wrote:

In D52444#1245744, @wolfgangp wrote:

In D52444#1245696, @dblaikie wrote:

In D52444#1245403, @wolfgangp wrote:

In D52444#1244247, @dblaikie wrote:

Should the address size be required/used for parsing the debug_loc.dwo
section? I would think there shouldn't be any addresses, or otherwise
address-size-dependent there?

Yes, but currently DWARFExpression is oblivious to DWO and insists on having a valid Address Size. This would have to be fixed before we can omit the address size.

Is that sufficiently expensive to be worth doing this workaround in the interim?

The question is whether to bring it up to DWARF5 level right away or to just have it deal with DWO with what's currently supported. The former would certainly be more expensive.

Not sure I follow what you mean by that - what it has to do with DWARF5 or not. The pre-DWARF5 version of split-DWARF wouldn't've used addresses in the .dwo file - the addresses aren't known at that time, I think?

This was more about workflow. There are a few DWARF5 operands that are not supported yet, so I was just musing whether to implement DWARF5 first completely or just fix the DWO case and complete DWARF5 later.

What if the address size was hardcoded to 4 here, for example? What would go wrong in that case?

A DW_OP_addr or DW_OP_call_ref opcode with an 8-byte operand would cause the rest of the expression to be parsed incorrectly. That's unlikely to occur in a location list, but it would definitely fail.

Those would be invalid OP kinds in location list in debug_loc.dwo though, right?

I would think at least a DW_OP_call_ref could legally occur in a location description, even in a DWO, e.g in a memory location description. Unlikely anybody generates this, I would think.

As a matter of fact, DW_OP_call_ref uses AddrSize to decide whether to get 32 or 64 bits, when this should be dependent on the DWARF format (32 vs. 64). So instead of addrsize we should pass the format instead in a DWO context.

(could leave a fixit that this shouldn't be needed - maybe use Optional for the address size, so it asserts if someone doesn't provide an address size but it ends up being needed down the line)

I had originally thought to get location lists up to DWARF 5 level (i.e. just the table and range aspect), but deal with expressions later. Perhaps in the interim it's best to have what's currently there deal with DWO correctly so we don't have anything half-working out there.

I think my general question is: What behavior of llvm-dwarfdump would be wrong if the address size was hardcoded to 4 (or 3? or any other number) for parsing debug_loc.dwo?

If it's hard-coded to anything other than 4 or 8 the constructor for DWARFExpression would assert. If it's hard-coded to 4 it would work just fine (except for the DW_OP_call_ref example with an 8-byte operand mentioned above, and that would probably just report an obscure error and move on to the next list).

[wolfgangp]

If it's hard-coded to anything other than 4 or 8 the constructor for DWARFExpression would assert. If it's hard-coded to 4 it would work just fine (except for the DW_OP_call_ref example with an 8-byte operand mentioned above, and that would probably just report an obscure error and move on to the next list).

In fact, the 8-byte operand would only be valid for DWARF64, which we don't support at the moment, so the bottom line is I can't see anything that would be incorrectly handled with a hard-coded 4.

[wolfgangp]

Hardcoding address size to 4 as a workaround with a FIXME comment. DWARFExpression is unaware of DWO and requires it at the moment.

[dblaikie]

You should be able to remove the condition on numDWOCompileUnits now that parsing debug_loc.dwo doesn't need a CU of any kind? Be good to have a/the test case validate that the dumping works in the absence of any debug_info(.dwo) sections. Just the debug_loc.dwo all by itself. (this is my main motivation with the line of questioning/discussion here - it's nice to be able to dump independent sections even if they appear alone and/or without parsing other (Possibly corrupt) debug info)

(also, I did go back and read the spec - and you're right that call_ref uses an address-width-sized value, and that it could be used in debug_loclists.dwo. Appendix B of DWARF5 even states specifically (in (fo)) that DW_OP_call_ref can be used to reference from somewhere in debug_info.dwo to somewhere else in debug_info.dwo - though interestingly it doesn't mention any chance of referencing debug_info.dwo from debug_loc.dwo - so maybe it isn't valid in debug_loclists.dwo (or debug_loc.dwo)?

And honestly I'm not sure it makes sense to use it in debug_info.dwo either - because DWP tools are meant to work without relocations - and this form would require a relocation)

[probinson]

In D52444#1253212, @dblaikie wrote:

(also, I did go back and read the spec - and you're right that call_ref uses an address-width-sized value, and that it could be used in debug_loclists.dwo.

Where do you see that? In DWARF 5, section 2.5.1.5, item 4 (p.36) it says call_ref is a reference not an address; its size depends on format (32/64) not target address size.

Appendix B of DWARF5 even states specifically (in (fo)) that DW_OP_call_ref can be used to reference from somewhere in debug_info.dwo to somewhere else in debug_info.dwo - though interestingly it doesn't mention any chance of referencing debug_info.dwo from debug_loc.dwo - so maybe it isn't valid in debug_loclists.dwo (or debug_loc.dwo)?

And honestly I'm not sure it makes sense to use it in debug_info.dwo either - because DWP tools are meant to work without relocations - and this form would require a relocation)

This is probably a spec bug; DW_OP_call_ref (and DW_FORM_ref_addr) shouldn't be allowed in a .dwo section. I see call_ref is already prohibited in type units, and .dwo sections should follow the same reasoning, I think. I'll file this as a DWARF issue.

[dblaikie]

In D52444#1253929, @probinson wrote:

In D52444#1253212, @dblaikie wrote:

(also, I did go back and read the spec - and you're right that call_ref uses an address-width-sized value, and that it could be used in debug_loclists.dwo.

Where do you see that? In DWARF 5, section 2.5.1.5, item 4 (p.36) it says call_ref is a reference not an address; its size depends on format (32/64) not target address size.

sorry, yes, I meant format

Appendix B of DWARF5 even states specifically (in (fo)) that DW_OP_call_ref can be used to reference from somewhere in debug_info.dwo to somewhere else in debug_info.dwo - though interestingly it doesn't mention any chance of referencing debug_info.dwo from debug_loc.dwo - so maybe it isn't valid in debug_loclists.dwo (or debug_loc.dwo)?

And honestly I'm not sure it makes sense to use it in debug_info.dwo either - because DWP tools are meant to work without relocations - and this form would require a relocation)

This is probably a spec bug; DW_OP_call_ref (and DW_FORM_ref_addr) shouldn't be allowed in a .dwo section. I see call_ref is already prohibited in type units, and .dwo sections should follow the same reasoning, I think. I'll file this as a DWARF issue.

SGTM - thanks!

[wolfgangp]

• Removed the condition on the existence of compile units.
• Added a test to check that llvm-dwarfdump can dump the .debug_loc.dwo section even without compile units present.

[dblaikie]

test/tools/llvm-dwarfdump/X86/debug_loc_dwo.ll

5–6 ↗

(On Diff #168182)

I think this is already tested - in, say, test/DebugInfo/X86/fission-ranges.ll

I think you can probably skip this test case - and just keep the other one that's testing the new functionality you're enabling?

test/tools/llvm-dwarfdump/X86/debug_loc_dwo.s

15 ↗

(On Diff #168182)

What further interpretation of the address index could it do?

[wolfgangp]

test/tools/llvm-dwarfdump/X86/debug_loc_dwo.ll

5–6 ↗

(On Diff #168182)

There is a subtle difference between the fission-ranges.ll and this one. In fission-ranges, the object file contains both the non-split and the split sections, which is why it was passing even before this fix.

The new test case demonstrates that it works also when the .dwo sections have been separated out into a separate .dwo file, which was not working.

I can fold this case into fission-ranges if you prefer.

test/tools/llvm-dwarfdump/X86/debug_loc_dwo.s

15 ↗

(On Diff #168182)

What further interpretation of the address index could it do?

Theoretically, if you're dumping an object file where the fission sections haven't been separated out yet, you could refer to the skeleton unit's .debug_addr section through the index and dump the actual addresses. Don't know if that would be a common use-case, though.

[dblaikie]

test/tools/llvm-dwarfdump/X86/debug_loc_dwo.ll

5–6 ↗

(On Diff #168182)

Ah, I see.

Not sure it's worth test coverage though - if it can be dumped without any other sections (demonstrated by the other test), then the fact that it can be dumped with some but not other sections present seems like it's not an interesting/distinct case to test for?

test/tools/llvm-dwarfdump/X86/debug_loc_dwo.s

15 ↗

(On Diff #168182)

Fair enough - doesn't seem to be relevant to this test case (since there's no debug_addr section in this file), so might be confusing in a comment here?

[wolfgangp]

Eliminated one of the test cases due to redundancy and adjusted a comment in the other one.

test/tools/llvm-dwarfdump/X86/debug_loc_dwo.ll

5–6 ↗

(On Diff #168182)

True, the other test case covers the failing case as well. I'll remove it then.

test/tools/llvm-dwarfdump/X86/debug_loc_dwo.s

15 ↗

(On Diff #168182)

OK I'll get rid of it.

[dblaikie]

Great - thanks!

llvm/trunk/lib/DebugInfo/DWARF/DWARFContext.cpp

 }
 
 const DWARFDebugLocDWO *DWARFContext::getDebugLocDWO() {
   if (LocDWO)
     return LocDWO.get();
 
   LocDWO.reset(new DWARFDebugLocDWO());
   // Assume all compile units have the same address byte size.
-  if (getNumCompileUnits()) {
-    DataExtractor LocData(DObj->getLocDWOSection().Data, isLittleEndian(),
-                          getUnitAtIndex(0)->getAddressByteSize());
-    LocDWO->parse(LocData);
-  }
+  // FIXME: We don't need AddressSize for split DWARF since relocatable
+  // addresses cannot appear there. At the moment DWARFExpression requires it.
+  DataExtractor LocData(DObj->getLocDWOSection().Data, isLittleEndian(), 4);
   return LocDWO.get();
 }
 
 const DWARFDebugAranges *DWARFContext::getDebugAranges() {
   if (Aranges)
     return Aranges.get();
 
   Aranges.reset(new DWARFDebugAranges());

llvm/trunk/test/tools/llvm-dwarfdump/X86/debug_loc_dwo.s

+# RUN: llvm-mc %s -filetype obj -triple x86_64-pc-linux -o %t.o
+# RUN: llvm-dwarfdump --debug-loc %t.o | FileCheck %s
+
+# We make sure that llvm-dwarfdump can dump the .debug_loc.dwo section
+# without requiring a compilation unit in the .debug_info.dwo section.
+
+# CHECK:         .debug_loc.dwo contents:
+# CHECK-NEXT:    0x00000000:
+# CHECK-NEXT:    Addr idx 1 (w/ length 16): DW_OP_reg5 RDI
+
+.section .debug_loc.dwo,"",@progbits
+# One location list. The pre-DWARF v5 implementation only recognizes
+# DW_LLE_startx_length as an entry kind in .debug_loc.dwo (besides
+# end_of_list), which is what llvm generates as well.
+.byte 3          # DW_LLE_startx_length
+.byte 0x01       # Index
+.long 0x10       # Length
+.short 1         # Loc expr size
+.byte 85         # DW_OP_reg5
+.byte 0          # DW_LLE_end_of_list