Page MenuHomePhabricator

[DWARF] [ObjectYAML] Adding APIs for unittesting
ClosedPublic

Authored by beanz on Jan 17 2017, 2:30 PM.

Details

Summary

This patch adds some new APIs to enable using the YAML DWARF representation in unit tests. The most basic new API is DWARFYAML::EmitDebugSections which converts a YAML string into a series of owned MemoryBuffer objects stored in a StringMap. The string map can then be used to construct a DWARFContext for parsing in place of an ObjectFile.

Diff Detail

Repository
rL LLVM

Event Timeline

beanz created this revision.Jan 17 2017, 2:30 PM
clayborg edited edge metadata.Jan 17 2017, 3:06 PM

Looks fine to me. I'll let David chime in though just to be sure.

dblaikie edited edge metadata.Jan 18 2017, 1:20 PM

Looks neat - few ideas/questions.

lib/DebugInfo/DWARF/DWARFContext.cpp
819–851 ↗(On Diff #84747)

Looks like a duplicate of the StringSwitch in the other ctor - could be refactored to a common utility function?

lib/ObjectYAML/DWARFEmitter.cpp
356–358 ↗(On Diff #84747)

Should this perhaps return ErrorOr<StringMap<...>> & so the error can be propagated/reported?

360–369 ↗(On Diff #84747)

These are the supported sections at the moment - but I guess it'll grow support over time? Any way to refactor this so it goes along with the support as it's added? (is there similar code elsewhere already doing something like this to generate all the outputs in yaml2obj?)

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
1163–1177 ↗(On Diff #84747)

Are raw string literals an option here/yet? (are they compatible with all LLVM's supported compilers)

beanz added inline comments.Jan 18 2017, 1:48 PM
lib/DebugInfo/DWARF/DWARFContext.cpp
819–851 ↗(On Diff #84747)

Yep, will do.

lib/ObjectYAML/DWARFEmitter.cpp
356–358 ↗(On Diff #84747)

Will do.

360–369 ↗(On Diff #84747)

Yes, I expect this to grow over time. Because the order and way in which debug sections are emitted can vary between object files there is no duplication of this in yaml2obj that would make sense to remove.

beanz added inline comments.Jan 18 2017, 2:11 PM
unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
1163–1177 ↗(On Diff #84747)

I looked at this. Using multi-line raw syntax is, IMO, undesirable for encoded yaml because yaml is whitespace-sensitive. Using the non-raw syntax the strings in the code can be formatted with clang-format and still look like well-formed yaml. With the raw syntax the indentation gets screwy.

beanz updated this revision to Diff 84893.Jan 18 2017, 3:22 PM

Updates based on feedback from dblaikie.

dblaikie accepted this revision.Jan 18 2017, 6:18 PM
dblaikie added a subscriber: lhames.

Looks good - I might experiment with raw string literals, see how it looks/feels personally at some point. Reckon it'd be easier to maintain, at least. (not having to wrap every line in "" and newlines, etc.

lib/DebugInfo/DWARF/DWARFContext.cpp
620–621 ↗(On Diff #84893)

You can roll the variable declaration into the 'if' here, if you like

787–788 ↗(On Diff #84893)

And here

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
1180 ↗(On Diff #84893)

Probably ASSERT_TRUE - if there is an error and you continue after this, it'll assert later from the Error not being handled which will probably be confusing.

@lhames might have some ideas about how to handle this sort of thing - I think it came up before/in other bits of Greg's work, so might be nice to have a solid idiom of "expect this to not Error" and log useful stuff about the error if it does fail, etc.

1182 ↗(On Diff #84893)

Can write this as '*ErrOrSections' if you like

This revision is now accepted and ready to land.Jan 18 2017, 6:18 PM
This revision was automatically updated to reflect the committed changes.
dblaikie added inline comments.Jan 20 2017, 11:30 AM
lib/DebugInfo/DWARF/DWARFContext.cpp
620–653 ↗(On Diff #84893)

Did this regress when you committed it (if I'm reading the patch/changes/Phabricator correctly) - looks like the StringSwitch came back, rather than using the common utility function, MapSectionToMember.

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
1187 ↗(On Diff #84893)

This should probably be an ASSERT - otherwise it'll crash/UB on the next line anyway.

Did you chat to Lang about the fact that this would produce a follow-on assert if it failed? (since the code doesn't handle the error, if there is one)

Perhaps something like HandleExpectedError would be suitable/necessary here.

1188 ↗(On Diff #84893)

Could write this as **Obj, if you like.

beanz added inline comments.Jan 20 2017, 3:46 PM
lib/DebugInfo/DWARF/DWARFContext.cpp
620–653 ↗(On Diff #84893)

I think the diff just shows up strange. There is only one copy of the StringSwitch in the code I see checked in.

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
1187 ↗(On Diff #84893)

Sorry, forgot to update this. Lang is OOO and not easily reachable. I'll chat with him when he gets back in the meantime I'll update to ASSERT and commit.