[Lldb-commits] [PATCH] D51569: Hold GIL while allocating memory for PythonString.
tatyana-krasnukha created this revision.
tatyana-krasnukha added reviewers: clayborg, granata.enrico.
Herald added a subscriber: lldb-commits.
Swig wraps C++ code into `SWIG_PYTHON_THREAD_BEGIN_ALLOW; ...
SWIG_PYTHON_THREAD_END_ALLOW;`
Thus, lldb crashs with "Fatal Python error: Python memory allocator called
without holding the GIL" when calls `lldb_SB***___str__` function.
Repository:
rLLDB LLDB
https://reviews.llvm.org/D51569
Files:
scripts/Python/python-extensions.swig
Index: scripts/Python/python-extensions.swig
===
--- scripts/Python/python-extensions.swig
+++ scripts/Python/python-extensions.swig
@@ -7,10 +7,14 @@
size_t desc_len = description.GetSize();
if (desc_len > 0 && (desc[desc_len-1] == '\n' || desc[desc_len-1] == '\r'))
--desc_len;
-if (desc_len > 0)
-return lldb_private::PythonString(llvm::StringRef(desc, desc_len)).release();
-else
+
+if (desc_len == 0)
return lldb_private::PythonString("").release();
+
+SWIG_PYTHON_THREAD_BEGIN_BLOCK;
+PyObject *result = lldb_private::PythonString(llvm::StringRef(desc, desc_len)).release();
+SWIG_PYTHON_THREAD_END_BLOCK;
+return result;
}
}
%extend lldb::SBBlock {
@@ -21,10 +25,14 @@
size_t desc_len = description.GetSize();
if (desc_len > 0 && (desc[desc_len-1] == '\n' || desc[desc_len-1] == '\r'))
--desc_len;
-if (desc_len > 0)
-return lldb_private::PythonString(llvm::StringRef(desc, desc_len)).release();
-else
+
+if (desc_len == 0)
return lldb_private::PythonString("").release();
+
+SWIG_PYTHON_THREAD_BEGIN_BLOCK;
+PyObject *result = lldb_private::PythonString(llvm::StringRef(desc, desc_len)).release();
+SWIG_PYTHON_THREAD_END_BLOCK;
+return result;
}
}
%extend lldb::SBBreakpoint {
@@ -35,10 +43,14 @@
size_t desc_len = description.GetSize();
if (desc_len > 0 && (desc[desc_len-1] == '\n' || desc[desc_len-1] == '\r'))
--desc_len;
-if (desc_len > 0)
-return lldb_private::PythonString(llvm::StringRef(desc, desc_len)).release();
-else
+
+if (desc_len == 0)
return lldb_private::PythonString("").release();
+
+SWIG_PYTHON_THREAD_BEGIN_BLOCK;
+PyObject *result = lldb_private::PythonString(llvm::StringRef(desc, desc_len)).release();
+SWIG_PYTHON_THREAD_END_BLOCK;
+return result;
}
%pythoncode %{
@@ -64,10 +76,14 @@
size_t desc_len = description.GetSize();
if (desc_len > 0 && (desc[desc_len-1] == '\n' || desc[desc_len-1] == '\r'))
--desc_len;
-if (desc_len > 0)
-return lldb_private::PythonString(llvm::StringRef(desc, desc_len)).release();
-else
+
+if (desc_len == 0)
return lldb_private::PythonString("").release();
+
+SWIG_PYTHON_THREAD_BEGIN_BLOCK;
+PyObject *result = lldb_private::PythonString(llvm::StringRef(desc, desc_len)).release();
+SWIG_PYTHON_THREAD_END_BLOCK;
+return result;
}
}
@@ -79,10 +95,14 @@
size_t desc_len = description.GetSize();
if (desc_len > 0 && (desc[desc_len-1] == '\n' || desc[desc_len-1] == '\r'))
--desc_len;
-if (desc_len > 0)
-return lldb_private::PythonString(llvm::StringRef(desc, desc_len)).release();
-else
+
+if (desc_len == 0)
return lldb_private::PythonString("").release();
+
+SWIG_PYTHON_THREAD_BEGIN_BLOCK;
+PyObject *result = lldb_private::PythonString(llvm::StringRef(desc, desc_len)).release();
+SWIG_PYTHON_THREAD_END_BLOCK;
+return result;
}
}
@@ -110,10 +130,14 @@
size_t desc_len = description.GetSize();
if (desc_len > 0 && (desc[desc_len-1] == '\n' || desc[desc_len-1] == '\r'))
--desc_len;
-if (desc_len > 0)
-return lldb_private::PythonString(llvm::StringRef(desc, desc_len)).release();
-else
+
+if (desc_len == 0)
return lldb_private::PythonString("").release();
+
+SWIG_PYTHON_THREAD_BEGIN_BLOCK;
+PyObject *result = lldb_private::PythonString(llvm::StringRef(desc, desc_len)).release();
+
[Lldb-commits] [PATCH] D51557: Replace uses of LazyBool with LazyBool template
labath added a comment.
I've been wanting to implement something like this for a long time, so thank
you for working on this. :)
In https://reviews.llvm.org/D51557#1221273, @zturner wrote:
> Also I think it doesn't need to be specific to member variables. We could
> just have
>
> template class Lazy {
> std::function Update;
> Optional Value;
> public:
> LazyValue(std::function Update) : Update(std::move(Update)) {}
> };
>
I think the issue with this approach is size. std::function is huge and
heap-allocated, whereas this implementation is the same size as `Optional`
(which might still be too much for some of our use cases -- in some classes we
store a bunch of `needs_update` flags as bitfields to conserve space, which
would not be possible here).
Comment at: include/lldb/DataFormatters/ValueObjectPrinter.h:138-144
+ bool UpdateShouldPrint();
+ bool UpdateIsNil();
+ bool UpdateIsUnit();
+ bool UpdateIsPtr();
+ bool UpdateIsRef();
+ bool UpdateIsAggregate();
+ bool UpdateIsInstancePtr();
I wouldn't call these `update` as they don't actually update anything and just
return the value. In other parts of the codebase we use `compute` for functions
like this.
Comment at: include/lldb/DataFormatters/ValueObjectPrinter.h:146
+
+#define LLDB_VOP ValueObjectPrinter
+ LazyBoolMember m_should_print;
shafik wrote:
> davide wrote:
> > can you typedef?
> I feel like using ... = is cleaner
I am actually tempted to have a macro which would define all of this
boilerplate for you. :D
Something like
```
#define LAZY_MEMBER(Class, Type, Name) \
public: \
Type get ## Name() { return m_ ## Name.get(*this); } \
private: \
LazyMember m_ ## Name; \
Type compute ## Name();
```
Then all that would be left for the user is to define the compute function in
the cpp file.
Comment at: include/lldb/Utility/Lazy.h:25-28
+ T m_value;
+ bool m_needs_update;
+
+ static_assert(std::is_trivial::value, "Only trivial types are
supported.");
if you use `Optional` instead of hand-rolling the flag here, you could
probably get rid of the "trivial type" limitation.
Repository:
rLLDB LLDB
https://reviews.llvm.org/D51557
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r341274 - Ignore unicode decode errors in test suite's encoded_file class
Author: labath Date: Sat Sep 1 05:15:46 2018 New Revision: 341274 URL: http://llvm.org/viewvc/llvm-project?rev=341274&view=rev Log: Ignore unicode decode errors in test suite's encoded_file class These happen in a couple of tests when lldb tries to pretty print a const char * variable in the inferior which points to garbage. Instead, we have the python replace the invalid sequences with the unicode replacement character. Modified: lldb/trunk/packages/Python/lldbsuite/support/encoded_file.py Modified: lldb/trunk/packages/Python/lldbsuite/support/encoded_file.py URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/support/encoded_file.py?rev=341274&r1=341273&r2=341274&view=diff == --- lldb/trunk/packages/Python/lldbsuite/support/encoded_file.py (original) +++ lldb/trunk/packages/Python/lldbsuite/support/encoded_file.py Sat Sep 1 05:15:46 2018 @@ -31,7 +31,7 @@ def _encoded_write(old_write, encoding): # If we were asked to write a `str` (in Py2) or a `bytes` (in Py3) decode it # as unicode before attempting to write. if isinstance(s, six.binary_type): -s = s.decode(encoding) +s = s.decode(encoding, "replace") return old_write(s) return impl ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D51208: [DWARF] Fix dwarf5-index-is-used.cpp
labath added a comment. In https://reviews.llvm.org/D51208#1214743, @dblaikie wrote: > >> But if LLDB has different performance characteristics, or the default > >> should be different for other reasons - I'm fine with that. I think I left > >> it on for Apple so as not to mess with their stuff because of the > >> MachO/dsym sort of thing that's a bit different from the environments I'm > >> looking at. > > > > These are the numbers from my llvm-dev email in June: > > > >> setting a breakpoint on a non-existing function without the use of > >> accelerator tables: > >> real0m5.554s > >> user0m43.764s > >> sys 0m6.748s > >> > >> setting a breakpoint on a non-existing function with accelerator tables: > >> real0m3.517s > >> user0m3.136s > >> sys 0m0.376s > > > > This is an extreme case, > > What was being tested here? Is it a realistic case (ie: not a pathalogical > case with an absurd number of symbols, etc)? Using ELF? Fission or not? These numbers are from linux (so ELF), without fission, and with -fstandalone-debug. The binary being debugged is a debug build of clang. The only somewhat pathological aspect of this is that I deliberately chose a function name not present in the binary when setting the breakpoint. > How's it compare to GDB performance, I wonder? Perhaps LLDB hasn't been > optimized for the non-indexed case and could be - though that'd still > potentially motivate turning on indexes by default for LLDB until someone has > a motivation to do any non-indexed performance tuning/improvements. Yes, I could very well be that LLDB is optimized for the indexed case (or perhaps the other way around, maybe the accelerator tables are optimized for the kind of searches that lldb likes to do). Though I don't mean to say that the non-indexed case is not optimized too -- a number of people over the years have spent a lot of time trying to make the index building step as fast as possible. However, these were all very low-level optimizations (the "improve parallelism by reducing lock contention" kind) and it's possible we might come up with something with better performance characteristics if we looked at a bigger picture. However, that would probably require redesigning a lot of things. So, with that in mind, I agree we should enable debug_names for -glldb. I'll create a patch for that. Repository: rLLDB LLDB https://reviews.llvm.org/D51208 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D51208: [DWARF] Fix dwarf5-index-is-used.cpp
labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. As for the original patch, which we've kind of hijacked, I think it is a good idea to commit this for now. Once the clang patch is in, I'll exchange these flags for -glldb. Repository: rLLDB LLDB https://reviews.llvm.org/D51208 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
