[Lldb-commits] [PATCH] D60468: Lock accesses to OptionValueFileSpecList objects

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D60468#1460212 <https://reviews.llvm.org/D60468#1460212>, @friss wrote:

> In D60468#1460065 <https://reviews.llvm.org/D60468#1460065>, @labath wrote:
>
> > No opinion on the patch, but what is the reason for having settings that are shared between multiple Debugger instances? My expectation was that the debugger objects are completely independent, and I would be surprised if the value of some setting changed from under me because of something that happened in another debug session.
>
>
> As Jim said, there are parts of the debugger that simply do not have access to a debugger by design. I'm honestly not sure this is the correct design in hindsight. I tried to make the global state per-debugger, but I stopped when I needed to change the Plugin registration to take a debugger (because SymbolVendor plugins would need a debugger to access search paths for example). This approach also has other challenges and breaks command completion in non-trivial ways.


Yeah, I think we're on the same page here. I'm going to hijack this patch a bit more (sorry), because this is a good opportunity to bring up something that I have been thinking about lately. I think sharing state between debugger instances is a good thing (particularly for expensive things like parsed debug info), but I think it should be done in a way that is invisible to the debugger instances. What this would mean in practice is that any piece of data that affects the contents of a shared object should be used in determining whether that object can be shared. For settings like exec-search-path and similar, this could mean that the logic to look up the actual executable and it's symbols happens within the context of a specific debugger instance (and it's own settings), but then, once all paths have been resolved, you check the shared cache to see if that specific path is present in there already. So, if two Debuggers have the same (or similar) search paths, they will still end up sharing the state, but if they have different search paths, things will behave just as if they each one had their own copy of all modules.

The reason I've been thinking about this is the "target symbols add" command, which adds/modifies the symbol file of a module. Despite the name, this command does not only modify the module symbols of the currently selected target, it can also modify the module of a target in a completely different debugger instance (!).

In D60468#1460022 <https://reviews.llvm.org/D60468#1460022>, @friss wrote:

> In D60468#1460013 <https://reviews.llvm.org/D60468#1460013>, @clayborg wrote:
>
> > Almost seems like we can build the mutex into the base class OptionValue as we need general threaded protection for every setting. They any function that gets or sets the value should be able to protect itself using the base mutex
>
>
> If we generalize this to most other properties, then this would make sense. Note that all properties that store very simple types don't need this, as they don't risk being accessed in a broken state.


While the simple types may not cause a crash, this access pattern would still qualify as a data race that would probably show up if you ran things under tsan. However, given how current OptionValue classes are implemented, I'm not sure how much can be actually done here in a generic way, and given that we seem to agree that this is not the best long term solution here, I don't see a reason to do everything in this patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60468/new/

https://reviews.llvm.org/D60468