- User Since
- Sep 23 2014, 5:24 PM (374 w, 4 d)
Thu, Nov 18
Wed, Nov 17
I have also never looked at the IR Interpreter code, and am not particularly familiar with llvm IR, sorry. Maybe Shafik Yaghmour?
Tue, Nov 16
Something seems to have happened to reviews.llvm.org. I can sort of see patches, but the interface isn't responding, and I can't actually make comments there, so we'll try this way instead.
Mon, Nov 15
It makes sense for the string formatters to try reading from the target if there's no process. So the general idea is fine.
Fri, Nov 12
The stop id includes all the user expression stops. So the same "drive through code" could produce very different stop counts if you ran a bunch of p-s in one session but not the other. You can't do better than that at present since we only track the natural/vrs. expression stop id for the last stop. But if it mattered you could keep a separate "natural stop" counter and report that instead. You can't quite get back to the natural stops by subtracting the number of expressions, because not all expressions run the target, and some do twice (if the one-thread run times out).
Thu, Nov 11
Wed, Nov 10
Address Greg's review comments.
Address Pavel's review comments
Tue, Nov 9
Thu, Nov 4
Regex Type summary matching happens after all the ConstString format matching. Enrico did it that way because the ConstString matching is so much quicker, so if we can find a match there we'll get to it more quickly...
Wed, Nov 3
So probably the only safe thing to do is to ensure if the command doesn't specify a target in the execution context, we pick the currently selected one. IIUC, that's the proposal here, so I think this is all good. But we want to be careful not to allow implicit behaviors like this to change the currently selected target/thread/frame.
This needs some more design, I think. The obvious solution to Ted's problem is that target create should make the new target the currently selected one. Then the second HandleCommand would have a selected target, and the second command would work. But that doesn't seem right to me when you consider that there might be more than one process at a time in a target, which we definitely want to.
I think we should make simple cases like this work for sure. But OTOH, if you are writing a script that you might use anywhere non-trivial, SBCommandInterpreter.HandleCommand(char *, SBError) is NOT a safe thing to call. You could be in an lldb session with more than one Target, and while your command is running, the other target could have hit a breakpoint that it intended to stop at, and so it becomes the currently selected target.
Tue, Nov 2
I would do this:
Ha! It was apparently so easy to add the test that it wasn't noticeable (at least by me!).
Maybe "Controlling" plan would be better?
I have no problem with adopting more inclusive language, but "Primary" doesn't express the right concept. The "master" plan is one that coordinates other plans. There can be many "master plans" on the stack at any one time during a complex set of stepping operations, each distinguished in that it is in charge of the plans that it enqueued. So it isn't Primary in a real sense.
This is great, thanks!
I still don't think adding python specific ways of getting at generic ScriptInterpreter features (e.g. the path to the lldb module for that script interpreter - not your fault but... - and the path to the script's native script runner binary) with language specific affordances isn't one we should continue...
Mon, Nov 1
But I do agree with Jonas that we should find a less python-specific way to implement this. I don't think you need to actually fill in the Lua parts, but whoever does that shouldn't have to copy everything that was done for python, a lot of this should be shareable.
Since you do need to match the lldb module to the python it was built against, this seems a useful addition.
Oct 28 2021
Thanks for pointing these out.
Address Ismail's comments.
Do you care about the history of these settings? After all, the problem might arise because someone set a setting then unset it. Your statistics approach wouldn't catch that. If you are really trying to build an architecture where we can track this sort of problem down, then you might need more of a history approach, where the settings and certain other changes in the state of the debugger mark epochs, and you aggregate data into those epochs?
Oct 27 2021
Note, the breakpoint serialization for breakpoints only serializes the filter/resolver and the breakpoint specific options. It doesn't do anything with locations or their options. After all, you when you go to reset a breakpoint in another session (which is what this serialization is for) you have no way of knowing what locations you are going to find.
Oct 26 2021
There's one comment typo, but you can fix that on submission. Otherwise, LGTM.
Oct 21 2021
Oct 19 2021
It looks like you missed one conversion. I agree with Pavel that None is not only more pythonic but also more lldbutils-y. Other than those nits, LGTM.
Oct 18 2021
If you have the breakpoint ID, you can find the SB Breakpoint and just ask it directly what it's hit count or location hit count are, and you don't have to scrub text output.
Oct 15 2021
Actually, they aren't testing the same "sort" of thing, it's exactly the same test: "was the breakpoint hit once". But those tests are also really inaccurate as they just ask whether any breakpoint has a hit count of 1. It would be really easy to edit a test, add another breakpoint that gets hit before you get to this self.expect. In that case, this test would stop testing what it was intended to test.
All these tests are testing the same sort of thing. It would be better to make an lldbutil function that does this test and convert all these tests over to that. This sort of change is exactly why we don't want code that's poking at command results to find info about things like breakpoints scattered all over the testsuite.
Oct 12 2021
I changed the command name from "command multiword" to "command container".
Oct 11 2021
Oct 8 2021
I realized I typed this all down a while ago but forgot to hit submit. I think I still agree with former me...
Sounds good. When we started adding extension points for python, we made the implementations be free functions. But then over time we realized it was often more convenient if you had a object managing the callback, that way it could more easily store state over the invocations, etc. If doing free functions is harder in Java, I think it would be fine to only implement the "class with an invocation method" version of the callback.
Support for a scripting language in lldb has three distinct pieces.
Sep 30 2021
My brain wants llvm::Error to be false when there's an error, (probably because false is the bad state.) Fix a case where I didn't catch myself...
Addresses Jonas' first round of comments.
Addressed Jonas' review comments.
Sep 29 2021
Made -C with an absolute path an error, added at test for it.
Sep 27 2021
The one thing that makes me sad about this is that "command script import" chose "-c" as the short letter for this option, but "-c" was already taken for "command source", so I used -C for "command source".
I'm fine with this, but I'll defer to Jonas since he had the last significant comments.
I don't think this patch added all that much value, and pretty much only worked if the non-stop threads never stopped... I think we'd have to start deeper in lldb to if we really want to support having threads that might stop while we think the process is stopped, and carrying this forward isn't going to help that effort when we get around to it.
Sep 22 2021
The other slightly awkward choice I made was to have all the commands that take command paths (e.g. "command script add" and "command multiword add") take them with each component of the path as a separate argument. We have an argument type for commands, but the problem with that is then you'd have to make the path a single word, i.e.:
I also didn't mirror "command script list" - I'm not sure how useful that is and anyway, that's an orthogonal piece of work, and this patch is big enough already.
Note, I didn't do anything on the SB API side. We don't have a way to add commands of any sort through the API's, so we would have to do all that work along with adding multiword support, which is certainly fodder for a separate patch.
Sep 15 2021
Sep 14 2021
BTW, do you know what change made this stop working?