Page MenuHomePhabricator

Adding support to step into the callable wrapped by libc++ std::function

Authored by shafik on Oct 3 2018, 4:13 PM.

Diff Detail


Event Timeline

shafik created this revision.Oct 3 2018, 4:13 PM
davide added a comment.Oct 3 2018, 4:24 PM

Some basic comments. Haven't looked at the implementation very closely, I'll do that probably tomorrow. Thanks for working on this!

59–60 ↗(On Diff #168196)

Please add a doxygen comment to this function. (I understand the surrounding ones are not commented, but we should start somewhere)/

1–3 ↗(On Diff #168196)

This comment seems a little outdated.

8–9 ↗(On Diff #168196)

I'm not sure you need these.

27–28 ↗(On Diff #168196)

Is this affected by the debug info variant used? (i.e. dSYM/gmodules/DWARF/dwo).
If not, this could be a NO_DEBUG_INFO test

56–73 ↗(On Diff #168196)

All these lines check hardcoded make me a little nervous, as they're really fragile. Can you make them parametric at least?
so that if somebody slides down the test case they're still valid (i.e. base + offset)

1–9 ↗(On Diff #168196)

You don't need a license for the test case, I think.

99–105 ↗(On Diff #168196)

Can you add a comment here?

shafik updated this revision to Diff 168389.Oct 4 2018, 3:08 PM
shafik marked 6 inline comments as done.

Addressing comments and fixing a bug in ThreadPlanStepThrough.cpp where I would overwrite the result of objc_runtime.

Just a couple of trivial requests, mostly about comments...

45–61 ↗(On Diff #168389)

Most of this setup (excepting the line_number calls) can be done in one line using lldbutils.run_to_source_breakpoint.

64 ↗(On Diff #168389)

Can you also check the source file? The way libcxx does inlining nowadays, all these std::function calls introduce lots of inlining so the chance that you got to the right line but in an inlined function is not zero...

27–28 ↗(On Diff #168196)

I don't think this test will be sensitive to those differences, so this probably can be a NO_DEBUG_INFO test.

295–301 ↗(On Diff #168389)

I'm not sure this comment is helpful. You want to do the trampoline detection regardless of the value of the regexp. And we really aren't violating that contract, since we don't intent to STOP on step into std::function...

329 ↗(On Diff #168389)

You need to explain what this step in is about. Otherwise this is pretty mysterious.

shafik updated this revision to Diff 169105.Oct 10 2018, 3:03 PM
shafik marked 5 inline comments as done.
  • Addressing comments
  • Adding test to make sure we step through a std::function wrapping a member variable

@jingham I believe I addressed your comments, please review again.

jingham requested changes to this revision.Oct 10 2018, 4:02 PM

Couple more lines you can delete from the test case, and I think you should make this a debug-variant insensitive test. Do that and this is good.

29–30 ↗(On Diff #169105)

You don't need these two lines anymore. You never use the exe variable, and the target you make with this "file" command is never used (you correctly use the one returned by run_to_source_breakpoint.)

Also, this doesn't seem like a debug variant sensitive test, so you really should make this a NO_DEBUG_INFO test. We're trying to do that wherever it makes sense just to keep down the number of tests we run.

This revision now requires changes to proceed.Oct 10 2018, 4:02 PM
shafik updated this revision to Diff 169259.Oct 11 2018, 11:16 AM
shafik marked an inline comment as done.

Removing uneeded code and making test NO_DEBUG_INFO_TESTCASE

@jingham removed code and made it NO_DEBUG_INFO_TESTCASE

jingham accepted this revision.Oct 11 2018, 11:37 AM

This looks great, thanks for making this work, it will be SO helpful for debugging std::function uses.

This revision is now accepted and ready to land.Oct 11 2018, 11:37 AM
davide accepted this revision.Oct 11 2018, 11:44 AM

LGTM too. Thanks for taking the time to address the comments.

This revision was automatically updated to reflect the committed changes.

This change has the same problem as D49271 - the class name LibCxxFunctionTestCase is re-used from another test case. This will cause issues when the tests report results and create logs. Please use unique names for each class and test.