[Lldb-commits] [lldb] [lldb] Remove raw access to PluginInstances vector (PR #132884)

[David Peixotto via lldb-commits]

dmpots wrote:

> > We did consider this approach. There are some drawbacks beyond the performance issue, but the tradeoff may be worth it.
> > If we define the method like: `std::vector<PluginInstance> GetEnabledInstances()` then we are returning copies of the PluginInstance. Anything that modifies the internal state of the `PluginInstance` will be lost since it is operating on a copy (e.g. `GetEnabledInstances().front().DoSomethingToModifyInternalState()`).
> 
> Is that a theoretical concern or actually something that happens (or will happen with your patches)? Looking at the existing code, it doesn't look like anyone is modifying the instance and I can remove the non-const overload and we still compile. If anything, we probably don't want anyone to change these things as we're iterating over them.

More of a theoretical concern. I have a later patch that adds a `bool enabled` member to the PluginInstance. so updates to that would be ignored. But we could store the enabled state outside the PluginInstance by changing the vector to a pair of `{instance, enabled}`.

There were lots of plugins and I did not look in detail to see if they had internal state. But as you said if we remove the const overload and everything still works then we are a probably safe with the current set of instances.

As long as we are ok with a change from reference semantics to copy semantics for future use cases as well then I think we could go this route.

> > We could also return pointers to the plugin instances like`std::vector<PluginInstance*> GetEnabledInstances()`. But these would be pointers to the locations in the vector stored in the `PluginInstance` class. So if a new plugin is added to the PluginInstances list the pointers could be invalidated. Probably not an issue with the current code, but could be unsafe to hand out those references in general.
> 
> While definitely unsafe an something I think we shouldn't do, that's a problem today too if someone were to modify the instance list from another thread or take the address of one of its elements.

In this sense returning a copy is safer (but we would need to protect access to the underlying vector while making the copy if we really think its a problem).
 
> > I suppose another way would be to keep a `std::vector<shared_ptr<PluginInstance>>` and then creating a copy would not be an issue since any updates would also modify the original object.
> 
> I'd like to avoid shared_pointers if possible, but we could achieve the same thing by storing unique pointers in the vector and handing out raw points like you suggested above.

If you are ok with removing the reference semantics then we should probably just return a copy since that is the simplest solution.

https://github.com/llvm/llvm-project/pull/132884