[Lldb-commits] [PATCH] D35083: [TypeSystem] Guard the global `ASTSourceMap` with a mutex

[Sean Callanan via Phabricator via lldb-commits]

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 <typename FnType>
+static decltype(std::declval<FnType>()(std::declval<ASTSourceMap&>()))
+WithExclusiveSourceMap(FnType fn) {
----------------
lhames wrote:
> Does std::result_of<FnType>::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 <typename FnType>
static typename std::result_of<FnType>::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