User Details
- User Since
- Sep 23 2014, 5:24 PM (469 w, 1 d)
Today
Yesterday
Tue, Sep 19
Wed, Aug 30
Sorry, I got distracted, but after the fact I also agree this is reasonable.
Aug 17 2023
This seems good to me. Tests that aren't using stdio for a purpose shouldn't have to worry about random output.
Aug 16 2023
Aug 15 2023
I implemented Alex's suggestion for a GetListeners filter, but doing that triggered some bugs due to the fact that we weren't really pruning the m_listeners correctly as listeners come and go. I fixed some bugs there along with implementing this suggestion.
Respond to review comments.
Aug 14 2023
LGTM
Aug 10 2023
Not sure if codesign exists on iOS devices, but at least we could do this on macOS.
In this function we have the path to the binary. We could spawn codesign -d -entitlements - and then we would know whether it had that entitlement.
Aug 9 2023
It's fairly common for AppKit developers to use access to static methods under NSApplication to see the state of the application. Since you aren't actually accessing the frame variables for this purpose, it's also pretty common for those folks to do:
Aug 4 2023
Aug 3 2023
Remove redundant code
Use llvm::popcount instead of using the builtin directly.
Aug 2 2023
LGTM. Thanks for adding this.
Jul 19 2023
Excellent!
LGTM, with a couple suggestions.
Jul 17 2023
Jul 11 2023
Jul 10 2023
This looks like it might be the place where we also say "set s could either be set or show". But that's actually done elsewhere so this change is okay. We should maybe make sure we have a test for setting s<TAB> -> {set or show} options.
Jul 7 2023
Jul 6 2023
Add some std::move's, Jonas agrees all the cool kids are doing that these days.
Yes, that makes sense. lldb always updates its shared library state in reaction to a stop, so it's much safer to have the scripted process emulate this behavior. Plus, refreshing the state for a given stop makes more sense in the place where you generate the stop, than it does in the place where you tell yourself you've resumed.
Jul 5 2023
Protect InterruptRequested from null function & format strings.
Address review comments:
Jun 27 2023
The fact that you had to duplicate code from Target::Launch means the various paths to Launching a process aren't properly factored out. However, that seems like a lot just to get Dave's fix for "platform process launch" working, so this LGTM as a short-term patch.
Jun 26 2023
Jun 23 2023
There's similar code for the runtime versions 12-15 which actually looks correct. It seems to claim that the duplicate classes are all stuffed after the capacity in the classOffsets array and that the one that won will indeed be in that list but will not be marked as a duplicate. Did it not work to fix what look like transcription errors going from the 12-15 version of this code to the 16 version?
Jun 15 2023
LGTM. Let's see if we can get this to actually trap the concurrent test failures.
Jun 13 2023
LGTM
It's OK to retain this as the default, and as you say, taking it out would be a trivial patch after this work. The control does allow you to do "Have I already made this child" before setting about to make it, though I can't currently see anywhere where we could use this information.
I can dream up a few speculative uses of can_create => false but they are all a little far-fetched. Certainly passing true is the dominant mode, so making it a default seems fine to me and reduces a bunch of noise.
That does look like a thinko. Nothing about a file's section addresses changing will change the name to symbol map as that isn't dependent on load addresses.
Jun 12 2023
I don't think we need to support the case where the "end line token" doesn't have to stand on its own line. It's reasonable for lldb to impose this policy, and if we are going to do that then requiring the "\n" after the end line token also seems odd. The way this is done seems okay to me.
I wonder about this one. In every instance where the API is used, its result is turned into a ConstString first. That's because this variable name lives in the same slot as normal variable names, which come from the debug information and so tend to be in the ConstString pool for better reasons. Do you project being able to get rid of that latter requirement? If not, it seems a bit odd to go to the trouble to avoid this value starting life as a ConstString when the first thing everybody does with it is to turn it into a ConstString.
LGTM, if no one has found a use for this way of filtering by this point, there probably isn't a good one.
Jun 1 2023
May 31 2023
I'd also maybe call this the Target "Label" not the Name. We have a fairly strong use of Name for breakpoint names, and this doesn't have that character at all. Also, if they are Labels, I think it's legit for us to keep them unique, which I think is more sane than trying to handle two targets with the same label...
Make sure we do something sensible with "target select <NAME>" if the user has given the same name to two targets (or disallow doing that, one or the other).
Why did you choose to have a separate FileSpecBuilder class, rather than making FileSpec smarter about the structure of the path (e.g. have an array of StringRef's into the paths for each component.) We could do the parse once the first time a path element was requested, and then operations like "RemovePathComponent" would be trivial. We could maybe even get rid of the distinction between "path" and "filename" since "filename" is really just "last path component".
May 30 2023
May 24 2023
The help string for the setting seems clear. There's also some logic to handle the setting vrs. the values we find from the stub which you describe in the comment to the review, but it would be nice to see that in a comment in the code somewhere to help out future generations.
May 22 2023
If a Scripted process is able to provide private events and enough info to lldb's lower level calls (read memory, registers) to work the thread-plan machinery then it won't need to generate the inferred stop reasons like PlanComplete and Instrumentation. But the scripted threads might just be producing public stops w/o the underlying model that would allow lldb to generate them. In that case, the scripted thread will also need to be able to produce the inferred stop reasons.
May 16 2023
This seems fine in general, with one quibble:
May 15 2023
LGTM, Adrian's comment is still outstanding however.
Your test measured setting a found simple breakpoint. That should measure filling all the names caches - we do that the first time you try to set a breakpoint of any sort. But doesn't measure the effects on lookup. I am guessing you will find the same "not much difference" here as well, but it would be good to test that. So it would be good to also ensure you aren't slowing down looking for a selector by name, and looking for a selector you aren't going to find by name, and looking by full ObjC name. But if that's also true, then I'm fine with this.
May 11 2023
I added the enum, that's a good idea.
Address Review Comments
May 10 2023
Address review comments
LGTM for support of something that really should hurt a little more than you are making it...
May 9 2023
Most of this is fine. I wonder about avoiding caching the full name and name w/o category & selector name. One of the main uses of this class is to take incoming ObjC names from the ConstString pool, chop them up into full name, name w/o category, and selectorName, which we then insert into the lookup names tables for the modules they are found in. All those lookup table names will also exist in the ConstString pool.