[Lldb-commits] [PATCH] D69214: remove multi-argument form of PythonObject::Reset()

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna updated this revision to Diff 225775.
lawrence_danna added a comment.

comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69214/new/

https://reviews.llvm.org/D69214

Files:
  lldb/include/lldb/Interpreter/ScriptInterpreter.h
  lldb/scripts/Python/python-extensions.swig
  lldb/scripts/Python/python-typemaps.swig
  lldb/scripts/Python/python-wrapper.swig
  lldb/scripts/lldb.swig
  lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
  lldb/source/Plugins/ScriptInterpreter/Python/PythonExceptionState.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonExceptionState.h
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
  lldb/unittests/ScriptInterpreter/Python/CMakeLists.txt
  lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
  lldb/unittests/ScriptInterpreter/Python/PythonExceptionStateTests.cpp

Index: lldb/unittests/ScriptInterpreter/Python/PythonExceptionStateTests.cpp
===
--- lldb/unittests/ScriptInterpreter/Python/PythonExceptionStateTests.cpp
+++ /dev/null
@@ -1,164 +0,0 @@
-//===-- PythonExceptionStateTest.cpp --*- C++
-//-*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-
-#include "gtest/gtest.h"
-
-#include "Plugins/ScriptInterpreter/Python/PythonDataObjects.h"
-#include "Plugins/ScriptInterpreter/Python/PythonExceptionState.h"
-#include "Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h"
-#include "Plugins/ScriptInterpreter/Python/lldb-python.h"
-
-#include "PythonTestSuite.h"
-
-using namespace lldb_private;
-
-class PythonExceptionStateTest : public PythonTestSuite {
-public:
-protected:
-  void RaiseException() {
-PyErr_SetString(PyExc_RuntimeError, "PythonExceptionStateTest test error");
-  }
-};
-
-TEST_F(PythonExceptionStateTest, TestExceptionStateChecking) {
-  PyErr_Clear();
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  RaiseException();
-  EXPECT_TRUE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-}
-
-TEST_F(PythonExceptionStateTest, TestAcquisitionSemantics) {
-  PyErr_Clear();
-  PythonExceptionState no_error(false);
-  EXPECT_FALSE(no_error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-  RaiseException();
-  PythonExceptionState error(false);
-  EXPECT_TRUE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-  error.Discard();
-
-  PyErr_Clear();
-  RaiseException();
-  error.Acquire(false);
-  EXPECT_TRUE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-}
-
-TEST_F(PythonExceptionStateTest, TestDiscardSemantics) {
-  PyErr_Clear();
-
-  // Test that discarding an exception does not restore the exception
-  // state even when auto-restore==true is set
-  RaiseException();
-  PythonExceptionState error(true);
-  EXPECT_TRUE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  error.Discard();
-  EXPECT_FALSE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-}
-
-TEST_F(PythonExceptionStateTest, TestResetSemantics) {
-  PyErr_Clear();
-
-  // Resetting when auto-restore is true should restore.
-  RaiseException();
-  PythonExceptionState error(true);
-  EXPECT_TRUE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-  error.Reset();
-  EXPECT_FALSE(error.IsError());
-  EXPECT_TRUE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-
-  // Resetting when auto-restore is false should discard.
-  RaiseException();
-  PythonExceptionState error2(false);
-  EXPECT_TRUE(error2.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-  error2.Reset();
-  EXPECT_FALSE(error2.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-}
-
-TEST_F(PythonExceptionStateTest, TestManualRestoreSemantics) {
-  PyErr_Clear();
-  RaiseException();
-  PythonExceptionState error(false);
-  EXPECT_TRUE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  error.Restore();
-  EXPECT_FALSE(error.IsError());
-  EXPECT_TRUE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-}
-
-TEST_F(PythonExceptionStateTest, TestAutoRestoreSemantics) {
-  PyErr_Clear();
-  // Test that using the auto-restore flag correctly restores the exception
-  // state on destruction, and not using the auto-restore flag corre

[Lldb-commits] [PATCH] D69214: remove multi-argument form of PythonObject::Reset()

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna added inline comments.



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:261-264
 void PythonBytes::SetBytes(llvm::ArrayRef bytes) {
   const char *data = reinterpret_cast(bytes.data());
-  PyObject *py_bytes = PyBytes_FromStringAndSize(data, bytes.size());
-  PythonObject::Reset(PyRefType::Owned, py_bytes);
+  *this = Take(PyBytes_FromStringAndSize(data, bytes.size()));
 }

labath wrote:
> Do I understand correctly that the idea is to eventually replace these 
> functions with constructor calls too?
yea



Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:778
+  const char *script;
+  const char *function_name;
+  PythonCallable function;

labath wrote:
> I'm wondering if there's a way to avoid passing in the function name 
> argument. Perhaps if the callable is returned as a result of the script ("def 
> foo: ...; return foo")? WDYT?
I can't use return, but i can have the scripts put their function in a 
well-known output variable.



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:789-790
+  llvm::Expected operator()(Args &&... args) {
+llvm::Error e = Init();
+if (e)
+  return std::move(e);

labath wrote:
> if (llvm::Error e = ...)
Is that style considered normative?

I have a strong preference against putting function calls that are being run 
for their side effects inside an `if`.   



Comment at: 
lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp:789-802
+  Expected r = foo();
+
+  bool failed = !r;
+  ASSERT_TRUE(failed);
+
+  std::string backtrace;
+  llvm::handleAllErrors(r.takeError(), [&](const PythonException &E) {

labath wrote:
> Maybe:
> ```
> EXPECT_THAT_EXPECTED(foo(),
>   
> llvm::Failed(testing::Property(&PythonException::ReadBacktrace,
>  testing::ContainsRegex(...) )) ));
> ```
that doesn't seem to support multiple regexes, and is doing it 5 times really 
any more clear than just doing it manually?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69214/new/

https://reviews.llvm.org/D69214



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D69214: remove multi-argument form of PythonObject::Reset()

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna updated this revision to Diff 225767.
lawrence_danna marked 16 inline comments as done.
lawrence_danna added a comment.

fixes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69214/new/

https://reviews.llvm.org/D69214

Files:
  lldb/include/lldb/Interpreter/ScriptInterpreter.h
  lldb/scripts/Python/python-extensions.swig
  lldb/scripts/Python/python-typemaps.swig
  lldb/scripts/Python/python-wrapper.swig
  lldb/scripts/lldb.swig
  lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
  lldb/source/Plugins/ScriptInterpreter/Python/PythonExceptionState.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonExceptionState.h
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
  lldb/unittests/ScriptInterpreter/Python/CMakeLists.txt
  lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
  lldb/unittests/ScriptInterpreter/Python/PythonExceptionStateTests.cpp

Index: lldb/unittests/ScriptInterpreter/Python/PythonExceptionStateTests.cpp
===
--- lldb/unittests/ScriptInterpreter/Python/PythonExceptionStateTests.cpp
+++ /dev/null
@@ -1,164 +0,0 @@
-//===-- PythonExceptionStateTest.cpp --*- C++
-//-*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-
-#include "gtest/gtest.h"
-
-#include "Plugins/ScriptInterpreter/Python/PythonDataObjects.h"
-#include "Plugins/ScriptInterpreter/Python/PythonExceptionState.h"
-#include "Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h"
-#include "Plugins/ScriptInterpreter/Python/lldb-python.h"
-
-#include "PythonTestSuite.h"
-
-using namespace lldb_private;
-
-class PythonExceptionStateTest : public PythonTestSuite {
-public:
-protected:
-  void RaiseException() {
-PyErr_SetString(PyExc_RuntimeError, "PythonExceptionStateTest test error");
-  }
-};
-
-TEST_F(PythonExceptionStateTest, TestExceptionStateChecking) {
-  PyErr_Clear();
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  RaiseException();
-  EXPECT_TRUE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-}
-
-TEST_F(PythonExceptionStateTest, TestAcquisitionSemantics) {
-  PyErr_Clear();
-  PythonExceptionState no_error(false);
-  EXPECT_FALSE(no_error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-  RaiseException();
-  PythonExceptionState error(false);
-  EXPECT_TRUE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-  error.Discard();
-
-  PyErr_Clear();
-  RaiseException();
-  error.Acquire(false);
-  EXPECT_TRUE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-}
-
-TEST_F(PythonExceptionStateTest, TestDiscardSemantics) {
-  PyErr_Clear();
-
-  // Test that discarding an exception does not restore the exception
-  // state even when auto-restore==true is set
-  RaiseException();
-  PythonExceptionState error(true);
-  EXPECT_TRUE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  error.Discard();
-  EXPECT_FALSE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-}
-
-TEST_F(PythonExceptionStateTest, TestResetSemantics) {
-  PyErr_Clear();
-
-  // Resetting when auto-restore is true should restore.
-  RaiseException();
-  PythonExceptionState error(true);
-  EXPECT_TRUE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-  error.Reset();
-  EXPECT_FALSE(error.IsError());
-  EXPECT_TRUE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-
-  // Resetting when auto-restore is false should discard.
-  RaiseException();
-  PythonExceptionState error2(false);
-  EXPECT_TRUE(error2.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-  error2.Reset();
-  EXPECT_FALSE(error2.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-}
-
-TEST_F(PythonExceptionStateTest, TestManualRestoreSemantics) {
-  PyErr_Clear();
-  RaiseException();
-  PythonExceptionState error(false);
-  EXPECT_TRUE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  error.Restore();
-  EXPECT_FALSE(error.IsError());
-  EXPECT_TRUE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-}
-
-TEST_F(PythonExceptionStateTest, TestAutoRestoreSemantics) {
-  PyErr_Clear();
-  // Test that using the auto-restore flag correctly restores the exception
-  // state on destru

[Lldb-commits] [PATCH] D69133: eliminate nontrivial Reset(...) from TypedPythonObject

[Lawrence D'Anna via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG722b61892454: eliminate nontrivial Reset(...) from 
TypedPythonObject (authored by lawrence_danna).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69133/new/

https://reviews.llvm.org/D69133

Files:
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

Index: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
===
--- lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
+++ lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
@@ -27,8 +27,7 @@
   void SetUp() override {
 PythonTestSuite::SetUp();
 
-PythonString sys_module("sys");
-m_sys_module.Reset(PyRefType::Owned, PyImport_Import(sys_module.get()));
+m_sys_module = unwrapIgnoringErrors(PythonModule::Import("sys"));
 m_main_module = PythonModule::MainModule();
 m_builtins_module = PythonModule::BuiltinsModule();
   }
@@ -70,13 +69,10 @@
   PythonDictionary dict(PyInitialValue::Empty);
 
   PyObject *new_dict = PyDict_New();
-  dict.Reset(PyRefType::Owned, new_dict);
+  dict = Take(new_dict);
   EXPECT_EQ(new_dict, dict.get());
 
-  dict.Reset(PyRefType::Owned, nullptr);
-  EXPECT_EQ(nullptr, dict.get());
-
-  dict.Reset(PyRefType::Owned, PyDict_New());
+  dict = Take(PyDict_New());
   EXPECT_NE(nullptr, dict.get());
   dict.Reset();
   EXPECT_EQ(nullptr, dict.get());
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -55,6 +55,7 @@
 
 using namespace lldb;
 using namespace lldb_private;
+using namespace lldb_private::python;
 
 // Defined in the SWIG source file
 #if PY_MAJOR_VERSION >= 3
@@ -765,19 +766,16 @@
   if (!main_dict.IsValid())
 return m_session_dict;
 
-  PythonObject item = main_dict.GetItemForKey(PythonString(m_dictionary_name));
-  m_session_dict.Reset(PyRefType::Borrowed, item.get());
+  m_session_dict = unwrapIgnoringErrors(
+  As(main_dict.GetItem(m_dictionary_name)));
   return m_session_dict;
 }
 
 PythonDictionary &ScriptInterpreterPythonImpl::GetSysModuleDictionary() {
   if (m_sys_module_dict.IsValid())
 return m_sys_module_dict;
-
-  PythonObject sys_module(PyRefType::Borrowed, PyImport_AddModule("sys"));
-  if (sys_module.IsValid())
-m_sys_module_dict.Reset(PyRefType::Borrowed,
-PyModule_GetDict(sys_module.get()));
+  PythonModule sys_module = unwrapIgnoringErrors(PythonModule::Import("sys"));
+  m_sys_module_dict = sys_module.GetDictionary();
   return m_sys_module_dict;
 }
 
@@ -1053,9 +1051,8 @@
   PythonDictionary locals = GetSessionDictionary();
 
   if (!locals.IsValid()) {
-locals.Reset(
-PyRefType::Owned,
-PyObject_GetAttrString(globals.get(), m_dictionary_name.c_str()));
+locals = unwrapIgnoringErrors(
+As(globals.GetAttribute(m_dictionary_name)));
   }
 
   if (!locals.IsValid())
@@ -1204,9 +1201,8 @@
   PythonDictionary locals = GetSessionDictionary();
 
   if (!locals.IsValid())
-locals.Reset(
-PyRefType::Owned,
-PyObject_GetAttrString(globals.get(), m_dictionary_name.c_str()));
+locals = unwrapIgnoringErrors(
+As(globals.GetAttribute(m_dictionary_name)));
 
   if (!locals.IsValid())
 locals = globals;
Index: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
@@ -151,6 +151,30 @@
   return std::move(thing);
 }
 
+// This class can be used like a utility function to convert from
+// a llvm-friendly Twine into a null-terminated const char *,
+// which is the form python C APIs want their strings in.
+//
+// Example:
+// const llvm::Twine &some_twine;
+// PyFoo_Bar(x, y, z, NullTerminated(some_twine));
+//
+// Why a class instead of a function?  If the twine isn't already null
+// terminated, it will need a temporary buffer to copy the string
+// into.   We need that buffer to stick around for the lifetime of the
+// statement.
+class NullTerminated {
+  const char *str;
+  llvm::SmallString<32> storage;
+
+public:
+  NullTerminated(const llvm::Twine &twine) {
+llvm::StringRef ref = twine.toNullTerminatedStringRef(storage);
+str = ref.begin();
+  }
+  operator const char *() { return str; }
+};
+
 } // namespace python
 
 enum class PyInitialValue {

[Lldb-commits] [lldb] 722b618 - eliminate nontrivial Reset(...) from TypedPythonObject

[Lawrence D'Anna via lldb-commits]

Author: Lawrence D'Anna
Date: 2019-10-19T18:43:49Z
New Revision: 722b61892454b3217d73ec486e52156c5a92b5b3

URL: 
https://github.com/llvm/llvm-project/commit/722b61892454b3217d73ec486e52156c5a92b5b3
DIFF: 
https://github.com/llvm/llvm-project/commit/722b61892454b3217d73ec486e52156c5a92b5b3.diff

LOG: eliminate nontrivial Reset(...) from TypedPythonObject

Summary:
This deletes `Reset(...)`, except for the no-argument form `Reset()`
from `TypedPythonObject`, and therefore from `PythonString`, `PythonList`,
etc.

It updates the various callers to use assignment, `As<>`, `Take<>`,
and `Retain<>`, as appropriate.

followon to https://reviews.llvm.org/D69080

Reviewers: JDevlieghere, clayborg, labath, jingham

Reviewed By: labath

Subscribers: lldb-commits

Tags: #lldb

Differential Revision: https://reviews.llvm.org/D69133

llvm-svn: 375350

Added: 


Modified: 
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

Removed: 




diff  --git 
a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp 
b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
index 2b70762e368f..d0d593656efd 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
@@ -34,6 +34,7 @@ using namespace lldb_private::python;
 using llvm::cantFail;
 using llvm::Error;
 using llvm::Expected;
+using llvm::Twine;
 
 template <> Expected python::As(Expected &&obj) {
   if (!obj)
@@ -278,7 +279,7 @@ PythonByteArray::PythonByteArray(llvm::ArrayRef 
bytes)
 
 PythonByteArray::PythonByteArray(const uint8_t *bytes, size_t length) {
   const char *str = reinterpret_cast(bytes);
-  Reset(PyRefType::Owned, PyByteArray_FromStringAndSize(str, length));
+  *this = Take(PyByteArray_FromStringAndSize(str, length));
 }
 
 bool PythonByteArray::Check(PyObject *py_obj) {
@@ -522,11 +523,11 @@ StructuredData::BooleanSP 
PythonBoolean::CreateStructuredBoolean() const {
 
 PythonList::PythonList(PyInitialValue value) {
   if (value == PyInitialValue::Empty)
-Reset(PyRefType::Owned, PyList_New(0));
+*this = Take(PyList_New(0));
 }
 
 PythonList::PythonList(int list_size) {
-  Reset(PyRefType::Owned, PyList_New(list_size));
+  *this = Take(PyList_New(list_size));
 }
 
 bool PythonList::Check(PyObject *py_obj) {
@@ -578,11 +579,11 @@ StructuredData::ArraySP 
PythonList::CreateStructuredArray() const {
 
 PythonTuple::PythonTuple(PyInitialValue value) {
   if (value == PyInitialValue::Empty)
-Reset(PyRefType::Owned, PyTuple_New(0));
+*this = Take(PyTuple_New(0));
 }
 
 PythonTuple::PythonTuple(int tuple_size) {
-  Reset(PyRefType::Owned, PyTuple_New(tuple_size));
+  *this = Take(PyTuple_New(tuple_size));
 }
 
 PythonTuple::PythonTuple(std::initializer_list objects) {
@@ -649,7 +650,7 @@ StructuredData::ArraySP 
PythonTuple::CreateStructuredArray() const {
 
 PythonDictionary::PythonDictionary(PyInitialValue value) {
   if (value == PyInitialValue::Empty)
-Reset(PyRefType::Owned, PyDict_New());
+*this = Take(PyDict_New());
 }
 
 bool PythonDictionary::Check(PyObject *py_obj) {
@@ -696,10 +697,10 @@ PythonDictionary::GetItem(const PythonObject &key) const {
   return Retain(o);
 }
 
-Expected PythonDictionary::GetItem(const char *key) const {
+Expected PythonDictionary::GetItem(const Twine &key) const {
   if (!IsValid())
 return nullDeref();
-  PyObject *o = PyDict_GetItemString(m_py_obj, key);
+  PyObject *o = PyDict_GetItemString(m_py_obj, NullTerminated(key));
   if (PyErr_Occurred())
 return exception();
   if (!o)
@@ -717,11 +718,11 @@ Error PythonDictionary::SetItem(const PythonObject &key,
   return Error::success();
 }
 
-Error PythonDictionary::SetItem(const char *key,
+Error PythonDictionary::SetItem(const Twine &key,
 const PythonObject &value) const {
   if (!IsValid() || !value.IsValid())
 return nullDeref();
-  int r = PyDict_SetItemString(m_py_obj, key, value.get());
+  int r = PyDict_SetItemString(m_py_obj, NullTerminated(key), value.get());
   if (r < 0)
 return exception();
   return Error::success();
@@ -763,20 +764,20 @@ PythonModule PythonModule::AddModule(llvm::StringRef 
module) {
   return PythonModule(PyRefType::Borrowed, PyImport_AddModule(str.c_str()));
 }
 
-Expected PythonModule::Import(const char *name) {
-  PyObject *mod = PyImport_ImportModule(name);
+Expected PythonModule::Import(const Twine &name) {
+  PyObject *mod = PyImport_ImportModule(NullTerminated(name));
   if (!mod)
 return exception();
   return Take(mod);
 }
 
-Expected PythonModule::Get(const char *name) {
+Expected PythonModule::Get(const Twine &name) {
   if (

[Lldb-commits] [PATCH] D69210: [Disassembler] Simplify MCInst predicates

[Vedant Kumar via Phabricator via lldb-commits]

vsk added a comment.

Hm, this patch is bugging me.

It looks a bit like instructions are still decoded multiple times in different 
ways (e.g. in the `Decode` and `CalculateMnemonicOperandsAndComment` methods, 
which both modify `m_opcode`). Any ideas on whether/how to consolidate these?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69210/new/

https://reviews.llvm.org/D69210



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D69214: remove multi-argument form of PythonObject::Reset()

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna marked an inline comment as done.
lawrence_danna added inline comments.



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1057
+
+  // global is protected by the GIL
+  static PythonScript read_exception(read_exception_script, "read_exception");

labath wrote:
> I just realized I don't actually know what this means. Is it that the object 
> takes the GIL internally? Or that one must have the GIL taken before he 
> starts to interact with it? Maybe it'd be better to clarify this in the class 
> description and just delete these comments.
Almost everything in PythonDataObjects requires you to call it with the GIL 
already locked.  The only things that don't are the File methods because 
they're called by things that don't know anything about python.

Normally when I need to do something like this I would use dispatch_once, or 
whatever the LLVM version of that is, so you avoid static initializers, but 
also initialize the thing in a thread-safe way.

But in this case we already know that this function can only be called by a 
thread that has the GIL, so it's safe to just initialize a global variable 
without further synchronization.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69214/new/

https://reviews.llvm.org/D69214



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D69133: eliminate nontrivial Reset(...) from TypedPythonObject

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna updated this revision to Diff 225758.
lawrence_danna added a comment.

comment on NullTerminated


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69133/new/

https://reviews.llvm.org/D69133

Files:
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

Index: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
===
--- lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
+++ lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
@@ -27,8 +27,7 @@
   void SetUp() override {
 PythonTestSuite::SetUp();
 
-PythonString sys_module("sys");
-m_sys_module.Reset(PyRefType::Owned, PyImport_Import(sys_module.get()));
+m_sys_module = unwrapIgnoringErrors(PythonModule::Import("sys"));
 m_main_module = PythonModule::MainModule();
 m_builtins_module = PythonModule::BuiltinsModule();
   }
@@ -70,13 +69,10 @@
   PythonDictionary dict(PyInitialValue::Empty);
 
   PyObject *new_dict = PyDict_New();
-  dict.Reset(PyRefType::Owned, new_dict);
+  dict = Take(new_dict);
   EXPECT_EQ(new_dict, dict.get());
 
-  dict.Reset(PyRefType::Owned, nullptr);
-  EXPECT_EQ(nullptr, dict.get());
-
-  dict.Reset(PyRefType::Owned, PyDict_New());
+  dict = Take(PyDict_New());
   EXPECT_NE(nullptr, dict.get());
   dict.Reset();
   EXPECT_EQ(nullptr, dict.get());
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -55,6 +55,7 @@
 
 using namespace lldb;
 using namespace lldb_private;
+using namespace lldb_private::python;
 
 // Defined in the SWIG source file
 #if PY_MAJOR_VERSION >= 3
@@ -765,19 +766,16 @@
   if (!main_dict.IsValid())
 return m_session_dict;
 
-  PythonObject item = main_dict.GetItemForKey(PythonString(m_dictionary_name));
-  m_session_dict.Reset(PyRefType::Borrowed, item.get());
+  m_session_dict = unwrapIgnoringErrors(
+  As(main_dict.GetItem(m_dictionary_name)));
   return m_session_dict;
 }
 
 PythonDictionary &ScriptInterpreterPythonImpl::GetSysModuleDictionary() {
   if (m_sys_module_dict.IsValid())
 return m_sys_module_dict;
-
-  PythonObject sys_module(PyRefType::Borrowed, PyImport_AddModule("sys"));
-  if (sys_module.IsValid())
-m_sys_module_dict.Reset(PyRefType::Borrowed,
-PyModule_GetDict(sys_module.get()));
+  PythonModule sys_module = unwrapIgnoringErrors(PythonModule::Import("sys"));
+  m_sys_module_dict = sys_module.GetDictionary();
   return m_sys_module_dict;
 }
 
@@ -1053,9 +1051,8 @@
   PythonDictionary locals = GetSessionDictionary();
 
   if (!locals.IsValid()) {
-locals.Reset(
-PyRefType::Owned,
-PyObject_GetAttrString(globals.get(), m_dictionary_name.c_str()));
+locals = unwrapIgnoringErrors(
+As(globals.GetAttribute(m_dictionary_name)));
   }
 
   if (!locals.IsValid())
@@ -1204,9 +1201,8 @@
   PythonDictionary locals = GetSessionDictionary();
 
   if (!locals.IsValid())
-locals.Reset(
-PyRefType::Owned,
-PyObject_GetAttrString(globals.get(), m_dictionary_name.c_str()));
+locals = unwrapIgnoringErrors(
+As(globals.GetAttribute(m_dictionary_name)));
 
   if (!locals.IsValid())
 locals = globals;
Index: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
@@ -151,6 +151,30 @@
   return std::move(thing);
 }
 
+// This class can be used like a utility function to convert from
+// a llvm-friendly Twine into a null-terminated const char *,
+// which is the form python C APIs want their strings in.
+//
+// Example:
+// const llvm::Twine &some_twine;
+// PyFoo_Bar(x, y, z, NullTerminated(some_twine));
+//
+// Why a class instead of a function?  If the twine isn't already null
+// terminated, it will need a temporary buffer to copy the string
+// into.   We need that buffer to stick around for the lifetime of the
+// statement.
+class NullTerminated {
+  const char *str;
+  llvm::SmallString<32> storage;
+
+public:
+  NullTerminated(const llvm::Twine &twine) {
+llvm::StringRef ref = twine.toNullTerminatedStringRef(storage);
+str = ref.begin();
+  }
+  operator const char *() { return str; }
+};
+
 } // namespace python
 
 enum class PyInitialValue { Invalid, Empty };
@@ -323,10 +347,11 @@
 return python::Take(obj);
   }
 
-

[Lldb-commits] [PATCH] D69133: eliminate nontrivial Reset(...) from TypedPythonObject

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:154
 
+class NullTerminated {
+  const char *str;

Actually, could you also write a short blurb about what is the purpose of this 
class.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69133/new/

https://reviews.llvm.org/D69133



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D69214: remove multi-argument form of PythonObject::Reset()

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna updated this revision to Diff 225746.
lawrence_danna added a comment.

rebased


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69214/new/

https://reviews.llvm.org/D69214

Files:
  lldb/include/lldb/Interpreter/ScriptInterpreter.h
  lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
  lldb/source/Plugins/ScriptInterpreter/Python/PythonExceptionState.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonExceptionState.h
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
  lldb/unittests/ScriptInterpreter/Python/CMakeLists.txt
  lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
  lldb/unittests/ScriptInterpreter/Python/PythonExceptionStateTests.cpp

Index: lldb/unittests/ScriptInterpreter/Python/PythonExceptionStateTests.cpp
===
--- lldb/unittests/ScriptInterpreter/Python/PythonExceptionStateTests.cpp
+++ /dev/null
@@ -1,164 +0,0 @@
-//===-- PythonExceptionStateTest.cpp --*- C++
-//-*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-
-#include "gtest/gtest.h"
-
-#include "Plugins/ScriptInterpreter/Python/PythonDataObjects.h"
-#include "Plugins/ScriptInterpreter/Python/PythonExceptionState.h"
-#include "Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h"
-#include "Plugins/ScriptInterpreter/Python/lldb-python.h"
-
-#include "PythonTestSuite.h"
-
-using namespace lldb_private;
-
-class PythonExceptionStateTest : public PythonTestSuite {
-public:
-protected:
-  void RaiseException() {
-PyErr_SetString(PyExc_RuntimeError, "PythonExceptionStateTest test error");
-  }
-};
-
-TEST_F(PythonExceptionStateTest, TestExceptionStateChecking) {
-  PyErr_Clear();
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  RaiseException();
-  EXPECT_TRUE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-}
-
-TEST_F(PythonExceptionStateTest, TestAcquisitionSemantics) {
-  PyErr_Clear();
-  PythonExceptionState no_error(false);
-  EXPECT_FALSE(no_error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-  RaiseException();
-  PythonExceptionState error(false);
-  EXPECT_TRUE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-  error.Discard();
-
-  PyErr_Clear();
-  RaiseException();
-  error.Acquire(false);
-  EXPECT_TRUE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-}
-
-TEST_F(PythonExceptionStateTest, TestDiscardSemantics) {
-  PyErr_Clear();
-
-  // Test that discarding an exception does not restore the exception
-  // state even when auto-restore==true is set
-  RaiseException();
-  PythonExceptionState error(true);
-  EXPECT_TRUE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  error.Discard();
-  EXPECT_FALSE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-}
-
-TEST_F(PythonExceptionStateTest, TestResetSemantics) {
-  PyErr_Clear();
-
-  // Resetting when auto-restore is true should restore.
-  RaiseException();
-  PythonExceptionState error(true);
-  EXPECT_TRUE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-  error.Reset();
-  EXPECT_FALSE(error.IsError());
-  EXPECT_TRUE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-
-  // Resetting when auto-restore is false should discard.
-  RaiseException();
-  PythonExceptionState error2(false);
-  EXPECT_TRUE(error2.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-  error2.Reset();
-  EXPECT_FALSE(error2.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-}
-
-TEST_F(PythonExceptionStateTest, TestManualRestoreSemantics) {
-  PyErr_Clear();
-  RaiseException();
-  PythonExceptionState error(false);
-  EXPECT_TRUE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  error.Restore();
-  EXPECT_FALSE(error.IsError());
-  EXPECT_TRUE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-}
-
-TEST_F(PythonExceptionStateTest, TestAutoRestoreSemantics) {
-  PyErr_Clear();
-  // Test that using the auto-restore flag correctly restores the exception
-  // state on destruction, and not using the auto-restore flag correctly
-  // does NOT restore the state on destruction.
-  {
-RaiseException();
-PythonExceptionState error(false);
-EXPECT_TRUE(error.IsError())

[Lldb-commits] [PATCH] D69214: remove multi-argument form of PythonObject::Reset()

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

No fudnamental issues with this patch (I'm ignoring the CStringArg topic here). 
I do think that it would be nice to have some more unit tests for the new APIs 
you're introducing (or elevating to API status). I'm mainly thinking of the 
PythonScript stuff (a test where we evaluate the script successfully) and the 
runString functions.




Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:68-70
+/* If this is true then any exceptions raised by the script will be
+ * cleared with PyErr_Clear().   If false then they will be left for
+ * the caller to clean up */

I should've mentioned this earlier, but llvm style is to generally use `//` 
comments .,



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:261-264
 void PythonBytes::SetBytes(llvm::ArrayRef bytes) {
   const char *data = reinterpret_cast(bytes.data());
-  PyObject *py_bytes = PyBytes_FromStringAndSize(data, bytes.size());
-  PythonObject::Reset(PyRefType::Owned, py_bytes);
+  *this = Take(PyBytes_FromStringAndSize(data, bytes.size()));
 }

Do I understand correctly that the idea is to eventually replace these 
functions with constructor calls too?



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1057
+
+  // global is protected by the GIL
+  static PythonScript read_exception(read_exception_script, "read_exception");

I just realized I don't actually know what this means. Is it that the object 
takes the GIL internally? Or that one must have the GIL taken before he starts 
to interact with it? Maybe it'd be better to clarify this in the class 
description and just delete these comments.



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1064-1065
+  if (!backtrace) {
+llvm::consumeError(backtrace.takeError());
+return "backtrace unavailable";
+  }

maybe just return `toString(backtrace.takeError())` here?



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1512
+Error PythonScript::Init() {
+  if (!function.IsValid()) {
+PythonDictionary globals(PyInitialValue::Empty);

Use early return here (`if (valid) return Success;`)



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1516-1517
+auto builtins = PythonModule::BuiltinsModule();
+Error error = globals.SetItem("__builtins__", builtins);
+if (error)
+  return error;

if (Error error = ...)



Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:331
   const T &... t) const {
+using namespace python;
 const char format[] = {'(', PythonFormat::format..., ')', 0};

Maybe it's time to move the entire PythonObject hierarchy into the python 
namespace? That would be consistent with some of the newer lldb plugins, which 
have all of their code inside a sub-namespace of lldb_private..



Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:778
+  const char *script;
+  const char *function_name;
+  PythonCallable function;

I'm wondering if there's a way to avoid passing in the function name argument. 
Perhaps if the callable is returned as a result of the script ("def foo: ...; 
return foo")? WDYT?



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:789-790
+  llvm::Expected operator()(Args &&... args) {
+llvm::Error e = Init();
+if (e)
+  return std::move(e);

if (llvm::Error e = ...)



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp:1169-1180
+Status error;
+llvm::handleAllErrors(
+return_value.takeError(),
+[&](PythonException &E) {
+  error.SetErrorString(E.ReadBacktrace());
+  if (!options.GetMaskoutErrors())
+E.Restore();

Would something like this also work?
```
llvm::Error error = handleErrors(return_value.takeError(), [](PythonException 
&E) {
  llvm::Error str = makeStringError(E.ReadBacktrace());
  if (!options.GetMaskoutErrors)
E.Restore();
  return str;
});
return std::move(error);
```



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp:1986-1987
+reply_pyobj = Take(setting);
+  else
+reply_pyobj.Reset();
 

If doesn't look like this Reset call is really needed here..



Comment at: 
lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp:789-802
+  Expected r = foo();
+
+  bool failed = !r;
+  ASSERT_TRUE(failed);
+
+  std::string backtrace;
+  llvm::han

[Lldb-commits] [PATCH] D69133: eliminate nontrivial Reset(...) from TypedPythonObject

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna updated this revision to Diff 225745.
lawrence_danna added a comment.

fixed


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69133/new/

https://reviews.llvm.org/D69133

Files:
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

Index: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
===
--- lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
+++ lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
@@ -27,8 +27,7 @@
   void SetUp() override {
 PythonTestSuite::SetUp();
 
-PythonString sys_module("sys");
-m_sys_module.Reset(PyRefType::Owned, PyImport_Import(sys_module.get()));
+m_sys_module = unwrapIgnoringErrors(PythonModule::Import("sys"));
 m_main_module = PythonModule::MainModule();
 m_builtins_module = PythonModule::BuiltinsModule();
   }
@@ -70,13 +69,10 @@
   PythonDictionary dict(PyInitialValue::Empty);
 
   PyObject *new_dict = PyDict_New();
-  dict.Reset(PyRefType::Owned, new_dict);
+  dict = Take(new_dict);
   EXPECT_EQ(new_dict, dict.get());
 
-  dict.Reset(PyRefType::Owned, nullptr);
-  EXPECT_EQ(nullptr, dict.get());
-
-  dict.Reset(PyRefType::Owned, PyDict_New());
+  dict = Take(PyDict_New());
   EXPECT_NE(nullptr, dict.get());
   dict.Reset();
   EXPECT_EQ(nullptr, dict.get());
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -55,6 +55,7 @@
 
 using namespace lldb;
 using namespace lldb_private;
+using namespace lldb_private::python;
 
 // Defined in the SWIG source file
 #if PY_MAJOR_VERSION >= 3
@@ -765,19 +766,16 @@
   if (!main_dict.IsValid())
 return m_session_dict;
 
-  PythonObject item = main_dict.GetItemForKey(PythonString(m_dictionary_name));
-  m_session_dict.Reset(PyRefType::Borrowed, item.get());
+  m_session_dict = unwrapIgnoringErrors(
+  As(main_dict.GetItem(m_dictionary_name)));
   return m_session_dict;
 }
 
 PythonDictionary &ScriptInterpreterPythonImpl::GetSysModuleDictionary() {
   if (m_sys_module_dict.IsValid())
 return m_sys_module_dict;
-
-  PythonObject sys_module(PyRefType::Borrowed, PyImport_AddModule("sys"));
-  if (sys_module.IsValid())
-m_sys_module_dict.Reset(PyRefType::Borrowed,
-PyModule_GetDict(sys_module.get()));
+  PythonModule sys_module = unwrapIgnoringErrors(PythonModule::Import("sys"));
+  m_sys_module_dict = sys_module.GetDictionary();
   return m_sys_module_dict;
 }
 
@@ -1053,9 +1051,8 @@
   PythonDictionary locals = GetSessionDictionary();
 
   if (!locals.IsValid()) {
-locals.Reset(
-PyRefType::Owned,
-PyObject_GetAttrString(globals.get(), m_dictionary_name.c_str()));
+locals = unwrapIgnoringErrors(
+As(globals.GetAttribute(m_dictionary_name)));
   }
 
   if (!locals.IsValid())
@@ -1204,9 +1201,8 @@
   PythonDictionary locals = GetSessionDictionary();
 
   if (!locals.IsValid())
-locals.Reset(
-PyRefType::Owned,
-PyObject_GetAttrString(globals.get(), m_dictionary_name.c_str()));
+locals = unwrapIgnoringErrors(
+As(globals.GetAttribute(m_dictionary_name)));
 
   if (!locals.IsValid())
 locals = globals;
Index: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
@@ -151,6 +151,18 @@
   return std::move(thing);
 }
 
+class NullTerminated {
+  const char *str;
+  llvm::SmallString<32> storage;
+
+public:
+  NullTerminated(const llvm::Twine &twine) {
+llvm::StringRef ref = twine.toNullTerminatedStringRef(storage);
+str = ref.begin();
+  }
+  operator const char *() { return str; }
+};
+
 } // namespace python
 
 enum class PyInitialValue { Invalid, Empty };
@@ -323,10 +335,11 @@
 return python::Take(obj);
   }
 
-  llvm::Expected GetAttribute(const char *name) const {
+  llvm::Expected GetAttribute(const llvm::Twine &name) const {
+using namespace python;
 if (!m_py_obj)
   return nullDeref();
-PyObject *obj = PyObject_GetAttrString(m_py_obj, name);
+PyObject *obj = PyObject_GetAttrString(m_py_obj, NullTerminated(name));
 if (!obj)
   return exception();
 return python::Take(obj);
@@ -392,10 +405,11 @@
   // This can be eliminated once we drop python 2 support.
   static void Convert(PyRefType &type, PyObject *&py_

[Lldb-commits] [PATCH] D69133: eliminate nontrivial Reset(...) from TypedPythonObject

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna added a comment.

In D69133#1715544 , @labath wrote:

> While I appreciate the desire to reduce "boilerplate", I also think there's a 
> lot of value in consistency


oh, yea somehow I thought going through the twine would still copy the string, 
I see you're right, it won't.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69133/new/

https://reviews.llvm.org/D69133



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D69133: eliminate nontrivial Reset(...) from TypedPythonObject

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

While I appreciate the desire to reduce "boilerplate", I also think there's a 
lot of value in consistency. None of the existing APIs in llvm do anything like 
this, and are perfectly happy to call toNullTerminatedStringRef manually. 
You've now introduced another string-like type, which people have to figure out 
how it works. If you really feel like you must avoid the extra line of code 
(because that's the only thing that this saves, really) then you could at least 
move the class into the implementation of the methods, so that at least the 
inferface is consistent with all other llvm functions taking an llvm::Twine. 
You could define an llvm::Twine constructor and a "const char *" conversion 
operator for this class, so that you can type 
`Py_whatever(NullTerminated(twine))`. This way we could also define a "char *" 
conversion operator on the class in the PY2 case, also avoiding the need for 
the py2_const_cast thingy.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69133/new/

https://reviews.llvm.org/D69133



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D68130: [lldb] Don't emit artificial constructor declarations as global functions

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D68130#1715433 , @teemperor wrote:

> Well, I'm fine with x-failing it on Linux, even though I guess at some point 
> someone (i.e., probably me) has to figure out why this stuff is broken in the 
> expression parser.


I said does**n't**. What I was trying to say is that if you xfail it on linux, 
the test fill start "failing" on all the configurations where if was succeeding 
before.

That said, I am quite surprised that this behaves nondeterministically, as the 
test is very hermetic. If I had to guess, I would actually say that this 
hermeticity is the problem. Synthesizing the default constructor and calling it 
probably generates some calls into the c++(abi) library. Depending on the 
system configuration (static/dynamic linking, -Wl,--as-needed, etc), the 
necessary external functions may or may not be available. My guess would be 
that ensuring the test creates at least one other (unrelated) object will 
ensure the c++ runtime support functions are all there and this test succeeds 
consistently. However, as it is succeeding on the machines I have around right 
now, I don't have a way to verify this hypothesis.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68130/new/

https://reviews.llvm.org/D68130



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D69153: convert LLDBSwigPythonCallTypeScript to ArgInfo::max_positional_args

[Lawrence D'Anna via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbdcad0aca0a0: convert LLDBSwigPythonCallTypeScript to 
ArgInfo::max_positional_args (authored by lawrence_danna).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69153/new/

https://reviews.llvm.org/D69153

Files:
  
lldb/packages/Python/lldbsuite/test/commands/expression/formatters/TestFormatters.py
  
lldb/packages/Python/lldbsuite/test/commands/expression/formatters/formatters.py
  lldb/scripts/Python/python-wrapper.swig


Index: lldb/scripts/Python/python-wrapper.swig
===
--- lldb/scripts/Python/python-wrapper.swig
+++ lldb/scripts/Python/python-wrapper.swig
@@ -163,14 +163,19 @@
 }
 
 PythonObject result;
-auto argc = pfunc.GetNumArguments();
-// if the third argument is supported, or varargs are allowed
+auto argc = pfunc.GetArgInfo();
+if (!argc) {
+llvm::consumeError(argc.takeError());
+return false;
+}
+
 PythonObject value_arg(PyRefType::Owned, SBTypeToSWIGWrapper(sb_value));
 PythonObject options_arg(PyRefType::Owned, 
SBTypeToSWIGWrapper(sb_options));
-if (argc.count == 3 || argc.has_varargs)
-result = pfunc(value_arg,dict,options_arg);
-else
+
+if (argc.get().max_positional_args < 3)
 result = pfunc(value_arg,dict);
+else
+result = pfunc(value_arg,dict,options_arg);
 
 retval = result.Str().GetString().str();
 
Index: 
lldb/packages/Python/lldbsuite/test/commands/expression/formatters/formatters.py
===
--- 
lldb/packages/Python/lldbsuite/test/commands/expression/formatters/formatters.py
+++ 
lldb/packages/Python/lldbsuite/test/commands/expression/formatters/formatters.py
@@ -1,3 +1,5 @@
+import lldb
+
 def foo_SummaryProvider(valobj, dict):
 a = valobj.GetChildMemberWithName('a')
 a_ptr = valobj.GetChildMemberWithName('a_ptr')
@@ -15,3 +17,8 @@
 ', i_ptr = ' + str(i_ptr.GetValueAsUnsigned(0)) + ' -> ' + 
str(i_ptr.Dereference().GetValueAsUnsigned(0)) + \
 ', b_ref = ' + str(b_ref.GetValueAsUnsigned(0)) + \
 ', h = ' + str(h.GetValueAsUnsigned(0)) + ' , k = ' + 
str(k.GetValueAsUnsigned(0))
+
+def foo_SummaryProvider3(valobj, dict, options):
+if not isinstance(options, lldb.SBTypeSummaryOptions):
+raise Exception()
+return foo_SummaryProvider(valobj, dict) + ", WITH_OPTS"
\ No newline at end of file
Index: 
lldb/packages/Python/lldbsuite/test/commands/expression/formatters/TestFormatters.py
===
--- 
lldb/packages/Python/lldbsuite/test/commands/expression/formatters/TestFormatters.py
+++ 
lldb/packages/Python/lldbsuite/test/commands/expression/formatters/TestFormatters.py
@@ -74,6 +74,21 @@
 # EXPR-TYPES-NEW-FOO-NEXT:   }
 # EXPR-TYPES-NEW-FOO-NEXT: }
 
+
+self.runCmd("type summary add -F formatters.foo_SummaryProvider3 foo")
+self.filecheck("expression foo1", __file__, 
'-check-prefix=EXPR-FOO1opts')
+# EXPR-FOO1opts: (foo) $
+# EXPR-FOO1opts-SAME: a = 12
+# EXPR-FOO1opts-SAME: a_ptr = {{[0-9]+}} -> 13
+# EXPR-FOO1opts-SAME: i = 24
+# EXPR-FOO1opts-SAME: i_ptr = {{[0-9]+}} -> 25
+# EXPR-FOO1opts-SAME: b_ref = {{[0-9]+}}
+# EXPR-FOO1opts-SAME: h = 27
+# EXPR-FOO1opts-SAME: k = 29
+# EXPR-FOO1opts-SAME: WITH_OPTS
+
+self.runCmd("type summary delete foo")
+
 self.runCmd("type summary add -F formatters.foo_SummaryProvider foo")
 
 self.expect("expression new int(12)",


Index: lldb/scripts/Python/python-wrapper.swig
===
--- lldb/scripts/Python/python-wrapper.swig
+++ lldb/scripts/Python/python-wrapper.swig
@@ -163,14 +163,19 @@
 }
 
 PythonObject result;
-auto argc = pfunc.GetNumArguments();
-// if the third argument is supported, or varargs are allowed
+auto argc = pfunc.GetArgInfo();
+if (!argc) {
+llvm::consumeError(argc.takeError());
+return false;
+}
+
 PythonObject value_arg(PyRefType::Owned, SBTypeToSWIGWrapper(sb_value));
 PythonObject options_arg(PyRefType::Owned, SBTypeToSWIGWrapper(sb_options));
-if (argc.count == 3 || argc.has_varargs)
-result = pfunc(value_arg,dict,options_arg);
-else
+
+if (argc.get().max_positional_args < 3)
 result = pfunc(value_arg,dict);
+else
+result = pfunc(value_arg,dict,options_arg);
 
 retval = result.Str().GetString().str();
 
Index: lldb/packages/Python/lldbsuite/test/commands/expression/formatters/formatters.py
===
--- lldb/packages/Python/lldbsuite/test/commands/expression/formatters/formatters.py
+++ lldb/packages/Python/lldbsuite/test/command

[Lldb-commits] [PATCH] D69014: [LLDB] bugfix: command script add -f doesn't work for some callables

[Lawrence D'Anna via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2386537c2469: [LLDB] bugfix: command script add -f 
doesn't work for some callables (authored by lawrence_danna).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69014/new/

https://reviews.llvm.org/D69014

Files:
  
lldb/packages/Python/lldbsuite/test/commands/command/script/TestCommandScript.py
  lldb/packages/Python/lldbsuite/test/commands/command/script/callables.py
  lldb/packages/Python/lldbsuite/test/commands/command/script/py_import
  lldb/scripts/Python/python-wrapper.swig
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
  lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

Index: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
===
--- lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
+++ lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
@@ -643,8 +643,8 @@
 auto arginfo = lambda.GetArgInfo();
 ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
 EXPECT_EQ(arginfo.get().count, 1);
+EXPECT_EQ(arginfo.get().max_positional_args, 1u);
 EXPECT_EQ(arginfo.get().has_varargs, false);
-EXPECT_EQ(arginfo.get().is_bound_method, false);
   }
 
   {
@@ -655,8 +655,8 @@
 auto arginfo = lambda.GetArgInfo();
 ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
 EXPECT_EQ(arginfo.get().count, 2);
+EXPECT_EQ(arginfo.get().max_positional_args, 2u);
 EXPECT_EQ(arginfo.get().has_varargs, false);
-EXPECT_EQ(arginfo.get().is_bound_method, false);
   }
 
   {
@@ -667,6 +667,7 @@
 auto arginfo = lambda.GetArgInfo();
 ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
 EXPECT_EQ(arginfo.get().count, 2);
+EXPECT_EQ(arginfo.get().max_positional_args, 2u);
 EXPECT_EQ(arginfo.get().has_varargs, false);
   }
 
@@ -678,8 +679,9 @@
 auto arginfo = lambda.GetArgInfo();
 ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
 EXPECT_EQ(arginfo.get().count, 2);
+EXPECT_EQ(arginfo.get().max_positional_args,
+  PythonCallable::ArgInfo::UNBOUNDED);
 EXPECT_EQ(arginfo.get().has_varargs, true);
-EXPECT_EQ(arginfo.get().is_bound_method, false);
   }
 
   {
@@ -690,6 +692,8 @@
 auto arginfo = lambda.GetArgInfo();
 ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
 EXPECT_EQ(arginfo.get().count, 2);
+EXPECT_EQ(arginfo.get().max_positional_args,
+  PythonCallable::ArgInfo::UNBOUNDED);
 EXPECT_EQ(arginfo.get().has_varargs, true);
   }
 
@@ -698,7 +702,18 @@
 class Foo:
   def bar(self, x):
  return x
+  @classmethod
+  def classbar(cls, x):
+ return x
+  @staticmethod
+  def staticbar(x):
+ return x
+  def __call__(self, x):
+ return x
+obj = Foo()
 bar_bound   = Foo().bar
+bar_class   = Foo().classbar
+bar_static  = Foo().staticbar
 bar_unbound = Foo.bar
 )";
 PyObject *o =
@@ -711,16 +726,37 @@
 auto arginfo = bar_bound.get().GetArgInfo();
 ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
 EXPECT_EQ(arginfo.get().count, 2); // FIXME, wrong
+EXPECT_EQ(arginfo.get().max_positional_args, 1u);
 EXPECT_EQ(arginfo.get().has_varargs, false);
-EXPECT_EQ(arginfo.get().is_bound_method, true);
 
 auto bar_unbound = As(globals.GetItem("bar_unbound"));
 ASSERT_THAT_EXPECTED(bar_unbound, llvm::Succeeded());
 arginfo = bar_unbound.get().GetArgInfo();
 ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
 EXPECT_EQ(arginfo.get().count, 2);
+EXPECT_EQ(arginfo.get().max_positional_args, 2u);
+EXPECT_EQ(arginfo.get().has_varargs, false);
+
+auto bar_class = As(globals.GetItem("bar_class"));
+ASSERT_THAT_EXPECTED(bar_class, llvm::Succeeded());
+arginfo = bar_class.get().GetArgInfo();
+ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
+EXPECT_EQ(arginfo.get().max_positional_args, 1u);
+EXPECT_EQ(arginfo.get().has_varargs, false);
+
+auto bar_static = As(globals.GetItem("bar_static"));
+ASSERT_THAT_EXPECTED(bar_static, llvm::Succeeded());
+arginfo = bar_static.get().GetArgInfo();
+ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
+EXPECT_EQ(arginfo.get().max_positional_args, 1u);
+EXPECT_EQ(arginfo.get().has_varargs, false);
+
+auto obj = As(globals.GetItem("obj"));
+ASSERT_THAT_EXPECTED(obj, llvm::Succeeded());
+arginfo = obj.get().GetArgInfo();
+ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
+EXPECT_EQ(arginfo.get().max_positional_args, 1u);
 EXPECT_EQ(arginfo.get().has_varargs, false);
-EXPECT_EQ(arginfo.get().is_bound_method, false);
   }
 
 #if PY_MAJOR_VERSION >= 3 && PY_MINOR_VERSION >= 3
@@ -734,9 +770,9 @@
 auto arginfo = hex.get().GetArgInfo();
 ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
 EXPECT_EQ(arginfo.get()

[Lldb-commits] [lldb] bdcad0a - convert LLDBSwigPythonCallTypeScript to ArgInfo::max_positional_args

[Lawrence D'Anna via lldb-commits]

Author: Lawrence D'Anna
Date: 2019-10-19T07:05:39Z
New Revision: bdcad0aca0a05145364ee153a8f54af4aea2c445

URL: 
https://github.com/llvm/llvm-project/commit/bdcad0aca0a05145364ee153a8f54af4aea2c445
DIFF: 
https://github.com/llvm/llvm-project/commit/bdcad0aca0a05145364ee153a8f54af4aea2c445.diff

LOG: convert LLDBSwigPythonCallTypeScript to ArgInfo::max_positional_args

Summary:
This patch converts another user of ArgInfo::count over to
use ArgInfo::max_positional_args instead.   I also add a test
to make sure both documented signatures for python type formatters
work.

Reviewers: JDevlieghere, clayborg, labath, jingham

Reviewed By: labath

Subscribers: lldb-commits

Tags: #lldb

Differential Revision: https://reviews.llvm.org/D69153

llvm-svn: 375334

Added: 


Modified: 

lldb/packages/Python/lldbsuite/test/commands/expression/formatters/TestFormatters.py

lldb/packages/Python/lldbsuite/test/commands/expression/formatters/formatters.py
lldb/scripts/Python/python-wrapper.swig

Removed: 




diff  --git 
a/lldb/packages/Python/lldbsuite/test/commands/expression/formatters/TestFormatters.py
 
b/lldb/packages/Python/lldbsuite/test/commands/expression/formatters/TestFormatters.py
index b13d6555f33b..011dabce6e9d 100644
--- 
a/lldb/packages/Python/lldbsuite/test/commands/expression/formatters/TestFormatters.py
+++ 
b/lldb/packages/Python/lldbsuite/test/commands/expression/formatters/TestFormatters.py
@@ -74,6 +74,21 @@ def cleanup():
 # EXPR-TYPES-NEW-FOO-NEXT:   }
 # EXPR-TYPES-NEW-FOO-NEXT: }
 
+
+self.runCmd("type summary add -F formatters.foo_SummaryProvider3 foo")
+self.filecheck("expression foo1", __file__, 
'-check-prefix=EXPR-FOO1opts')
+# EXPR-FOO1opts: (foo) $
+# EXPR-FOO1opts-SAME: a = 12
+# EXPR-FOO1opts-SAME: a_ptr = {{[0-9]+}} -> 13
+# EXPR-FOO1opts-SAME: i = 24
+# EXPR-FOO1opts-SAME: i_ptr = {{[0-9]+}} -> 25
+# EXPR-FOO1opts-SAME: b_ref = {{[0-9]+}}
+# EXPR-FOO1opts-SAME: h = 27
+# EXPR-FOO1opts-SAME: k = 29
+# EXPR-FOO1opts-SAME: WITH_OPTS
+
+self.runCmd("type summary delete foo")
+
 self.runCmd("type summary add -F formatters.foo_SummaryProvider foo")
 
 self.expect("expression new int(12)",

diff  --git 
a/lldb/packages/Python/lldbsuite/test/commands/expression/formatters/formatters.py
 
b/lldb/packages/Python/lldbsuite/test/commands/expression/formatters/formatters.py
index dae84988af9e..ac2888bd203f 100644
--- 
a/lldb/packages/Python/lldbsuite/test/commands/expression/formatters/formatters.py
+++ 
b/lldb/packages/Python/lldbsuite/test/commands/expression/formatters/formatters.py
@@ -1,3 +1,5 @@
+import lldb
+
 def foo_SummaryProvider(valobj, dict):
 a = valobj.GetChildMemberWithName('a')
 a_ptr = valobj.GetChildMemberWithName('a_ptr')
@@ -15,3 +17,8 @@ def foo_SummaryProvider(valobj, dict):
 ', i_ptr = ' + str(i_ptr.GetValueAsUnsigned(0)) + ' -> ' + 
str(i_ptr.Dereference().GetValueAsUnsigned(0)) + \
 ', b_ref = ' + str(b_ref.GetValueAsUnsigned(0)) + \
 ', h = ' + str(h.GetValueAsUnsigned(0)) + ' , k = ' + 
str(k.GetValueAsUnsigned(0))
+
+def foo_SummaryProvider3(valobj, dict, options):
+if not isinstance(options, lldb.SBTypeSummaryOptions):
+raise Exception()
+return foo_SummaryProvider(valobj, dict) + ", WITH_OPTS"
\ No newline at end of file

diff  --git a/lldb/scripts/Python/python-wrapper.swig 
b/lldb/scripts/Python/python-wrapper.swig
index c50f9d1fab92..277b8657d344 100644
--- a/lldb/scripts/Python/python-wrapper.swig
+++ b/lldb/scripts/Python/python-wrapper.swig
@@ -163,14 +163,19 @@ LLDBSwigPythonCallTypeScript
 }
 
 PythonObject result;
-auto argc = pfunc.GetNumArguments();
-// if the third argument is supported, or varargs are allowed
+auto argc = pfunc.GetArgInfo();
+if (!argc) {
+llvm::consumeError(argc.takeError());
+return false;
+}
+
 PythonObject value_arg(PyRefType::Owned, SBTypeToSWIGWrapper(sb_value));
 PythonObject options_arg(PyRefType::Owned, 
SBTypeToSWIGWrapper(sb_options));
-if (argc.count == 3 || argc.has_varargs)
-result = pfunc(value_arg,dict,options_arg);
-else
+
+if (argc.get().max_positional_args < 3)
 result = pfunc(value_arg,dict);
+else
+result = pfunc(value_arg,dict,options_arg);
 
 retval = result.Str().GetString().str();
 



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 2386537 - [LLDB] bugfix: command script add -f doesn't work for some callables

[Lawrence D'Anna via lldb-commits]

Author: Lawrence D'Anna
Date: 2019-10-19T07:05:33Z
New Revision: 2386537c2469a97501a305c6b3138231b907a67f

URL: 
https://github.com/llvm/llvm-project/commit/2386537c2469a97501a305c6b3138231b907a67f
DIFF: 
https://github.com/llvm/llvm-project/commit/2386537c2469a97501a305c6b3138231b907a67f.diff

LOG: [LLDB] bugfix: command script add -f doesn't work for some callables

Summary:
When users define a debugger command from python, they provide a callable
object.   Because the signature of the function has been extended, LLDB
needs to inspect the number of parameters the callable can take.

The rule it was using to decide was weird, apparently not tested, and
giving wrong results for some kinds of python callables.

This patch replaces the weird rule with a simple one: if the callable can
take 5 arguments, it gets the 5 argument version of the signature.
Otherwise it gets the old 4 argument version.

It also adds tests with a bunch of different kinds of python callables
with both 4 and 5 arguments.

Reviewers: JDevlieghere, clayborg, labath, jingham

Reviewed By: labath

Subscribers: lldb-commits

Tags: #lldb

Differential Revision: https://reviews.llvm.org/D69014

llvm-svn: 375333

Added: 
lldb/packages/Python/lldbsuite/test/commands/command/script/callables.py

Modified: 

lldb/packages/Python/lldbsuite/test/commands/command/script/TestCommandScript.py
lldb/packages/Python/lldbsuite/test/commands/command/script/py_import
lldb/scripts/Python/python-wrapper.swig
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

Removed: 




diff  --git 
a/lldb/packages/Python/lldbsuite/test/commands/command/script/TestCommandScript.py
 
b/lldb/packages/Python/lldbsuite/test/commands/command/script/TestCommandScript.py
index 6531cd672792..9542d0264a6b 100644
--- 
a/lldb/packages/Python/lldbsuite/test/commands/command/script/TestCommandScript.py
+++ 
b/lldb/packages/Python/lldbsuite/test/commands/command/script/TestCommandScript.py
@@ -4,7 +4,7 @@
 
 from __future__ import print_function
 
-
+import sys
 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
@@ -22,6 +22,21 @@ def test(self):
 def pycmd_tests(self):
 self.runCmd("command source py_import")
 
+# Test a bunch of 
diff erent kinds of python callables with
+# both 4 and 5 positional arguments.
+self.expect("foobar", substrs=["All good"])
+self.expect("foobar4", substrs=["All good"])
+self.expect("vfoobar", substrs=["All good"])
+self.expect("v5foobar", substrs=["All good"])
+self.expect("sfoobar", substrs=["All good"])
+self.expect("cfoobar", substrs=["All good"])
+self.expect("ifoobar", substrs=["All good"])
+self.expect("sfoobar4", substrs=["All good"])
+self.expect("cfoobar4", substrs=["All good"])
+self.expect("ifoobar4", substrs=["All good"])
+self.expect("ofoobar", substrs=["All good"])
+self.expect("ofoobar4", substrs=["All good"])
+
 # Verify command that specifies eCommandRequiresTarget returns failure
 # without a target.
 self.expect('targetname',

diff  --git 
a/lldb/packages/Python/lldbsuite/test/commands/command/script/callables.py 
b/lldb/packages/Python/lldbsuite/test/commands/command/script/callables.py
new file mode 100644
index ..21e599b82e5b
--- /dev/null
+++ b/lldb/packages/Python/lldbsuite/test/commands/command/script/callables.py
@@ -0,0 +1,63 @@
+
+from __future__ import print_function
+
+import lldb
+
+# bunch of 
diff erent kinds of python callables that should
+# all work as commands.
+
+def check(debugger, command, context, result, internal_dict):
+if (not isinstance(debugger, lldb.SBDebugger) or
+not isinstance(command, str) or
+not isinstance(result, lldb.SBCommandReturnObject) or
+not isinstance(internal_dict, dict) or
+(not context is None and
+not isinstance(context, lldb.SBExecutionContext))):
+  raise Exception()
+result.AppendMessage("All good.")
+
+def vfoobar(*args):
+check(*args)
+
+def v5foobar(debugger, command, context, result, internal_dict, *args):
+check(debugger, command, context, result, internal_dict)
+
+def foobar(debugger, command, context, result, internal_dict):
+check(debugger, command, context, result, internal_dict)
+
+def foobar4(debugger, command, result, internal_dict):
+check(debugger, command, None, result, internal_dict)
+
+class FooBar:
+@staticmethod
+def sfoobar(debugger, command, context, result, internal_dict):
+  check(debugger, command, context, result, internal_dict)
+
+@classmethod
+def cfoobar(cls, debugger, command, context, result

[Lldb-commits] [PATCH] D69148: Disable exit-on-SIGPIPE in lldb

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D69148#1714785 , @vsk wrote:

> The death tests are flaky. I've noticed two issues:
>
> 1. When run under lit, the DisableExitOnSIGPIPE doesn't actually exit when it 
> receives SIGPIPE. Dtrace suggests that the unit test process inherits an 
> "ignore" sigaction from its parent.
> 2. When run in parallel, exiting causes the unit test to run atexit 
> destructors for other unit tests lumped into the SupportTests binary. One of 
> these is a condition_variable destructor and it hangs. Also, gtest warns: 
> `Death tests use fork(), which is unsafe particularly in a threaded context. 
> For this test, Google Test detected 21 threads.`
>
>   So I plan to give up on testing "EnableExitOnSIGPIPE" and will write a 
> non-death test to get some coverage.


gtest gives you a (somewhat rudimentary) mechanism of ensuring death tests run 
in a single threaded fashion. It consists of tacking "DeathTest" at the end of 
your test case name (like `TEST(SignalDeathTest, Whatever)`). These kinds of 
tests are run first, hopefully before the tests which spin up various other 
threads.

As for the parent "ignore" action (it's more likely to be a signal mask, 
actually), this can be reset with an appropriate sigprocmask(2) call.




Comment at: lldb/tools/driver/Driver.cpp:850
 #if !defined(_MSC_VER)
   signal(SIGPIPE, SIG_IGN);
   signal(SIGWINCH, sigwinch_handler);

I don't think this line does anything anymore.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69148/new/

https://reviews.llvm.org/D69148



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits