This is an archive of the discontinued LLVM Phabricator instance.

Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails
ClosedPublic

Authored by lemo on Jun 11 2018, 1:26 PM.

Details

Summary

There was no way to find out what's wrong if SBProcess SBTarget::LoadCore(const char *core_file) failed.
Additionally, the implementation was unconditionally setting sb_process, so it wasn't even possible to check if the return SBProcess is valid.

This change adds a new overload which surfaces the errors and also returns a valid SBProcess only if the core load succeeds:

SBProcess SBTarget::LoadCore(const char *core_file, SBError &error);

Diff Detail

Repository
rL LLVM

Event Timeline

lemo created this revision.Jun 11 2018, 1:26 PM
lemo updated this revision to Diff 150835.Jun 11 2018, 1:39 PM

Adding test cases for the new overload.

clayborg accepted this revision.Jun 11 2018, 1:46 PM

Remove extra spaces before ( and good to go.

scripts/interface/SBTarget.i
282 ↗(On Diff #150835)

remove space before (

285 ↗(On Diff #150835)

remove space before (

288 ↗(On Diff #150835)

remove space before (

This revision is now accepted and ready to land.Jun 11 2018, 1:46 PM

remove space before (

I'd be happy to make the change, but looking at the rest of the file it
seems that almost everything uses the opposite convention: Foo (...).

So, to make sure I'm making the right change, is this how it should look?

lldb::SBProcess
LoadCore(const char *core_file);

lldb::SBProcess
LoadCore(const char *core_file, lldb::SBError &error);
lemo updated this revision to Diff 150843.Jun 11 2018, 2:18 PM
lemo updated this revision to Diff 150844.
lemo marked 3 inline comments as done.

spaces removed :)

This revision was automatically updated to reflect the committed changes.
amccarth added inline comments.Jun 13 2018, 9:59 AM
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
78

Is it worth checking something more specific here? That the error indicates the file was not found?

lemo added a comment.Jun 13 2018, 10:40 AM

The intention in the scope of this change is just to check that the new
overload is exposed correctly through the Python API.

In general, guaranteeing specific error codes (messages?) is likely
impractical:

  1. It's not always easy to do the proper checks (ex. for 'file-not-found'

you'd actually get a null process, nothing more - see SBTarget::LoadCore()).

  1. It's unlikely that many clients need or want to check for specific

errors.

  1. Such a strict contract would be very fragile (ex. adding more specific

error condition detection would likely break the contract)

Perhaps ironically, I could take advantage of very specific error codes for
my scenario, but I don't think it's feasible. So for LoadCore() I'd like to
aim for more of a middle ground: accurate success/fail status + an
informative error content that can be used for diagnostics (logging) and
maybe as a weak hint.

What do you think?

My motivation was not to check for different kinds of errors but to ensure that the error code detail doesn't get lost through the SWIG plumbing. SBErrors seem to be marshaled properly when used as return values, but this is the first one I've seen returned by reference. The test ensures that the success/fail result makes it, but not that the detail (e.g., the error string) survives the trip.

It's probably not a big deal.

SBTarget.Attach, and Launch take SBError references, and they are pretty well tested. So I don't think that's a concern here.

We don't use the text content of error messages programmatically in lldb.

If you wanted to make these errors actionable, the low level LoadCore could map the errors that are representable as such to the appropriate Posix error number. Then you could get the error value, and reason based on that. We do that in a bunch of places, but always either in explicitly Posix code - or with the Mach error flavor in Mach specific code. It would be a little weird to return Posix FNF on a Windows host, however...

But that's well outside the scope of this patch. And given we don't do that now, I agree with Leonard, we shouldn't require some form for the error text here.