[Lldb-commits] [PATCH] D155161: [lldb] Convert script native types to StructuredData counterpart

[Alex Langford via Phabricator via lldb-commits]

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