[Lldb-commits] [PATCH] D72086: [lldb] Fix that SBThread.GetStopDescription is returning strings with uninitialized memory at the end.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG61b6a4e82653: [lldb] Fix that SBThread.GetStopDescription is
returning strings with… (authored by teemperor).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72086/new/
https://reviews.llvm.org/D72086
Files:
lldb/bindings/interface/SBThread.i
lldb/bindings/python/python-typemaps.swig
lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
Index: lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
===
--- lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
+++ lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
@@ -122,14 +122,20 @@
self.assertTrue(
thread.IsValid(),
"There should be a thread stopped due to breakpoint")
-#self.runCmd("process status")
-
-# Due to the typemap magic (see lldb.swig), we pass in an (int)length
to GetStopDescription
-# and expect to get a Python string as the return object!
-# The 100 is just an arbitrary number specifying the buffer size.
-stop_description = thread.GetStopDescription(100)
-self.expect(stop_description, exe=False,
-startstr='breakpoint')
+
+# Get the stop reason. GetStopDescription expects that we pass in the
size of the description
+# we expect plus an additional byte for the null terminator.
+
+# Test with a buffer that is exactly as large as the expected stop
reason.
+self.assertEqual("breakpoint 1.1",
thread.GetStopDescription(len('breakpoint 1.1') + 1))
+
+# Test some smaller buffer sizes.
+self.assertEqual("breakpoint",
thread.GetStopDescription(len('breakpoint') + 1))
+self.assertEqual("break", thread.GetStopDescription(len('break') + 1))
+self.assertEqual("b", thread.GetStopDescription(len('b') + 1))
+
+# Test that we can pass in a much larger size and still get the right
output.
+self.assertEqual("breakpoint 1.1",
thread.GetStopDescription(len('breakpoint 1.1') + 100))
def step_out_of_malloc_into_function_b(self, exe_name):
"""Test Python SBThread.StepOut() API to step out of a malloc call
where the call site is at function b()."""
Index: lldb/bindings/python/python-typemaps.swig
===
--- lldb/bindings/python/python-typemaps.swig
+++ lldb/bindings/python/python-typemaps.swig
@@ -95,7 +95,6 @@
/* Typemap definitions to allow SWIG to properly handle char buffer. */
// typemap for a char buffer
-// See also SBThread::GetStopDescription.
%typemap(in) (char *dst, size_t dst_len) {
if (!PyInt_Check($input)) {
PyErr_SetString(PyExc_ValueError, "Expecting an integer");
@@ -113,7 +112,6 @@
%typemap(in) (void *char_buf, size_t size) = (char *dst, size_t dst_len);
// Return the char buffer. Discarding any previous return result
-// See also SBThread::GetStopDescription.
%typemap(argout) (char *dst, size_t dst_len) {
Py_XDECREF($result); /* Blow away any previous result */
if (result == 0) {
@@ -132,6 +130,28 @@
%typemap(argout) (void *char_buf, size_t size) = (char *dst, size_t dst_len);
+// typemap for handling an snprintf-like API like SBThread::GetStopDescription.
+%typemap(in) (char *dst_or_null, size_t dst_len) {
+ if (!PyInt_Check($input)) {
+ PyErr_SetString(PyExc_ValueError, "Expecting an integer");
+ return NULL;
+ }
+ $2 = PyInt_AsLong($input);
+ if ($2 <= 0) {
+ PyErr_SetString(PyExc_ValueError, "Positive integer expected");
+ return NULL;
+ }
+ $1 = (char *) malloc($2);
+}
+%typemap(argout) (char *dst_or_null, size_t dst_len) {
+ Py_XDECREF($result); /* Blow away any previous result */
+ llvm::StringRef ref($1);
+ PythonString string(ref);
+ $result = string.release();
+ free($1);
+}
+
+
// typemap for an outgoing buffer
// See also SBEvent::SBEvent(uint32_t event, const char *cstr, uint32_t
cstr_len).
// Ditto for SBProcess::PutSTDIN(const char *src, size_t src_len).
Index: lldb/bindings/interface/SBThread.i
===
--- lldb/bindings/interface/SBThread.i
+++ lldb/bindings/interface/SBThread.i
@@ -127,7 +127,7 @@
Pass only an (int)length and expect to get a Python string describing the
stop reason.") GetStopDescription;
size_t
-GetStopDescription (char *dst, size_t dst_len);
+GetStopDescription (char *dst_or_null, size_t dst_len);
SBValue
GetStopReturnValue ();
Index: lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
===
--- lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
+++
[Lldb-commits] [PATCH] D72086: [lldb] Fix that SBThread.GetStopDescription is returning strings with uninitialized memory at the end.
labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Overall, I think it would be nice to introduce some consistency into the return values of functions which fill out a user-provided buffer, but the thing which convinced me that a separate typemap is needed is utf8 handling. For functions like GetStopDescription, we should hopefully be able to guarantee that the returned value is a valid utf8 string (and so we can return a python string), but for things like GetSTOUT we most certainly cannot (a python `bytes` object would be more suitable)... CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72086/new/ https://reviews.llvm.org/D72086 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D72086: [lldb] Fix that SBThread.GetStopDescription is returning strings with uninitialized memory at the end.
teemperor added a comment. I made a new typemap now that is only for GetStopDescription and it's snprintf semantics. This keeps all the other functions working. I kept the old behavior of requiring a >0 buffer size etc. in tact for now. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72086/new/ https://reviews.llvm.org/D72086 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D72086: [lldb] Fix that SBThread.GetStopDescription is returning strings with uninitialized memory at the end.
teemperor updated this revision to Diff 237590.
teemperor added a comment.
- Move to use a new typemap for GetStopDescription.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72086/new/
https://reviews.llvm.org/D72086
Files:
lldb/bindings/interface/SBThread.i
lldb/bindings/python/python-typemaps.swig
lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
Index: lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
===
--- lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
+++ lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
@@ -122,14 +122,20 @@
self.assertTrue(
thread.IsValid(),
"There should be a thread stopped due to breakpoint")
-#self.runCmd("process status")
-
-# Due to the typemap magic (see lldb.swig), we pass in an (int)length
to GetStopDescription
-# and expect to get a Python string as the return object!
-# The 100 is just an arbitrary number specifying the buffer size.
-stop_description = thread.GetStopDescription(100)
-self.expect(stop_description, exe=False,
-startstr='breakpoint')
+
+# Get the stop reason. GetStopDescription expects that we pass in the
size of the description
+# we expect plus an additional byte for the null terminator.
+
+# Test with a buffer that is exactly as large as the expected stop
reason.
+self.assertEqual("breakpoint 1.1",
thread.GetStopDescription(len('breakpoint 1.1') + 1))
+
+# Test some smaller buffer sizes.
+self.assertEqual("breakpoint",
thread.GetStopDescription(len('breakpoint') + 1))
+self.assertEqual("break", thread.GetStopDescription(len('break') + 1))
+self.assertEqual("b", thread.GetStopDescription(len('b') + 1))
+
+# Test that we can pass in a much larger size and still get the right
output.
+self.assertEqual("breakpoint 1.1",
thread.GetStopDescription(len('breakpoint 1.1') + 100))
def step_out_of_malloc_into_function_b(self, exe_name):
"""Test Python SBThread.StepOut() API to step out of a malloc call
where the call site is at function b()."""
Index: lldb/bindings/python/python-typemaps.swig
===
--- lldb/bindings/python/python-typemaps.swig
+++ lldb/bindings/python/python-typemaps.swig
@@ -95,7 +95,6 @@
/* Typemap definitions to allow SWIG to properly handle char buffer. */
// typemap for a char buffer
-// See also SBThread::GetStopDescription.
%typemap(in) (char *dst, size_t dst_len) {
if (!PyInt_Check($input)) {
PyErr_SetString(PyExc_ValueError, "Expecting an integer");
@@ -113,7 +112,6 @@
%typemap(in) (void *char_buf, size_t size) = (char *dst, size_t dst_len);
// Return the char buffer. Discarding any previous return result
-// See also SBThread::GetStopDescription.
%typemap(argout) (char *dst, size_t dst_len) {
Py_XDECREF($result); /* Blow away any previous result */
if (result == 0) {
@@ -132,6 +130,28 @@
%typemap(argout) (void *char_buf, size_t size) = (char *dst, size_t dst_len);
+// typemap for handling an snprintf-like API like SBThread::GetStopDescription.
+%typemap(in) (char *dst_or_null, size_t dst_len) {
+ if (!PyInt_Check($input)) {
+ PyErr_SetString(PyExc_ValueError, "Expecting an integer");
+ return NULL;
+ }
+ $2 = PyInt_AsLong($input);
+ if ($2 <= 0) {
+ PyErr_SetString(PyExc_ValueError, "Positive integer expected");
+ return NULL;
+ }
+ $1 = (char *) malloc($2);
+}
+%typemap(argout) (char *dst_or_null, size_t dst_len) {
+ Py_XDECREF($result); /* Blow away any previous result */
+ llvm::StringRef ref($1);
+ PythonString string(ref);
+ $result = string.release();
+ free($1);
+}
+
+
// typemap for an outgoing buffer
// See also SBEvent::SBEvent(uint32_t event, const char *cstr, uint32_t
cstr_len).
// Ditto for SBProcess::PutSTDIN(const char *src, size_t src_len).
Index: lldb/bindings/interface/SBThread.i
===
--- lldb/bindings/interface/SBThread.i
+++ lldb/bindings/interface/SBThread.i
@@ -127,7 +127,7 @@
Pass only an (int)length and expect to get a Python string describing the
stop reason.") GetStopDescription;
size_t
-GetStopDescription (char *dst, size_t dst_len);
+GetStopDescription (char *dst_or_null, size_t dst_len);
SBValue
GetStopReturnValue ();
Index: lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
===
--- lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
+++ lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
@@ -122,14 +122,20 @@
self.assertTrue(
thread.IsValid(),
[Lldb-commits] [PATCH] D72086: [lldb] Fix that SBThread.GetStopDescription is returning strings with uninitialized memory at the end.
teemperor added a comment. Ah, seems like we match in the type map by function signature and GetSTDOUT matches the signature by accident CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72086/new/ https://reviews.llvm.org/D72086 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D72086: [lldb] Fix that SBThread.GetStopDescription is returning strings with uninitialized memory at the end.
teemperor added a comment. For reasons that are beyond my understanding this change seems to break `SBProcess.GetSTDOUT` in random tests like this: == ERROR: test_change_value_dwarf (TestChangeValueAPI.ChangeValueAPITestCase) Exercise the SBValue::SetValueFromCString API. -- Traceback (most recent call last): File "/home/teemperor/work/ci/llvm/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1732, in test_method return attrvalue(self) File "/home/teemperor/work/ci/llvm/lldb/packages/Python/lldbsuite/test/decorators.py", line 454, in wrapper func(*args, **kwargs) File "/home/teemperor/work/ci/llvm/lldb/packages/Python/lldbsuite/test/decorators.py", line 110, in wrapper func(*args, **kwargs) File "/home/teemperor/work/ci/llvm/lldb/packages/Python/lldbsuite/test/python_api/value/change_values/TestChangeValueAPI.py", line 149, in test_change_value stdout = process.GetSTDOUT(1000) File "/home/teemperor/work/ci/build/lib/python3.8/site-packages/lldb/__init__.py", line 8020, in GetSTDOUT return _lldb.SBProcess_GetSTDOUT(self, dst) SystemError: returned NULL without setting an error Config=x86_64-/home/teemperor/work/ci/build/bin/clang-10 Anyone got a clue what is going on here? It seems like these tests only fail when run in combination with some other tests as running them alone is fine. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72086/new/ https://reviews.llvm.org/D72086 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D72086: [lldb] Fix that SBThread.GetStopDescription is returning strings with uninitialized memory at the end.
teemperor updated this revision to Diff 236986.
teemperor added a comment.
- Just removed the result parameter.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72086/new/
https://reviews.llvm.org/D72086
Files:
lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
lldb/scripts/Python/python-typemaps.swig
Index: lldb/scripts/Python/python-typemaps.swig
===
--- lldb/scripts/Python/python-typemaps.swig
+++ lldb/scripts/Python/python-typemaps.swig
@@ -121,7 +121,7 @@
$result = string.release();
Py_INCREF($result);
} else {
- llvm::StringRef ref(static_cast($1), result);
+ llvm::StringRef ref(static_cast($1));
PythonString string(ref);
$result = string.release();
}
Index: lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
===
--- lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
+++ lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
@@ -122,14 +122,20 @@
self.assertTrue(
thread.IsValid(),
"There should be a thread stopped due to breakpoint")
-#self.runCmd("process status")
-
-# Due to the typemap magic (see lldb.swig), we pass in an (int)length
to GetStopDescription
-# and expect to get a Python string as the return object!
-# The 100 is just an arbitrary number specifying the buffer size.
-stop_description = thread.GetStopDescription(100)
-self.expect(stop_description, exe=False,
-startstr='breakpoint')
+
+# Get the stop reason. GetStopDescription expects that we pass in the
size of the description
+# we expect plus an additional byte for the null terminator.
+
+# Test with a buffer that is exactly as large as the expected stop
reason.
+self.assertEqual("breakpoint 1.1",
thread.GetStopDescription(len('breakpoint 1.1') + 1))
+
+# Test some smaller buffer sizes.
+self.assertEqual("breakpoint",
thread.GetStopDescription(len('breakpoint') + 1))
+self.assertEqual("break", thread.GetStopDescription(len('break') + 1))
+self.assertEqual("b", thread.GetStopDescription(len('b') + 1))
+
+# Test that we can pass in a much larger size and still get the right
output.
+self.assertEqual("breakpoint 1.1",
thread.GetStopDescription(len('breakpoint 1.1') + 100))
def step_out_of_malloc_into_function_b(self, exe_name):
"""Test Python SBThread.StepOut() API to step out of a malloc call
where the call site is at function b()."""
Index: lldb/scripts/Python/python-typemaps.swig
===
--- lldb/scripts/Python/python-typemaps.swig
+++ lldb/scripts/Python/python-typemaps.swig
@@ -121,7 +121,7 @@
$result = string.release();
Py_INCREF($result);
} else {
- llvm::StringRef ref(static_cast($1), result);
+ llvm::StringRef ref(static_cast($1));
PythonString string(ref);
$result = string.release();
}
Index: lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
===
--- lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
+++ lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
@@ -122,14 +122,20 @@
self.assertTrue(
thread.IsValid(),
"There should be a thread stopped due to breakpoint")
-#self.runCmd("process status")
-
-# Due to the typemap magic (see lldb.swig), we pass in an (int)length to GetStopDescription
-# and expect to get a Python string as the return object!
-# The 100 is just an arbitrary number specifying the buffer size.
-stop_description = thread.GetStopDescription(100)
-self.expect(stop_description, exe=False,
-startstr='breakpoint')
+
+# Get the stop reason. GetStopDescription expects that we pass in the size of the description
+# we expect plus an additional byte for the null terminator.
+
+# Test with a buffer that is exactly as large as the expected stop reason.
+self.assertEqual("breakpoint 1.1", thread.GetStopDescription(len('breakpoint 1.1') + 1))
+
+# Test some smaller buffer sizes.
+self.assertEqual("breakpoint", thread.GetStopDescription(len('breakpoint') + 1))
+self.assertEqual("break", thread.GetStopDescription(len('break') + 1))
+self.assertEqual("b", thread.GetStopDescription(len('b') + 1))
+
+# Test that we can pass in a much larger size and still get the right output.
+self.assertEqual("breakpoint 1.1", thread.GetStopDescription(len('breakpoint 1.1') + 100))
def step_out_of_malloc_into_function_b(self, exe_name):
"""Test Python
[Lldb-commits] [PATCH] D72086: [lldb] Fix that SBThread.GetStopDescription is returning strings with uninitialized memory at the end.
clayborg added inline comments.
Comment at: lldb/scripts/Python/python-typemaps.swig:124
} else {
- llvm::StringRef ref(static_cast($1), result);
+ llvm::StringRef ref = static_cast($1);
PythonString string(ref);
This assignment looks a bit goofy IMHO. Just remove result from original code?:
```
llvm::StringRef ref(static_cast($1));
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72086/new/
https://reviews.llvm.org/D72086
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D72086: [lldb] Fix that SBThread.GetStopDescription is returning strings with uninitialized memory at the end.
teemperor updated this revision to Diff 236793.
teemperor added a comment.
- Remove unintended empty line change.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72086/new/
https://reviews.llvm.org/D72086
Files:
lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
lldb/scripts/Python/python-typemaps.swig
Index: lldb/scripts/Python/python-typemaps.swig
===
--- lldb/scripts/Python/python-typemaps.swig
+++ lldb/scripts/Python/python-typemaps.swig
@@ -121,7 +121,7 @@
$result = string.release();
Py_INCREF($result);
} else {
- llvm::StringRef ref(static_cast($1), result);
+ llvm::StringRef ref = static_cast($1);
PythonString string(ref);
$result = string.release();
}
Index: lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
===
--- lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
+++ lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
@@ -122,14 +122,20 @@
self.assertTrue(
thread.IsValid(),
"There should be a thread stopped due to breakpoint")
-#self.runCmd("process status")
-
-# Due to the typemap magic (see lldb.swig), we pass in an (int)length
to GetStopDescription
-# and expect to get a Python string as the return object!
-# The 100 is just an arbitrary number specifying the buffer size.
-stop_description = thread.GetStopDescription(100)
-self.expect(stop_description, exe=False,
-startstr='breakpoint')
+
+# Get the stop reason. GetStopDescription expects that we pass in the
size of the description
+# we expect plus an additional byte for the null terminator.
+
+# Test with a buffer that is exactly as large as the expected stop
reason.
+self.assertEqual("breakpoint 1.1",
thread.GetStopDescription(len('breakpoint 1.1') + 1))
+
+# Test some smaller buffer sizes.
+self.assertEqual("breakpoint",
thread.GetStopDescription(len('breakpoint') + 1))
+self.assertEqual("break", thread.GetStopDescription(len('break') + 1))
+self.assertEqual("b", thread.GetStopDescription(len('b') + 1))
+
+# Test that we can pass in a much larger size and still get the right
output.
+self.assertEqual("breakpoint 1.1",
thread.GetStopDescription(len('breakpoint 1.1') + 100))
def step_out_of_malloc_into_function_b(self, exe_name):
"""Test Python SBThread.StepOut() API to step out of a malloc call
where the call site is at function b()."""
Index: lldb/scripts/Python/python-typemaps.swig
===
--- lldb/scripts/Python/python-typemaps.swig
+++ lldb/scripts/Python/python-typemaps.swig
@@ -121,7 +121,7 @@
$result = string.release();
Py_INCREF($result);
} else {
- llvm::StringRef ref(static_cast($1), result);
+ llvm::StringRef ref = static_cast($1);
PythonString string(ref);
$result = string.release();
}
Index: lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
===
--- lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
+++ lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
@@ -122,14 +122,20 @@
self.assertTrue(
thread.IsValid(),
"There should be a thread stopped due to breakpoint")
-#self.runCmd("process status")
-
-# Due to the typemap magic (see lldb.swig), we pass in an (int)length to GetStopDescription
-# and expect to get a Python string as the return object!
-# The 100 is just an arbitrary number specifying the buffer size.
-stop_description = thread.GetStopDescription(100)
-self.expect(stop_description, exe=False,
-startstr='breakpoint')
+
+# Get the stop reason. GetStopDescription expects that we pass in the size of the description
+# we expect plus an additional byte for the null terminator.
+
+# Test with a buffer that is exactly as large as the expected stop reason.
+self.assertEqual("breakpoint 1.1", thread.GetStopDescription(len('breakpoint 1.1') + 1))
+
+# Test some smaller buffer sizes.
+self.assertEqual("breakpoint", thread.GetStopDescription(len('breakpoint') + 1))
+self.assertEqual("break", thread.GetStopDescription(len('break') + 1))
+self.assertEqual("b", thread.GetStopDescription(len('b') + 1))
+
+# Test that we can pass in a much larger size and still get the right output.
+self.assertEqual("breakpoint 1.1", thread.GetStopDescription(len('breakpoint 1.1') + 100))
def step_out_of_malloc_into_function_b(self, exe_name):
"""Test Python
[Lldb-commits] [PATCH] D72086: [lldb] Fix that SBThread.GetStopDescription is returning strings with uninitialized memory at the end.
teemperor marked an inline comment as done.
teemperor added inline comments.
Comment at: lldb/scripts/Python/python-typemaps.swig:124-125
} else {
- llvm::StringRef ref(static_cast($1), result);
+ const char *cstr = static_cast($1);
+ llvm::StringRef ref(cstr, strlen(cstr));
PythonString string(ref);
labath wrote:
> This is just `StringRef ref = $1`, right ?
Swig gives $1 a void* type (at least on my system), so I we still need the
`static_cast` (but we can get rid of the rest).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72086/new/
https://reviews.llvm.org/D72086
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D72086: [lldb] Fix that SBThread.GetStopDescription is returning strings with uninitialized memory at the end.
teemperor updated this revision to Diff 236790.
teemperor marked an inline comment as done.
teemperor added a comment.
- Simplify swig type map code.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72086/new/
https://reviews.llvm.org/D72086
Files:
lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
lldb/scripts/Python/python-typemaps.swig
Index: lldb/scripts/Python/python-typemaps.swig
===
--- lldb/scripts/Python/python-typemaps.swig
+++ lldb/scripts/Python/python-typemaps.swig
@@ -1,3 +1,4 @@
+
/* Typemap definitions, to allow SWIG to properly handle 'char**' data types.
*/
%typemap(in) char ** {
@@ -121,7 +122,7 @@
$result = string.release();
Py_INCREF($result);
} else {
- llvm::StringRef ref(static_cast($1), result);
+ llvm::StringRef ref = static_cast($1);
PythonString string(ref);
$result = string.release();
}
Index: lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
===
--- lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
+++ lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
@@ -122,14 +122,20 @@
self.assertTrue(
thread.IsValid(),
"There should be a thread stopped due to breakpoint")
-#self.runCmd("process status")
-
-# Due to the typemap magic (see lldb.swig), we pass in an (int)length
to GetStopDescription
-# and expect to get a Python string as the return object!
-# The 100 is just an arbitrary number specifying the buffer size.
-stop_description = thread.GetStopDescription(100)
-self.expect(stop_description, exe=False,
-startstr='breakpoint')
+
+# Get the stop reason. GetStopDescription expects that we pass in the
size of the description
+# we expect plus an additional byte for the null terminator.
+
+# Test with a buffer that is exactly as large as the expected stop
reason.
+self.assertEqual("breakpoint 1.1",
thread.GetStopDescription(len('breakpoint 1.1') + 1))
+
+# Test some smaller buffer sizes.
+self.assertEqual("breakpoint",
thread.GetStopDescription(len('breakpoint') + 1))
+self.assertEqual("break", thread.GetStopDescription(len('break') + 1))
+self.assertEqual("b", thread.GetStopDescription(len('b') + 1))
+
+# Test that we can pass in a much larger size and still get the right
output.
+self.assertEqual("breakpoint 1.1",
thread.GetStopDescription(len('breakpoint 1.1') + 100))
def step_out_of_malloc_into_function_b(self, exe_name):
"""Test Python SBThread.StepOut() API to step out of a malloc call
where the call site is at function b()."""
Index: lldb/scripts/Python/python-typemaps.swig
===
--- lldb/scripts/Python/python-typemaps.swig
+++ lldb/scripts/Python/python-typemaps.swig
@@ -1,3 +1,4 @@
+
/* Typemap definitions, to allow SWIG to properly handle 'char**' data types. */
%typemap(in) char ** {
@@ -121,7 +122,7 @@
$result = string.release();
Py_INCREF($result);
} else {
- llvm::StringRef ref(static_cast($1), result);
+ llvm::StringRef ref = static_cast($1);
PythonString string(ref);
$result = string.release();
}
Index: lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
===
--- lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
+++ lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
@@ -122,14 +122,20 @@
self.assertTrue(
thread.IsValid(),
"There should be a thread stopped due to breakpoint")
-#self.runCmd("process status")
-
-# Due to the typemap magic (see lldb.swig), we pass in an (int)length to GetStopDescription
-# and expect to get a Python string as the return object!
-# The 100 is just an arbitrary number specifying the buffer size.
-stop_description = thread.GetStopDescription(100)
-self.expect(stop_description, exe=False,
-startstr='breakpoint')
+
+# Get the stop reason. GetStopDescription expects that we pass in the size of the description
+# we expect plus an additional byte for the null terminator.
+
+# Test with a buffer that is exactly as large as the expected stop reason.
+self.assertEqual("breakpoint 1.1", thread.GetStopDescription(len('breakpoint 1.1') + 1))
+
+# Test some smaller buffer sizes.
+self.assertEqual("breakpoint", thread.GetStopDescription(len('breakpoint') + 1))
+self.assertEqual("break", thread.GetStopDescription(len('break') + 1))
+self.assertEqual("b",
[Lldb-commits] [PATCH] D72086: [lldb] Fix that SBThread.GetStopDescription is returning strings with uninitialized memory at the end.
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
This is fine. It also sounds like a good idea to solve the +1 vs snprintf
inconsistency.
Comment at: lldb/scripts/Python/python-typemaps.swig:124-125
} else {
- llvm::StringRef ref(static_cast($1), result);
+ const char *cstr = static_cast($1);
+ llvm::StringRef ref(cstr, strlen(cstr));
PythonString string(ref);
This is just `StringRef ref = $1`, right ?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72086/new/
https://reviews.llvm.org/D72086
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D72086: [lldb] Fix that SBThread.GetStopDescription is returning strings with uninitialized memory at the end.
teemperor updated this revision to Diff 236542.
teemperor edited the summary of this revision.
teemperor added a comment.
- Move change to SWIG type map.
- Fixed some type in the test.
The API never emulated the snprintf behavior as it includes a NULL byte when we
pass a nullptr buffer (and that's why it has the inconsistent ` + 1` in the
else branch). We get this whole thing even more wrong as we add the NULL byte
to the snprintf result in the other branch (at the end of the method). Anyway,
I changed this in the type map now and just use `strlen` on the result string
which is simpler (and doesn't rely on any of our bogus return values).
I'll make another patch where we can discuss what this function is supposed to
return.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72086/new/
https://reviews.llvm.org/D72086
Files:
lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
lldb/scripts/Python/python-typemaps.swig
Index: lldb/scripts/Python/python-typemaps.swig
===
--- lldb/scripts/Python/python-typemaps.swig
+++ lldb/scripts/Python/python-typemaps.swig
@@ -121,7 +121,8 @@
$result = string.release();
Py_INCREF($result);
} else {
- llvm::StringRef ref(static_cast($1), result);
+ const char *cstr = static_cast($1);
+ llvm::StringRef ref(cstr, strlen(cstr));
PythonString string(ref);
$result = string.release();
}
Index: lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
===
--- lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
+++ lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
@@ -122,14 +122,20 @@
self.assertTrue(
thread.IsValid(),
"There should be a thread stopped due to breakpoint")
-#self.runCmd("process status")
-
-# Due to the typemap magic (see lldb.swig), we pass in an (int)length
to GetStopDescription
-# and expect to get a Python string as the return object!
-# The 100 is just an arbitrary number specifying the buffer size.
-stop_description = thread.GetStopDescription(100)
-self.expect(stop_description, exe=False,
-startstr='breakpoint')
+
+# Get the stop reason. GetStopDescription expects that we pass in the
size of the description
+# we expect plus an additional byte for the null terminator.
+
+# Test with a buffer that is exactly as large as the expected stop
reason.
+self.assertEqual("breakpoint 1.1",
thread.GetStopDescription(len('breakpoint 1.1') + 1))
+
+# Test some smaller buffer sizes.
+self.assertEqual("breakpoint",
thread.GetStopDescription(len('breakpoint') + 1))
+self.assertEqual("break", thread.GetStopDescription(len('break') + 1))
+self.assertEqual("b", thread.GetStopDescription(len('b') + 1))
+
+# Test that we can pass in a much larger size and still get the right
output.
+self.assertEqual("breakpoint 1.1",
thread.GetStopDescription(len('breakpoint 1.1') + 100))
def step_out_of_malloc_into_function_b(self, exe_name):
"""Test Python SBThread.StepOut() API to step out of a malloc call
where the call site is at function b()."""
Index: lldb/scripts/Python/python-typemaps.swig
===
--- lldb/scripts/Python/python-typemaps.swig
+++ lldb/scripts/Python/python-typemaps.swig
@@ -121,7 +121,8 @@
$result = string.release();
Py_INCREF($result);
} else {
- llvm::StringRef ref(static_cast($1), result);
+ const char *cstr = static_cast($1);
+ llvm::StringRef ref(cstr, strlen(cstr));
PythonString string(ref);
$result = string.release();
}
Index: lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
===
--- lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
+++ lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
@@ -122,14 +122,20 @@
self.assertTrue(
thread.IsValid(),
"There should be a thread stopped due to breakpoint")
-#self.runCmd("process status")
-
-# Due to the typemap magic (see lldb.swig), we pass in an (int)length to GetStopDescription
-# and expect to get a Python string as the return object!
-# The 100 is just an arbitrary number specifying the buffer size.
-stop_description = thread.GetStopDescription(100)
-self.expect(stop_description, exe=False,
-startstr='breakpoint')
+
+# Get the stop reason. GetStopDescription expects that we pass in the size of the description
+# we expect plus an additional byte for the null terminator.
+
+# Test with a
[Lldb-commits] [PATCH] D72086: [lldb] Fix that SBThread.GetStopDescription is returning strings with uninitialized memory at the end.
labath added a reviewer: jingham.
labath added a comment.
Would it be possible to fix this problem in the swig bindings instead? (i.e.
move the `std::min` stuff into the swig code). That way, the behavior of this
function will at least match that of snprintf, which will hopefully be less
surprising that what you've done here. (And it will also fix the inconsistency
noticed by @shafik).
Comment at: lldb/source/API/SBThread.cpp:337
+ }
else {
// NULL dst passed in, return the length needed to contain the
shafik wrote:
> The `else` branch feels inconsistent with the change above. Especially the `+
> 1`.
yep
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72086/new/
https://reviews.llvm.org/D72086
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D72086: [lldb] Fix that SBThread.GetStopDescription is returning strings with uninitialized memory at the end.
shafik added inline comments.
Comment at: lldb/source/API/SBThread.cpp:337
+ }
else {
// NULL dst passed in, return the length needed to contain the
The `else` branch feels inconsistent with the change above. Especially the `+
1`.
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72086/new/
https://reviews.llvm.org/D72086
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D72086: [lldb] Fix that SBThread.GetStopDescription is returning strings with uninitialized memory at the end.
teemperor created this revision.
teemperor added a reviewer: labath.
Herald added subscribers: lldb-commits, JDevlieghere, abidh.
Herald added a project: LLDB.
`SBThread.GetStopDescription` is a curious API as it takes a buffer length as a
parameter that specifies
how many bytes the buffer we pass has. Then we fill the buffer until the
specified length (or the length
of the stop description string) and return the string length. If the buffer is
a nullptr however, we instead
return how many bytes we would have written to the buffer so that the user can
allocate a buffer with
the right size and pass that size to a subsequent `SBThread.GetStopDescription`
call.
Funnily enough, it is not possible to pass a nullptr via the Python SWIG
bindings, so that might be the
first API in LLDB that is not only hard to use correctly but impossible to use
correctly. The only way to
call this function via Python is to throw in a large size limit that is
hopefully large enough to contain the
stop description (otherwise we only get the truncated stop description).
Currently passing a size limit that is smaller than the returned stop
description doesn't cause the
Python bindings to return the stop description but instead the truncated stop
description + uninitialized characters
at the end of the string. The reason for this is that we return the result of
`snprintf` from the method
which returns the amount of bytes that *would* have been written (which is
larger than the buffer).
This causes our Python bindings to return a string that is as large as full
stop description but the
buffer that has been filled is only as large as the passed in buffer size.
This patch fixes this issue by just returning the actual length of the buffer
we have written to (which is either
the buffer length or the length of the stop description, whatever is shorter).
Repository:
rLLDB LLDB
https://reviews.llvm.org/D72086
Files:
lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
lldb/source/API/SBThread.cpp
Index: lldb/source/API/SBThread.cpp
===
--- lldb/source/API/SBThread.cpp
+++ lldb/source/API/SBThread.cpp
@@ -327,8 +327,13 @@
if (stop_info_sp) {
const char *stop_desc = stop_info_sp->GetDescription();
if (stop_desc) {
- if (dst)
-return ::snprintf(dst, dst_len, "%s", stop_desc);
+ if (dst) {
+::snprintf(dst, dst_len, "%s", stop_desc);
+// The string we return is either as long as buffer length minus
null
+// terminator or the number of characters in the description
(depending
+// which of these two is shorter).
+return std::min(dst_len - 1, strlen(stop_desc));
+ }
else {
// NULL dst passed in, return the length needed to contain the
// description
Index: lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
===
--- lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
+++ lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py
@@ -122,14 +122,20 @@
self.assertTrue(
thread.IsValid(),
"There should be a thread stopped due to breakpoint")
-#self.runCmd("process status")
-
-# Due to the typemap magic (see lldb.swig), we pass in an (int)length
to GetStopDescription
-# and expect to get a Python string as the return object!
-# The 100 is just an arbitrary number specifying the buffer size.
-stop_description = thread.GetStopDescription(100)
-self.expect(stop_description, exe=False,
-startstr='breakpoint')
+
+# Get the stop reason. GetStopDescription expects that we pass in the
size of the description
+# we expect plus an additional byte for the null terminator.
+
+# Test with a buffer that is exactly as large as the expected stop
reason.
+self.assertEqual("breakpoint 1.1",
thread.GetStopDescription(len('breakpoint 1.1') + 1)
+
+# Test some smaller buffer sizes.
+self.assertEqual("breakpoint",
thread.GetStopDescription(len('breakpoint') + 1))
+self.assertEqual("break", thread.GetStopDescription(len('break') + 1)
+self.assertEqual("b", thread.GetStopDescription(len('b') + 1)
+
+# Test that we can pass in a much larger size and still get the right
output.
+self.assertEqual("breakpoint 1.1",
thread.GetStopDescription(len('breakpoint 1.1') + 100)
def step_out_of_malloc_into_function_b(self, exe_name):
"""Test Python SBThread.StepOut() API to step out of a malloc call
where the call site is at function b()."""
Index: lldb/source/API/SBThread.cpp
===
--- lldb/source/API/SBThread.cpp
+++
