Page MenuHomePhabricator

dsymutil support for DW_OP_convert

Authored by aprantl on Feb 21 2019, 5:12 PM.



So far I only got this testcase and it fails...

I'll update this as soon as I'm done implementing the missing pieces in dsymutil. We'll have to scan each DW_AT_location for DW_OP_convert DIE references and mark them as kept and then during cloning ensure to update the relative CU DIE offset if necessary.

Diff Detail


Event Timeline

aprantl created this revision.Feb 21 2019, 5:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2019, 5:12 PM
aprantl updated this revision to Diff 188020.Feb 22 2019, 5:50 PM

Here's the the first part of the patch: support for inline locations. Support for location lists is still coming.

aprantl updated this revision to Diff 188250.Feb 25 2019, 1:23 PM

Here's a first complete implementation.

In this form a ReleaseAsserts dsymutil run on a ReleaseAsserts+modules clang is about 5% slower:

# trunk
$ time dsymutil clang   # best out of 3 runs

real	0m46.861s
user	0m53.870s
sys	0m4.019s

# with this patch

real	0m48.995s
user	0m55.888s
sys	0m4.174s
markus added inline comments.Feb 25 2019, 11:38 PM
723 ↗(On Diff #188250)

Check if DWARFExpression::Operation::Encoding is BaseTypeRef instead.

1151 ↗(On Diff #188250)

Instead of checking for DW_OP_convert explicitly we could check the DWARFExpression::Operation::Encoding to see if it is BaseTypeRef.

Thanks for pointing out that DW_OP_convert isn't the only operation that uses a CU-relative type DIE offset!
I went through the DWARF 5 spec once more and it looks like all the operations that require a type parameter also require that that type parameter is a basic type. Since basic types are tiny, we can just keep them all unconditionally and skip the entire scanExpressionDIERefs part. With this change dsymutil gets more than 2 seconds faster on clang, basically almost the original performance.

aprantl updated this revision to Diff 188451.Feb 26 2019, 1:35 PM
aprantl retitled this revision from [WIP] dsymutil support for DW_OP_convert to dsymutil support for DW_OP_convert.

Faster version without the first scan expression pass.

Removing the WIP tag; this is now ready for a thorough review!

aprantl marked an inline comment as done.Feb 26 2019, 1:35 PM
friss added inline comments.Feb 26 2019, 4:09 PM
1100 ↗(On Diff #188451)

if you hit the error, then it looks like you're only going to emit NOPs instead of the ULEB. This is going to be badly misinterpreted by a consumer. You really want to NOP-out the whole convert expression.

markus added inline comments.Feb 26 2019, 11:07 PM
1100 ↗(On Diff #188451)

If the BaseTypeRef ULEB does not fit then I believe that you want to replace it (the BaseTypeRef ULEB) with zeros as that will be interpreted as a reference to the generic type according to the spec. I guess that replacing the entire operation and its operands with several DW_OP_nop would also be an option. Doing the former seems easier though and would arguably be more accurate in my opinion.

aprantl updated this revision to Diff 188565.Feb 27 2019, 9:09 AM

Address review feedback.

friss added a comment.Feb 27 2019, 1:54 PM

LGTM on the surface. I wouldn't mind someone else (@JDevlieghere ?) taking a look.

Few nits in addition to the stuff we discussed offline.

47 ↗(On Diff #188451)

nit/subjective: I prefer mayHaveLocationDescription

1066 ↗(On Diff #188565)

Let's do this at the beginning of the functions.

1071 ↗(On Diff #188565)

Can we invert this and do a continue?

203 ↗(On Diff #188451)

Can we use the endianness enum from Support/Endian.h?

368 ↗(On Diff #188451)

Can you include why these two are different in the doxygen comment?

Drive-by nit.

713 ↗(On Diff #188565)

DWARF v5? As this will be yet another place to update in future revisions.

aprantl updated this revision to Diff 188649.Feb 27 2019, 5:03 PM
aprantl marked 3 inline comments as done.

Got rid of most of the callbacks and per-byte copies.

aprantl marked an inline comment as done.Feb 27 2019, 5:04 PM
aprantl added inline comments.
1071 ↗(On Diff #188565)

No, there is an else path, too.

aprantl updated this revision to Diff 188650.Feb 27 2019, 5:08 PM
aprantl marked an inline comment as done.

Got rid of the second cloneExpression function.

aprantl updated this revision to Diff 188651.Feb 27 2019, 5:12 PM

Add a comment.

This revision is now accepted and ready to land.Feb 28 2019, 1:21 PM
This revision was automatically updated to reflect the committed changes.