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

[Jim Ingham via Phabricator via llvm-commits]

jingham added a comment.

Commented inline.



================
Comment at: source/Plugins/Platform/POSIX/PlatformPOSIX.cpp:1075-1078
+  lldb::addr_t return_addr = process->CallocateMemory(2*addr_size,
+                                                      permissions,
+                                                      utility_error);
+  if (utility_error.Fail()) {
----------------
davide wrote:
> Where the `2*addr_size` is coming from? Probably we should add a comment.
I'll add a comment.  I describe the return structure a bit above where I fill in the argument vector - it's two pointers.  But I'll make it clear that's what this is.


================
Comment at: source/Plugins/Platform/POSIX/PlatformPOSIX.cpp:1117
+  options.SetLanguage(eLanguageTypeC_plus_plus);
+  options.SetIgnoreBreakpoints(true);
+  options.SetUnwindOnError(true);
----------------
davide wrote:
> why are we ignoring breakpoints?
This wasn't really a breakpoint that was hit by code initiated by the app you are debugging, so it makes more sense to treat it as an internal matter than to report breakpoint hits to the user.

Suppose the user had put a breakpoint on strcmp.  Then they did some operation that turned on an instrumentation function, which in turn required the instrumentation code to load a library.  You wouldn't want that operation to be interrupted mid-course, and return to the user with a "Stopped at breakpoint 1.1" UI.  That would be disconcerting - especially since from the user's perspective the target isn't even running - and the user would have to figure out what she should do to fix the situation.

There really isn' t much benefit to stopping in whatever breakpoints dlopen hit while doing this load operation.


================
Comment at: source/Plugins/Platform/POSIX/PlatformPOSIX.cpp:1121
+                                    // don't do the work to trap them.
+  options.SetTimeout(std::chrono::seconds(2));
+  
----------------
davide wrote:
> Shouldd we try setting up a slightly more generous timeout?
I didn't change the timeout: 2 seconds is what most of the utility functions use.  We haven't had any reports of this being too short.  


Repository:
  rL LLVM

https://reviews.llvm.org/D45703