Page MenuHomePhabricator

[LLDB][breakpoints] ArgInfo::count -> ArgInfo::max_positional_args
ClosedPublic

Authored by lawrence_danna on Oct 26 2019, 2:50 PM.

Details

Summary

Move breakpoints from the old, bad ArgInfo::count to the new, better
ArgInfo::max_positional_args. Soon ArgInfo::count will be no more.

It looks like this functionality is already well tested by
TestBreakpointCommandsFromPython.py, so there's no need to write
additional tests for it.

Diff Detail

Event Timeline

lawrence_danna created this revision.Oct 26 2019, 2:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 26 2019, 2:50 PM
labath added inline comments.Oct 27 2019, 8:08 AM
lldb/scripts/Python/python-wrapper.swig
67

Is there any case where fetching the argument info will fail, but we still can successfully call the target object? Should we just bail out here?

lawrence_danna marked an inline comment as done.Oct 27 2019, 10:54 AM
lawrence_danna added inline comments.
lldb/scripts/Python/python-wrapper.swig
67

probably not? My thinking is that since there's no meaningful way to return an error from this function we may as well try to call it and let the exception get logged. But I dunno. Should I change it?

lawrence_danna marked an inline comment as done.Oct 27 2019, 4:10 PM
labath added inline comments.Oct 28 2019, 7:28 AM
lldb/scripts/Python/python-wrapper.swig
67

Hmm... In that case, I think it would be better to print the error which caused the ArgInfo fetching to fail. Given that, in the new way of doing things, the PyErr_Cleaner object is not going to work anyway, we might as well use this opportunity to create a different mechanism for printing exceptions.

improved error handling

lawrence_danna marked an inline comment as done.Oct 28 2019, 1:15 PM
labath accepted this revision.Oct 29 2019, 12:13 AM

Looks fine, though I'd try to remove the extern "C" thingy to avoid the need to supress warnings and stuff...

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
76–79

How sure are we that these functions need to be extern "C" ? AFAICT, the only requirement is that they match the declarations in ScriptInterpreterPython.cpp. That can be easily achieved by just removing extern "C" from both declarations.

2283

Random idea: Should we make PythonException::message() include the backtrace?

This revision is now accepted and ready to land.Oct 29 2019, 12:13 AM
lawrence_danna marked 4 inline comments as done.Oct 29 2019, 1:40 PM
lawrence_danna added inline comments.
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
76–79

Looks like SWIG forces them to be extern "C".

2283

Huh, I don't know. Maybe?

lawrence_danna marked 2 inline comments as done.Oct 29 2019, 1:41 PM
This revision was automatically updated to reflect the committed changes.