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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I like the idea of leaving some comment to have a record that we discussed making a more generic storage facility, but I don't think we need to do anything else right now. I doubt we will see many new clients for this very soon.



================
Comment at: source/Plugins/Platform/POSIX/PlatformPOSIX.cpp:1166
+  // The dlopen succeeded!
+  if (token != 0x0)
+    return process->AddImageToken(token);
----------------
Use LLDB_INVALID_IMAGE_TOKEN here?


================
Comment at: source/Target/Process.cpp:6251
+UtilityFunction *Process::GetLoadImageUtilityFunction(Platform *platform) {
+  if (platform != GetTarget().GetPlatform().get())
+    return nullptr;
----------------
jingham wrote:
> 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.
I see three call in the codebase to SetPlatform. All of them seem to be in the initialization code, though some of them seem to happen after the process is launched (DynamicLoaderDarwinKernel constructor).

So it may not be possible to completely forbid setting the platform this way, but I think we can at least ban changing the platform once it has already been set in a pretty hard way (lldb_assert or even assert). I think a lot of things would currently break if someone started changing the platforms of a running process all of a sudden.


Repository:
  rL LLVM

https://reviews.llvm.org/D45703