Page MenuHomePhabricator

[DWARF5] Emit new unit header

Authored by probinson on Feb 21 2017, 8:51 AM.



Emit DWARF-5 conforming compile-unit and type-unit headers.

Diff Detail


Event Timeline

probinson created this revision.Feb 21 2017, 8:51 AM
dblaikie edited edge metadata.Feb 21 2017, 9:01 AM

Haven't actually read the spec to confirm this is implementing things correctly - few comments on the code, and looks like it could use some more test coverage. (is the DwarfGenerator change tested at all? I mean it's a test utility, so maybe that's overkill, not sure)

806 ↗(On Diff #89224)

Guess I should've read the DWARF5 spec. Why is there a split type unit? I remember Cary getting rid of this shortly after the first prototype of Fission. I'm asuming it's at least not required (ie: it's optional)?

693–694 ↗(On Diff #89224)

This is probably backwards - "Skeleton" is a pointer to the skeleton (so it implies this unit is /not/ a skeleton). Test coverage should demonstrate/confirm this?

aprantl edited edge metadata.Feb 21 2017, 9:14 AM

Thanks for getting this started!

308 ↗(On Diff #89224)

We might want to unify the spelling of DWARF v5 vs. DWARF5

316 ↗(On Diff #89224)

as seen here :-)

99 ↗(On Diff #89224)

This should probably be tested by a llvm-dwarfdump test?

(is the DwarfGenerator change tested at all? I mean it's a test utility, so maybe that's overkill, not sure)

It's like this: I was originally going to avoid the llvm-dwarfdump changes in the first patch, but it turns out there are a couple of tests that specify version 5 in the IR and use llvm-dwarfdump, so I had to make the DebugInfo lib changes. Having done that, the unit tests failed because the generator was not producing the new format. So I had to fix the generator.

806 ↗(On Diff #89224)

These are the unit types defined in the spec. Split and non-split type units are nearly identical, the difference being that a split type unit can't use DW_AT_str_offsets_base, and of course where the units actually go.

693–694 ↗(On Diff #89224)

So, you think I'm missing a split-dwarf test that uses v5? I'll look into this.

99 ↗(On Diff #89224)

Are there llvm-dwarfdump tests? I couldn't find any. There is a test/tools/llvm-dwarfdump directory (with AArch64 and ARM subdirectories) but not actual files.

This code was needed because there are a couple of debug-info tests that specify version 5 in the IR, and then use llvm-dwarfdump in their RUN lines. Having done this change, I had to fix the unit-test DWARF generator because the unit tests were failing. That was plenty to convince me that things were working.

If you want a separate llvm-dwarfdump test as well, I can do that.

probinson added inline comments.Feb 21 2017, 10:01 AM
127 ↗(On Diff #89224)

I had noticed the packing here is not optimal, Version can move after Abbrevs to save a little space. Not a big deal as there are usually relatively few units, but I thought I'd mention it. Can be saved for a separate cleanup commit.

probinson added inline comments.Feb 21 2017, 6:43 PM
308 ↗(On Diff #89224)

It looks like "DWARF v5" is more common so I've regularized to that throughout.

693–694 ↗(On Diff #89224)

I actually need to distinguish 3 cases here, because there are different unit types for (a) normal-full, (b) skeleton, and (c) split-full. But useSplitDwarf() will distinguish the first two. Similarly I will need to distinguish the normal and split type units.
Thanks for catching this!

This LGTM from my end, with an additional test that exercises llvm-dwarfdump. I'll let David move the status to "Accepted".

probinson updated this revision to Diff 89698.Feb 24 2017, 11:05 AM

Distinguish the 3 compile-unit types and 2 type-unit types correctly.
Test that all the header variants come out correctly.
Add a separate test for dwarfdump.
Fix up "DWARF5" -> "DWARF v5".

probinson marked 5 inline comments as done.Feb 24 2017, 11:06 AM

Is there test coverage for DW_UT_skeleton? DW_UT_type? (I only spot DW_UT_split_type and DW_UT_compile in the dwarfdump test case)

Oh, they're tested with assembly in dwarf-headers.ll - could they be tested with dwarfdump instead?

aprantl added inline comments.Feb 24 2017, 11:20 AM
127 ↗(On Diff #89224)

Let's do this as a separate NFC commit; no need to do pre-commit review for this change.

6 ↗(On Diff #89698)

I like the approach of using assembler for this!

probinson updated this revision to Diff 89729.Feb 24 2017, 2:37 PM

Convert dwarf-headers.ll test to use llvm-dwarfdump.

I suppose as dwarf-headers.ll is now using dwarfdump, it might not be X86-specific anymore. Maybe move it to Generic?

dblaikie accepted this revision.Feb 27 2017, 10:21 AM
This revision is now accepted and ready to land.Feb 27 2017, 10:21 AM

Oops, forgot to say actual words when I approved this (so Phab wouldn't've sent email)
Looks good - commit whenever you're ready.

This revision was automatically updated to reflect the committed changes.