Page MenuHomePhabricator

[DWARFYAML] Teach yaml2obj emit the correct line table program.
ClosedPublic

Authored by Higuoxing on Aug 11 2020, 2:53 AM.

Details

Summary

The following issues are addressed in this patch.

  1. The operands of DW_LNE_set_discriminator should be an ULEB128 number rather than an address.
  2. Test the emitted opcodes.

Diff Detail

Event Timeline

Higuoxing created this revision.Aug 11 2020, 2:53 AM
Herald added a project: Restricted Project. · View Herald Transcript
Higuoxing requested review of this revision.Aug 11 2020, 2:53 AM
jhenderson accepted this revision.Aug 11 2020, 3:10 AM

LGTM, since it's an improvement on the current behaviour, but I think my inline comment should be addressed in a follow-up (assuming you agree, of course).

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

It's good to have a test for this, but I actually think the behaviour is incorrect for yaml2obj. It's possible to have line tables without .debug_info, and there's no reference from a line table to .debug_info anyway. The address should be inferred from the target machine, in my opinion (in this case it would be 8).

From the DWARF v4 spec:

  1. DW_LNE_set_address The DW_LNE_set_address opcode takes a single relocatable address as an operand. The size of the operand is the size of an address on the target machine.
This revision is now accepted and ready to land.Aug 11 2020, 3:10 AM
Higuoxing added inline comments.Aug 11 2020, 3:16 AM
llvm/test/tools/yaml2obj/ELF/DWARF/debug-line.yaml
489–494

Oh, I see. What do you think of having an optional field AddressSize for the line table. If it's specified, yaml2obj will emit addresses according to the value.

jhenderson added inline comments.Aug 11 2020, 3:20 AM
llvm/test/tools/yaml2obj/ELF/DWARF/debug-line.yaml
489–494

We should probably only have an explicit AddressSize field for v5 line tables, I think, but we could use the same internal logic that that uses to identify it's default value when missing (which should match that of other sections), to determine the address size.

Higuoxing added inline comments.Aug 11 2020, 3:30 AM
llvm/test/tools/yaml2obj/ELF/DWARF/debug-line.yaml
489–494

I got it. We should make the address size inferred from the target machine when the version is less than or equal to 4. When the version is 5, there should be an optional field AddressSize. In a follow-up patch, the reference of address size of debug_info should be removed and this test case should be modified, right?

jhenderson added inline comments.Aug 11 2020, 3:52 AM
llvm/test/tools/yaml2obj/ELF/DWARF/debug-line.yaml
489–494

Yes, exactly.

Higuoxing added inline comments.Aug 11 2020, 4:17 AM
llvm/test/tools/yaml2obj/ELF/DWARF/debug-line.yaml
489–494

Thanks! I think it's not very difficult to fix it. I can give a fix and rebase this patch on that.

Higuoxing updated this revision to Diff 284676.Aug 11 2020, 5:25 AM

Remove test case (j)

Higuoxing requested review of this revision.Aug 11 2020, 5:26 AM
Higuoxing edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Aug 11 2020, 5:41 AM