[Lldb-commits] [PATCH] D45703: Use a utility function to speed up "process load image" for POSIX Platforms

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I'm not sure what facility we are designing here?  If it is only utility functions, I think that is a very narrow use case, and I'm not sure it needs a special purpose facility.  I would prefer to wait till I see a second instance for that.  After all, this is not SB API, we can change it as we go along.  I would prefer to just leave a comment "if you do this again make it more general" to avoid copying the pattern if somebody's ever going to do that.

If it is more general "I'm going to stash something in the process" then we need to know who owns the thing?  Will it be enough to just have the process destroy it when it goes away?  Do we want to notify the registree?  For this particular use case, I know the answer, but this is specifically a cache, which can always be rebuilt if we get nothing from the cache.  Is that what we're modeling in general...

I feel like it is premature to design this store till we have some more prospective uses.



================
Comment at: source/Target/Process.cpp:6251
+UtilityFunction *Process::GetLoadImageUtilityFunction(Platform *platform) {
+  if (platform != GetTarget().GetPlatform().get())
+    return nullptr;
----------------
labath wrote:
> Will this ever be true? I would not expect one to be able to change the platform of a process mid-session.
Reading through the code I could not convince myself that it could NOT happen.  Target::SetPlatform is a function anybody can call at any time.  I agree that it would be odd to do so, and we might think about prohibiting it (Target::SetPlatform could return an error, and forbid changing the Platform, if the Target has a live process.)

If everybody agrees that that is a good idea, I can do that and remove this check.


================
Comment at: source/Target/Process.cpp:6256
+
+void Process::SetLoadImageUtilityFunction(UtilityFunction *utility_func) {
+  m_dlopen_utility_func_up.reset(utility_func);
----------------
labath wrote:
> I think the argument of this function should be a unique_ptr as well.
Okay, I'll try that out.


Repository:
  rL LLVM

https://reviews.llvm.org/D45703