bulbazord added a comment.
Very interesting, I think this will be pretty useful for the ergonomics of the
SBAPI from python!
================
Comment at: lldb/bindings/python/python-typemaps.swig:74-75
+ if (!lldb_module) {
+ llvm::StringRef err_msg = llvm::toString(lldb_module.takeError());
+ PyErr_SetString(PyExc_TypeError, err_msg.data());
+ SWIG_fail;
----------------
Two things about this part:
1) `llvm::toString(Error)` returns a `std::string`, so taking a `StringRef` to
this thing is not safe. It's pointing to a temporary, the moment you get to
line 75 `err_msg` will be pointing to unmanaged memory.
2) `PyErr_SetString` takes a `const char *` as its second parameter. In
general, it is not safe to pass a StringRef's data field in these scenarios
because you can't always guarantee that it points to a null-terminated string.
If the StringRef is constructed from a std::string it's usually okay, but it's
difficult to reason about these kinds of things when you come across them.
================
Comment at: lldb/bindings/python/python-typemaps.swig:81-82
+ if (!sb_structured_data_class) {
+ llvm::StringRef err_msg =
llvm::toString(sb_structured_data_class.takeError());
+ PyErr_SetString(PyExc_TypeError, err_msg.data());
+ SWIG_fail;
----------------
Same as above here.
================
Comment at: lldb/bindings/python/python-typemaps.swig:91-92
+ if (!type) {
+ llvm::StringRef err_msg = llvm::toString(type.takeError());
+ PyErr_SetString(PyExc_TypeError, err_msg.data());
+ SWIG_fail;
----------------
Same here
================
Comment at: lldb/bindings/python/python-typemaps.swig:98-99
+ if (!type_name) {
+ llvm::StringRef err_msg = llvm::toString(type_name.takeError());
+ PyErr_SetString(PyExc_TypeError, err_msg.data());
+ SWIG_fail;
----------------
Same here
================
Comment at: lldb/bindings/python/python-typemaps.swig:104-105
+ if (llvm::StringRef(type_name.get()).startswith("SB")) {
+ // TODO: Add type name to error string.
+ PyErr_SetString(PyExc_TypeError, "input type is invalid");
+ SWIG_fail;
----------------
This may be as simple as:
```
std::string error_msg = "Input type is invalid: " + type_name.get();
PyErr_SetString(PyExc_TypeError, error_msg.c_str());
```
Right?
================
Comment at: lldb/bindings/python/python-typemaps.swig:108
+ } else {
+ $1 = $input;
+ }
----------------
Do we actually have to do any conversions here? Like to turn it into an
SBStructuredData from a python list or dictionary?
================
Comment at: lldb/include/lldb/API/SBStructuredData.h:34-36
+ static SBStructuredData
+ CreateFromScriptObject(const ScriptedTypedObject obj,
+ const lldb::SBDebugger &debugger);
----------------
Why do we offer both this and
`SBDebugger::CreateStructuredDataFromScriptObject`? They do the exact same
thing, when would you want to use one over the other?
================
Comment at: lldb/source/API/SBDebugger.cpp:1709-1710
+
+ ScriptInterpreter *interpreter =
+ m_opaque_sp->GetScriptInterpreter(true, obj.lang);
+
----------------
Can you document what `true` is supposed to represent here?
================
Comment at: lldb/source/API/SBStructuredData.cpp:51-53
+ const lldb::SBDebugger &debugger) {
+ return debugger.CreateStructuredDataFromScriptObject(obj);
+}
----------------
I already asked why this is necessary, but if it does stick around you'll want
to add `LLDB_INSTRUMENT_VA` here too yeah?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155161/new/
https://reviews.llvm.org/D155161
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
