This is an archive of the discontinued LLVM Phabricator instance.

[lldb/crashlog] Surface error using SBCommandReturnObject argument
ClosedPublic

Authored by mib on Jul 12 2022, 6:29 PM.

Details

Summary

This patch allows the crashlog script to surface its errors to lldb by
using the provided SBCommandReturnObject argument.

rdar://95048193

Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>

Diff Detail

Event Timeline

mib created this revision.Jul 12 2022, 6:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2022, 6:29 PM
mib requested review of this revision.Jul 12 2022, 6:29 PM
JDevlieghere added inline comments.Jul 13 2022, 10:02 AM
lldb/examples/python/crashlog.py
1011–1014

Can we do this with a try-catch + an exception?

kastiglione added inline comments.
lldb/examples/python/crashlog.py
1012–1014

Is this error function needed? SetError prepends the string "error: ".

mib marked 2 inline comments as done.Jul 28 2022, 3:54 PM
mib added inline comments.
lldb/examples/python/crashlog.py
1011–1014

@JDevlieghere As mentioned on D129611, if we raise an exception we won't be able to surface the error to lldb or even to IDEs. I think it's better to use the SBCommandReturnObject

@kastiglione true! I'll remove the "error: " prefix, but I think it's reasonable to keep this helper function to avoid code duplication.

mib updated this revision to Diff 448465.Jul 28 2022, 3:54 PM
mib marked an inline comment as done.
JDevlieghere added inline comments.Aug 2 2022, 9:04 AM
lldb/examples/python/crashlog.py
1011–1014

Couldn't you still raise the error where it happens and then catch it and then take it's value and put it into the SBCommandReturnObject?

mib updated this revision to Diff 449472.Aug 2 2022, 4:23 PM

Address @JDevlieghere comments:

  • Use exception approach / Remove error helper function
  • Add test
kastiglione accepted this revision.Aug 3 2022, 10:10 AM

lgtm

lldb/examples/python/crashlog.py
1251–1253

minor but I think this can be a single statement: result.SetError(str(e))

This revision is now accepted and ready to land.Aug 3 2022, 10:10 AM
JDevlieghere requested changes to this revision.Aug 3 2022, 11:06 AM
JDevlieghere added inline comments.
lldb/examples/python/crashlog.py
1012–1014

Rather than raising a generic Exception, we should define our own. Now there's no way to differentiate between these issues and any other exception that was thrown. Maybe that's the goal, in which case you can still catch the generic Exception, but by defining our own we leave the option open to be more specific.

This revision now requires changes to proceed.Aug 3 2022, 11:06 AM
mib updated this revision to Diff 449784.Aug 3 2022, 2:22 PM
mib marked 3 inline comments as done.
  • Add custom InteractiveCrashLogException
  • Simplify the except block
This revision is now accepted and ready to land.Aug 3 2022, 4:02 PM