Page MenuHomePhabricator

Lock accesses to OptionValueFileSpecList objects
ClosedPublic

Authored by friss on Apr 9 2019, 9:13 AM.

Details

Summary

Before a Debugger gets a Target, target settings are routed to a global set
of settings. Even without this, some part of the LLDB which exist independently
of the Debugger object (the Module cache, the Symbol vendors, ...) access
directly the global default store for those settings.

Of course, if you modify one of those global settings while they are being read,
bad things happen. We see this quite a bit with FileSpecList settings. In
particular, we see many cases where one debug session changes
target.exec-search-paths while another session starts up and it crashes when
one of those accesses invalid FileSpecs.

This patch addresses the specific FileSpecList issue by adding locking to
OptionValueFileSpecList and never returning by reference. It's not a general
solution, and I'd like to hear your ideas about how to address this in a more
principled way, but given global properties are a thing, it seems like we
need locking at this level even if we were to do a bigger refectoring.

Event Timeline

friss created this revision.Apr 9 2019, 9:13 AM

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

friss added a comment.Apr 9 2019, 9:34 AM

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.

labath added a subscriber: labath.Apr 9 2019, 9:55 AM

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.

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.

Two reasons:

First, because you have to prime new debuggers with something, and that lives outside any Debugger. THen "settings set -g" is supposed to change the global settings for new debuggers, so there is both reading and writing to the global state.

Secondly, work done in the shared module cache or down in the symbol vendors and the like is generally done without a Debugger or Target, so if you need settable properties (e.g. search paths) then the global version of the property is the only one available.

friss added a comment.Apr 9 2019, 11:23 AM

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.

Note that once a target is created, there is no cross-debugger contamination anymore except if you force things by passing '-g' to 'settings set'. But it is super weird (or rather wrong) that simple 'settings set' on one debugger have an impact on other debuggers.

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.

Note that once a target is created, there is no cross-debugger contamination anymore except if you force things by passing '-g' to 'settings set'. But it is super weird (or rather wrong) that simple 'settings set' on one debugger have an impact on other debuggers.

Yeah, there are some settings (like target.default-arch, target.trap-handler-names, target.process.stop-on-exec and a few others) that are marked global in the PropertyDefinition, which means they always refer directly to the global (outside all debuggers) OptionValue. I can't remember why that was done.

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 (!).

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.

There were no objections to the patch, and it fixes a real crash seen in the field so I'm going to check it in.

There seems to be consensus that properties should be per-debugger, so I'll work on this when I get some spare cycles. Note that making that change won't prevent won't completely prevent concurrent accesses, so we might still want to investigate making all the properties thread-safe.

clayborg accepted this revision.Apr 23 2019, 11:50 AM

Fine to fix the current issue. We should think about building this into the base class eventually. We can always have multi-threaded access, so we will always need to protect modifying operations.

This revision is now accepted and ready to land.Apr 23 2019, 11:50 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2019, 1:16 PM
Herald added a subscriber: teemperor. · View Herald Transcript