Page MenuHomePhabricator

[lldb-vscode] Add an option for loading core files
ClosedPublic

Authored by wallace on Apr 24 2020, 4:50 PM.

Details

Summary

Currently loading core files on lldb-vscode is broken because there's a check in the attach workflow that asserts that the PID is valid, which of course fails for this case.
Hence, I'm adding a "coreFile" argument for the attach request, which does the work correctly.

I have yet to make a test. So I'm asking for initial feedback in the meantime. I'll get some inspiration by the coredump tests in LLDB core.

This is a test debug config:

{
        "name": "Attach to coredump",
        "type": "fb-lldb",
        "request": "attach",
        "program": "/Users/wallace/Source/coredumptest/a.out",
        "coreFile": "/cores/core.33448",
}

And it worked on VSCode

Diff Detail

Event Timeline

wallace created this revision.Apr 24 2020, 4:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2020, 4:50 PM
wallace edited the summary of this revision. (Show Details)Apr 24 2020, 4:51 PM
wallace edited the summary of this revision. (Show Details)
wallace edited the summary of this revision. (Show Details)Apr 24 2020, 5:00 PM

You might look around for a core file in another test location and be able to use that. Functionality looks good to me. This could have been done with attachCommands before, but it is nice to have a built in and supported way to do this.

You might look around for a core file in another test location and be able to use that.

Yep, there's plenty to choose from in test/API/functionalities/postmortem/elf-core/.

lldb/tools/lldb-vscode/lldb-vscode.cpp
533–535

I can't tell what does this return from this snippet. I don't think this is a good use of auto.

576

Judging by this, it probably returns a StringRef. Is it always guaranteed to be null-terminated?

lldb/tools/lldb-vscode/package.json
226

Is the second part really true? lldb is able to open a core file without specifying the main executable (and in some cases it will even automatically find the matching executable).

wallace updated this revision to Diff 260501.Apr 27 2020, 5:01 PM

Added a test, which was easier than I thought!
I tested it on mac as well, so I hope it'll work well on the buildbots.

I also removed that bad use of auto. It seems that all StringRefs gotten from the JSON object are null-terminated.

labath accepted this revision.Apr 28 2020, 4:03 AM

Looks good. It may be interesting to tack a couple of resume/step-over/etc commands at the end of the test to make just the whole thing doesn't go berserk when those operations fail.

I also removed that bad use of auto. It seems that all StringRefs gotten from the JSON object are null-terminated.

Ok, json::parse is documented to return "self-contained" json object which owns its strings. It's not a big stretch to assume that those owned strings will also be null terminated, though it still seems moderately dangerous...

This revision is now accepted and ready to land.Apr 28 2020, 4:03 AM
wallace updated this revision to Diff 260731.Apr 28 2020, 12:27 PM

added the test suggested by @labath

This revision was automatically updated to reflect the committed changes.