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

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Wed Apr 18 01:53:14 PDT 2018


labath added inline comments.


================
Comment at: source/Plugins/Platform/POSIX/PlatformPOSIX.cpp:1166
+  // The dlopen succeeded!
+  if (token != 0x0)
+    return process->AddImageToken(token);
----------------
jingham wrote:
> labath wrote:
> > Use LLDB_INVALID_IMAGE_TOKEN here?
> That wouldn't be right.  LLDB_INVALID_IMAGE_TOKEN is the invalid token in lldb's namespace for image loading tokens, which has no relationship to any given platform's invalid token specifier.  In fact, LLDB_INVALID_IMAGE_TOKEN is UINT32_MAX, so it is actually different from the POSIX invalid image token.
I see. Thanks for the explanation.


================
Comment at: source/Target/Process.cpp:6251
+UtilityFunction *Process::GetLoadImageUtilityFunction(Platform *platform) {
+  if (platform != GetTarget().GetPlatform().get())
+    return nullptr;
----------------
jingham wrote:
> labath wrote:
> > 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.
> Not sure.  It seems reasonable to make a target, run it against one platform, then switch the platform and run it again against the new platform.  I'm not sure I'm comfortable saying that a target can only ever use one platform.
Yeah, you're right. I suppose that makes sense. In my mind a target was associated with a given platform from the moment it got created (that is how I always do things: `platform select`, `platform connect`, and only then `target create`). I never tried what would happen if I reversed that. So I guess the platform can change during the lifetime of a Target, but I hope that is not the case for the lifetime of a Process.


Repository:
  rL LLVM

https://reviews.llvm.org/D45703





More information about the lldb-commits mailing list