[Lldb-commits] [PATCH] D159011: [lldb][NFCI] Remove use of ConstString from UnixSignals

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D159011#4624495 <https://reviews.llvm.org/D159011#4624495>, @fdeazeve wrote:

> I am slightly wary of making this a global set without any kind of thread safe mechanism, as I (naively, not really knowing how this class is used) would expect us to be able to instantiate multiple servers and have them be accessed concurrently. As such, it makes more sense to have each server own its set of strings.
>
> Have we considered having UnixSignals be the one to extend the lifetime of strings? It seems that UnixSignals is the one storing the StringRefs and therefore requiring extended lifetime. I couldn't figure out if there is any other advantage to this vs the other suggestion.

Right, that's a fair concern I think. The reason I chose to make it not the job of `UnixSignals` is that the majority of these strings are string literals. I think having the one instance where the strings are not literals somewhere in a data section should be responsible for guaranteeing the lifetimes of these strings. I did not want to put them in the ConstString string pool because I don't think that string pool needs any more strings in it than already exist there.

That being said, you suggested the StringSet be a private member variable of `PlatformRemoteGDBServer`. This may work, but I wasn't sure what the lifecycle of `PlatformRemoteGDBServer` objects were. Perhaps it exists for a time but is destroyed sometime later. At that point, UnixSignals will hold onto some dangling references. I decided to make it a global object to guarantee its lifetime independent of any `PlatformRemoteGDBServer` instance. I don't mind wrapping it in a concurrent structure to guarantee thread safety though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159011