[Lldb-commits] [PATCH] D35083: [TypeSystem] Guard the global `ASTSourceMap` with a mutex
jingham added a comment. Ah, maybe you meant applying the thread safety annotation to Sean's solution to enforce the contract. That's an interesting solution, but makes a non-straightforward solution even more non-straightforward, so I agree this isn't the best example... Repository: rL LLVM https://reviews.llvm.org/D35083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D35083: [TypeSystem] Guard the global `ASTSourceMap` with a mutex
spyffe added a comment.
Responded to Lang's comments inline.
**Jim**: you say this patch "doesn't actually enforce that you take the lock to
get the SourceMap." How do you get the source map otherwise? It's static
inside the function so nothing can see it outside. Do you mean that it's still
possible to have the lambda make an alias to the reference and return?
**Pavel**: if I have `GetSourceMap()` take a `unique_lock&` and leave the
return value as it is, then clients get an unguarded alias to `s_source_map` to
play with regardless of whether they're holding the lock or not. We have to
trust that they hold on to the lock as long as they need to – or use
`GUARDED_BY`, adding more complexity for something that
`WithExclusiveSourceMap()` enforced by default. (Creating an escaping alias is
still an issue.)
I'm going to hold off for Lang's opinion on this. I feel like we have a few
workable solutions, and since this isn't API for anything even outside the
`.cpp` file picking one and going with it is fine.
Comment at: source/Symbol/ClangExternalASTSourceCommon.cpp:24
+template
+static decltype(std::declval()(std::declval()))
+WithExclusiveSourceMap(FnType fn) {
lhames wrote:
> Does std::result_of::type work as the return type?
>
> http://en.cppreference.com/w/cpp/types/result_of
No, it doesn't. If I change the declaration to
```
template
static typename std::result_of::type
WithExclusiveSourceMap(FnType fn) {
```
then the compiler can no longer resolve calls:
```
…/lldb/source/Symbol/ClangExternalASTSourceCommon.cpp:25:1: Candidate template
ignored: substitution failure [with FnType = (lambda at
…/lldb/source/Symbol/ClangExternalASTSourceCommon.cpp:39:7)]: implicit
instantiation of undefined template 'std::__1::result_of<(lambda at
…/lldb/source/Symbol/ClangExternalASTSourceCommon.cpp:39:7)>'
```
Comment at: source/Symbol/ClangExternalASTSourceCommon.cpp:28
static ASTSourceMap *s_source_map = new ASTSourceMap;
- return *s_source_map;
+ static std::mutex s_source_map_mutex;
+
lhames wrote:
> Should this be on a context object of some kind (ASTContext?).
No, it shouldn't. This is intended to be global.
Repository:
rL LLVM
https://reviews.llvm.org/D35083
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D35113: Clean up lldb-types.h
Eugene.Zelenko added inline comments. Comment at: include/lldb/Host/MainLoop.h:16 #include "llvm/ADT/DenseMap.h" +#include I think will be good idea to include csignal instead. Same in other files. See http://clang.llvm.org/extra/clang-tidy/checks/modernize-deprecated-headers.html. https://reviews.llvm.org/D35113 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D35083: [TypeSystem] Guard the global `ASTSourceMap` with a mutex
labath added a comment. In https://reviews.llvm.org/D35083#802400, @jingham wrote: > Ah, maybe you meant applying the thread safety annotation to Sean's solution > to enforce the contract. That's an interesting solution, but makes a > non-straightforward solution even more non-straightforward, so I agree this > isn't the best example... Actually, I meant applying the annotations to *your* solution :). The "alternatively" meant that instead of passing the by-reference locker argument to enforce the contract, you would annotate the function like: static GetASTMap() REQUIRES_CAPABILITY(my_mutex) Then the caller could just lock the mutex any way he likes and the compiler would smack him on the head if he forgets that. However, after playing around with this more, I have become less enthusiastic about it -- it basically requires you to wrap all locking primitives with your own (annotated) classes (recent libc++'s have them out of the box, but only on std::mutex). So, maybe you should just ignore my comments :) Repository: rL LLVM https://reviews.llvm.org/D35083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r307512 - Don't access Python objects while not holding the GIL.
Author: zturner
Date: Sun Jul 9 16:32:15 2017
New Revision: 307512
URL: http://llvm.org/viewvc/llvm-project?rev=307512&view=rev
Log:
Don't access Python objects while not holding the GIL.
Patch by Tatyana Krasnukha
Differential Revision: https://reviews.llvm.org/D34942
Modified:
lldb/trunk/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
Modified:
lldb/trunk/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp?rev=307512&r1=307511&r2=307512&view=diff
==
---
lldb/trunk/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
(original)
+++
lldb/trunk/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
Sun Jul 9 16:32:15 2017
@@ -1857,14 +1857,12 @@ StructuredData::DictionarySP ScriptInter
return StructuredData::DictionarySP();
PythonObject reply_pyobj;
- {
-Locker py_lock(this,
- Locker::AcquireLock | Locker::InitSession |
Locker::NoSTDIN);
-TargetSP target_sp(target->shared_from_this());
-reply_pyobj.Reset(PyRefType::Owned,
- (PyObject *)g_swig_plugin_get(generic->GetValue(),
-setting_name, target_sp));
- }
+ Locker py_lock(this,
+ Locker::AcquireLock | Locker::InitSession | Locker::NoSTDIN);
+ TargetSP target_sp(target->shared_from_this());
+ reply_pyobj.Reset(PyRefType::Owned,
+(PyObject *)g_swig_plugin_get(generic->GetValue(),
+ setting_name, target_sp));
PythonDictionary py_dict(PyRefType::Borrowed, reply_pyobj.get());
return py_dict.CreateStructuredDictionary();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
