Page MenuHomePhabricator

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

Authored by Higuoxing on Sat, Sep 26, 5:19 AM.

Details

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.

Diff Detail

Event Timeline

Higuoxing created this revision.Sat, Sep 26, 5:19 AM
Herald added a project: Restricted Project. · View Herald Transcript
Higuoxing requested review of this revision.Sat, Sep 26, 5:19 AM

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 updated this revision to Diff 294677.Mon, Sep 28, 5:57 AM
Higuoxing marked 3 inline comments as done.
  • 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 added inline comments.Wed, Sep 30, 12:20 AM
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 updated this revision to Diff 296099.Sun, Oct 4, 11:42 PM
Higuoxing marked an inline comment as done.

Use Version to determine the default opcode_base and standard_opcode_lengths fields.

jhenderson added inline comments.Tue, Oct 6, 4:43 AM
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 updated this revision to Diff 296892.Thu, Oct 8, 1:43 AM
Higuoxing marked 2 inline comments as done.
  • Test the default opcode_base field and the standard_opcode_lengths array of DWARFv2-DWARFv4 (DWARFv5 isn't supported yet).
  • Add missing comment.
jhenderson accepted this revision.Thu, Oct 8, 1:47 AM

LGTM.

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

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

This revision is now accepted and ready to land.Thu, Oct 8, 1:47 AM
Higuoxing added inline comments.Thu, Oct 8, 1:52 AM
llvm/test/tools/yaml2obj/ELF/DWARF/debug-line.yaml
706

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

Higuoxing edited the summary of this revision. (Show Details)Thu, Oct 8, 8:09 PM
This revision was landed with ongoing or failed builds.Thu, Oct 8, 8:10 PM
This revision was automatically updated to reflect the committed changes.