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

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In https://reviews.llvm.org/D35083#802439, @spyffe wrote:

> 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?


Nah, just missed you were hiding the map inside the wrapper amidst all the goo.

> **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.)

We do this all over the place in lldb.  The fact that you have to provide a mutex to access a data structure is pretty clear documentation that you should only access it under the aegis of that mutex.  And the scoped lockers make this extremely easy to do right.  Do you really think this is an issue - and do you think lldb would be made more readable by replacing all functions that access mutex-protected variables with this sort of construct?


Repository:
  rL LLVM

https://reviews.llvm.org/D35083