This is an archive of the discontinued LLVM Phabricator instance.

get rid of PythonInteger::GetInteger()
ClosedPublic

Authored by lawrence_danna on Apr 19 2020, 2:28 PM.

Details

Summary

One small step in my long running quest to improve python exception handling in
LLDB. Replace GetInteger() which just returns an int with As<long long> and
friends, which return Expected types that can track python exceptions

Diff Detail

Event Timeline

lawrence_danna created this revision.Apr 19 2020, 2:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2020, 2:28 PM
labath added inline comments.Apr 20 2020, 1:48 AM
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
55

obj->AsUnsignedLongLong() ?

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
3152–3162

This converts the Expected into a python exception, only to clear (and print) it at the next line. Is there a more direct way of doing it?

lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
125–133

EXPECT_THAT_EXPECTED(As<long long>(major_version_field), llvm::HasValue(PY_MAJOR_VERSION)) (and similarly in other tests too)

lawrence_danna marked 4 inline comments as done.

review fixes

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
3152–3162

I don't know, what did you have in mind? It could just call the Python C API directly and not bother converting it into an Expected, but would that be better?

labath accepted this revision.Apr 21 2020, 2:52 AM

Looks good. I'll leave it up to you to consider whether the PySys_WriteStderr thingy is a good idea.

lldb/bindings/python/python-typemaps.swig
77

it looks like this line needs wrapping

lldb/bindings/python/python-wrapper.swig
591–604

If you don't print this, then you could definitely do something like:

Expected<long long> result = As<long long>(pfunc.Call(PythonString(child_name));
if (result)
  return std::max<long long>(*result, 0);
consumeError(result.takeError());
return UINT32_MAX;
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
3152–3162

I was thinking if theres some function to print to stderr directly without doing the PyErr dance. PySys_WriteStderr maybe?

if (Expected<long long> py_return = As<long long>(implementor.CallMethod(callee_name)))
  result = *py_return;
else
  PySys_WriteStderr("%s", toString(py_return.takeError()).c_str());

?

This revision is now accepted and ready to land.Apr 21 2020, 2:52 AM
lawrence_danna marked 4 inline comments as done.

wrap long line

lldb/bindings/python/python-wrapper.swig
591–604

aparently this function is not even used... I'll put up a new diff to remove it

This revision was automatically updated to reflect the committed changes.
omjavaid reopened this revision.Apr 22 2020, 4:39 PM
omjavaid added a subscriber: omjavaid.

This causes multiple test failures on LLDB AArch64 Linux buildbot.
http://lab.llvm.org:8011/builders/lldb-aarch64-ubuntu/builds/3695

This revision is now accepted and ready to land.Apr 22 2020, 4:39 PM
omjavaid requested changes to this revision.Apr 22 2020, 4:42 PM

I have temporarily reverted this change to turn buildbot green.

This revision now requires changes to proceed.Apr 22 2020, 4:42 PM
This revision was not accepted when it landed; it landed in state Needs Revision.Apr 22 2020, 4:53 PM
This revision was automatically updated to reflect the committed changes.
omjavaid reopened this revision.Apr 23 2020, 3:07 AM
lawrence_danna updated this revision to Diff 259659.EditedApr 23 2020, 11:53 AM

fix python2 problems

@omjavaid sorry I didn't catch that on pre-submit testing. PyLong_AsLongLong and friends do not automatically convert on python2 like they do on python3. It's fixed now.

labath accepted this revision.Apr 24 2020, 4:41 AM

Still looks good.

omjavaid accepted this revision.Apr 27 2020, 3:47 AM

Thanks for getting this fixed. Looks good now.

This revision is now accepted and ready to land.Apr 27 2020, 3:47 AM
This revision was automatically updated to reflect the committed changes.