Page MenuHomePhabricator

jingham (Jim Ingham)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 23 2014, 5:24 PM (374 w, 4 d)

Recent Activity

Thu, Nov 18

jingham committed rGd9f56feda84a: Remove unused variable. (authored by jingham).
Remove unused variable.
Thu, Nov 18, 4:00 PM
jingham added a comment to D112222: [LLDB] libcxx summary formatters for std::string_view.

Sorry, but is anything further required from me on this patch ?

Thu, Nov 18, 2:49 PM · Restricted Project

Wed, Nov 17

jingham added a reverting change for rGdd5505a8f2c7: Revert "Make it possible for lldb to launch a remote binary with no local file.": rG92eaad2dd7ad: Revert "Revert "Make it possible for lldb to launch a remote binary with no….
Wed, Nov 17, 6:00 PM
jingham committed rG92eaad2dd7ad: Revert "Revert "Make it possible for lldb to launch a remote binary with no… (authored by jingham).
Revert "Revert "Make it possible for lldb to launch a remote binary with no…
Wed, Nov 17, 6:00 PM
jingham resigned from D113498: [lldb] Constant-resolve operands to `getelementptr`.

I have also never looked at the IR Interpreter code, and am not particularly familiar with llvm IR, sorry. Maybe Shafik Yaghmour?

Wed, Nov 17, 3:58 PM · Restricted Project

Tue, Nov 16

jingham added a reverting change for rGb715b79d54d5: Make it possible for lldb to launch a remote binary with no local file.: rGdd5505a8f2c7: Revert "Make it possible for lldb to launch a remote binary with no local file.".
Tue, Nov 16, 4:46 PM
jingham committed rGdd5505a8f2c7: Revert "Make it possible for lldb to launch a remote binary with no local file." (authored by jingham).
Revert "Make it possible for lldb to launch a remote binary with no local file."
Tue, Nov 16, 4:46 PM
jingham added a reverting change for D113521: Allow lldb to launch a remote executable when there isn't a local copy: rGdd5505a8f2c7: Revert "Make it possible for lldb to launch a remote binary with no local file.".
Tue, Nov 16, 4:46 PM · Restricted Project
jingham committed rGb715b79d54d5: Make it possible for lldb to launch a remote binary with no local file. (authored by jingham).
Make it possible for lldb to launch a remote binary with no local file.
Tue, Nov 16, 4:09 PM
jingham closed D113521: Allow lldb to launch a remote executable when there isn't a local copy.
Tue, Nov 16, 4:09 PM · Restricted Project
jingham updated subscribers of D113098: [lldb] (Partially) enable formatting of utf strings before the program is started.

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.

Tue, Nov 16, 10:40 AM · Restricted Project

Mon, Nov 15

jingham added a comment to D113810: Add the stop count to "statistics dump" in each target's dictionary..

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).

I actually don't care about the number of natural user stops as the user will be aware of these stops. The info I am interested in is when a debug session has thousands of stops that auto resumed a debug session. This can happen if unix signals are used for program flow control or working around things. Android uses SIGSEGV for catching java NULL derefs and many times we will auto resume from these depending on signal settings. If a debug session is taking a long time it is nice to know. Some users might also have some python module that gets loaded and might end up setting a breakpoint with a callback that can auto resume.

Mon, Nov 15, 7:19 PM · Restricted Project
jingham requested changes to D113098: [lldb] (Partially) enable formatting of utf strings before the program is started.

It makes sense for the string formatters to try reading from the target if there's no process. So the general idea is fine.

Mon, Nov 15, 5:00 PM · Restricted Project

Fri, Nov 12

jingham accepted D113810: Add the stop count to "statistics dump" in each target's dictionary..

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).

Fri, Nov 12, 4:04 PM · Restricted Project

Thu, Nov 11

jingham added a comment to D113521: Allow lldb to launch a remote executable when there isn't a local copy.

I have two high-level questions about this:

  • what should be the relative priority of executable ModuleSP vs the launch info? IIUC, in the current implementation the module takes precedence, but it seems to me it would be more natural if the launch info came out on top. One of the reasons I'm thinking about this is because you currently cannot re-run executables that use exec(2) (I mean, you can, but it will rarely produce the desired results). This happens because we use the post-exec executable, but the rest of the launch info refers to the main one. If the executable module was not the primary source of truth here, then maybe the we could somehow use this mechanism to improve the exec story as well (by storing the original executable in the launch info or something).

Anyway, I think you are asking the question "What should we do if the Target's ProcessLaunchInfo::GetExecutableFile differs from the one in the Target's Executable Module". Or rather, should we keep the Target's ProcessLaunchInfo as the truth of what that target wants to launch, rather than looking at the Executable module.

That question is orthogonal to the one this patch is determining, which is just about the case where there isn't an executable file in the target so that the user needs to provide this info from the outside. So while I agree that yours is an interesting question, it isn't directly germane to this patch.

Yes, that is the question I am asking. I didn't want to go off into designing an exec feature. I only mentioned because it seemed potentially related. We can put away that for now.

While my question may not be "directly germane" to this patch, I wouldn't say it's orthogonal either. Right now (IIUC) the executable module is always the source of truth for the launch operation. Now you're adding a second source, so one has to choose how to handle the situation when both of them are present. You may not care what happens in that case, but _something_ is going to happen nonetheless. I guess we basically have three options:
a) prefer the executable module (this is what the patch implements, and it's also the smallest deviation from status quo of completely ignoring launch info)
b) prefer the launch info (that is what I proposed)
c) make it an error (I think that's something we could all agree on)

The reason I proposed (b) is because of the principle of specific overriding general. The executable module is embedded into the target, so I would consider it more general than the launch info, which can be provided directly to the Launch method. Greg's use case (launching a remote binary that you already have a copy of) seems like it could benefit from that. However, maybe I have an even better one. What would happen with reruns? On the first run, the user would create a executable-less target, and provide a binary through the launch info. The binary would launch fine and exit. Then the user would try to launch again using the same target and launch info, but the code would go down a different path (I'm not sure which one) because the target would already have an executable. (This is actually kind of similar to the exec scenario, because the executable module would change between the two runs.)

Thu, Nov 11, 1:09 PM · Restricted Project
jingham accepted D113019: [lldb][NFC] Inclusive Language: rename master plan to controlling plan.

LOKTM

Thu, Nov 11, 11:00 AM · Restricted Project

Wed, Nov 10

jingham added inline comments to D113521: Allow lldb to launch a remote executable when there isn't a local copy.
Wed, Nov 10, 5:40 PM · Restricted Project
jingham updated the diff for D113521: Allow lldb to launch a remote executable when there isn't a local copy.

Address Greg's review comments.

Wed, Nov 10, 5:39 PM · Restricted Project
jingham added a comment to D113521: Allow lldb to launch a remote executable when there isn't a local copy.

I have two high-level questions about this:

  • what should be the relative priority of executable ModuleSP vs the launch info? IIUC, in the current implementation the module takes precedence, but it seems to me it would be more natural if the launch info came out on top. One of the reasons I'm thinking about this is because you currently cannot re-run executables that use exec(2) (I mean, you can, but it will rarely produce the desired results). This happens because we use the post-exec executable, but the rest of the launch info refers to the main one. If the executable module was not the primary source of truth here, then maybe the we could somehow use this mechanism to improve the exec story as well (by storing the original executable in the launch info or something).

As you point out indirectly above, there are really three entities in play here. There's the Target's ExecutableModule; there's the GetExecutableFile in a user-provided ProcessLaunchInfo passed to SBTarget::Launch, for instance; and there's the ProcessLaunchInfo that's stored in the TargetProperties held by the target.
So we need to decide what it means when these three entities differ.

Shouldn't the target;s launch info get a full copy of the user-provided ProcessLaunchInfo upon launch? If that is the case, then we always can refer to the target's launch info as the truth as far as ProcessLaunchInfo goes? Then the only question we have is what to do if the target has a module

First let's address the externally supplied LaunchInfo. Why would you make a Target with an executable module A, and then tell it to launch with a LaunchInfo whose GetExecutableFile is a different FileSpec from the one in the Executable Module? The only case where this seems useful is if there is no executable module. That's bourne out by the implementation, where if the Target has an executable module the one in the LaunchInfo is ignored. So maybe the right solution for this divergence is to make it an error to use a SBLaunchInfo with a different ExecutableFile. Or maybe we could use the LaunchInfo's FileSpec as the remote file in the case where they've already given us a local file? That's sort of coherent with the case where there was no local file. In my patch I'm really using the GetExecutableFile to hold the remote path that we would have in the Module except that we don't have a way to make an Executable Module for the target that just has a remote file spec and nothing else.

Maybe you don't have the platform implemented to install a binary and you have a local copy of the binary at "/local/a.out" and then you want the remote to launch "/remote/a.out" using this path from the launch info?

In the case of exec, I don't think we want to use values of the Executable Module stuffed into a user-provided LaunchInfo and then passed to Target::Launch or Process::Launch to make re-run work. That would mean the user would have to keep around the ProcessLaunchInfo with the original binary and then feed it back to the target to get the launch to work, which seems awkward, and I don't see how you would do that well on the command line.

It depends on what we currently do with the target's launch info. If it is still setup like it was before, then re-running should just work. If we allow the executable module in the target to take precedence, then we can't use it.

So you might think you should solve this by leaving the original launch information in the TargetProperties LaunchInfo, and then re-run would always use that. But this has the problem that even though you KNOW on re-run you're first going to be debugging the initial binary, that won't be the main executable between runs, so setting breakpoints - if you want some to take in the initial binary - is going to be confusing. To handle this situation gracefully, I think we'd need to reset the ExecutableModule in the target when the exec-ed binary exits, not just squirrel the binary FileSpec away in the TargetProperties ProcessLaunchInfo.

Not sure I agree with that.

Anyway, I think you are asking the question "What should we do if the Target's ProcessLaunchInfo::GetExecutableFile differs from the one in the Target's Executable Module". Or rather, should we keep the Target's ProcessLaunchInfo as the truth of what that target wants to launch, rather than looking at the Executable module.

That question is orthogonal to the one this patch is determining, which is just about the case where there isn't an executable file in the target so that the user needs to provide this info from the outside. So while I agree that yours is an interesting question, it isn't directly germane to this patch.

true.

Wed, Nov 10, 5:03 PM · Restricted Project
jingham added inline comments to D113521: Allow lldb to launch a remote executable when there isn't a local copy.
Wed, Nov 10, 4:00 PM · Restricted Project
jingham updated the diff for D113521: Allow lldb to launch a remote executable when there isn't a local copy.

Address Pavel's review comments

Wed, Nov 10, 3:52 PM · Restricted Project
jingham added a comment to D113521: Allow lldb to launch a remote executable when there isn't a local copy.

I have two high-level questions about this:

  • what should be the relative priority of executable ModuleSP vs the launch info? IIUC, in the current implementation the module takes precedence, but it seems to me it would be more natural if the launch info came out on top. One of the reasons I'm thinking about this is because you currently cannot re-run executables that use exec(2) (I mean, you can, but it will rarely produce the desired results). This happens because we use the post-exec executable, but the rest of the launch info refers to the main one. If the executable module was not the primary source of truth here, then maybe the we could somehow use this mechanism to improve the exec story as well (by storing the original executable in the launch info or something).
Wed, Nov 10, 1:12 PM · Restricted Project

Tue, Nov 9

jingham requested review of D113521: Allow lldb to launch a remote executable when there isn't a local copy.
Tue, Nov 9, 3:18 PM · Restricted Project

Thu, Nov 4

jingham accepted D113174: [lldb] Summary provider for char flexible array members.

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...

Thu, Nov 4, 11:40 AM · Restricted Project

Wed, Nov 3

jingham added a comment to D111209: Don't push null ExecutionContext on CommandInterpreter exectx stack.

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.

Wed, Nov 3, 10:29 AM · Restricted Project
jingham added a comment to D111209: Don't push null ExecutionContext on CommandInterpreter exectx stack.

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.

Wed, Nov 3, 10:21 AM · Restricted Project
jingham added a comment to D111209: Don't push null ExecutionContext on CommandInterpreter exectx stack.

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.

Wed, Nov 3, 10:09 AM · Restricted Project

Tue, Nov 2

jingham added a comment to D112973: [lldb] make it easier to find LLDB's python.

I would do this:

Tue, Nov 2, 6:28 PM · Restricted Project
jingham requested changes to D113019: [lldb][NFC] Inclusive Language: rename master plan to controlling plan.
Tue, Nov 2, 11:36 AM · Restricted Project
jingham added a comment to D113019: [lldb][NFC] Inclusive Language: rename master plan to controlling plan.

Or Coordinating?

Tue, Nov 2, 11:01 AM · Restricted Project
jingham accepted D112988: [lldb] fix --source-quietly.

Ha! It was apparently so easy to add the test that it wasn't noticeable (at least by me!).

Tue, Nov 2, 10:36 AM · Restricted Project
jingham added a comment to D113019: [lldb][NFC] Inclusive Language: rename master plan to controlling plan.

Maybe "Controlling" plan would be better?

Tue, Nov 2, 10:20 AM · Restricted Project
jingham added a comment to D113019: [lldb][NFC] Inclusive Language: rename master plan to controlling plan.

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.

Tue, Nov 2, 10:19 AM · Restricted Project
jingham added a comment to D112988: [lldb] fix --source-quietly.

This is great, thanks!

Tue, Nov 2, 10:12 AM · Restricted Project
jingham added a comment to D112973: [lldb] make it easier to find LLDB's python.

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...

Tue, Nov 2, 10:07 AM · Restricted Project

Mon, Nov 1

jingham added a comment to D112973: [lldb] make it easier to find LLDB's python.

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.

Mon, Nov 1, 5:34 PM · Restricted Project
jingham added a comment to D112973: [lldb] make it easier to find LLDB's python.

I feel like this puts too much Python specific logic in the driver compared to the convenience it brings. Even though we already leak python details (like --python-path), LLDB has always been designed to support several scripting languages. I'm not convinced this use case is compelling enough to make the situation worse.

FWIW crashlog.py has a good example of how to import LLDB. It's complicated slightly by us having to honor the LLDB_DEFAULT_PYTHON_VERSION, but otherwise it's pretty simple:

try:
    import lldb
except ImportError:
    lldb_python_path = subprocess.check_output(['xcrun', 'lldb', '-P']).decode("utf-8").strip()
    if os.path.exists(lldb_python_path) and not sys.path.__contains__(lldb_python_path):
        sys.path.append(lldb_python_path)
        import lldb

A potential alternative to this could be an lldb-python (python) script that reinvokes itself with the correct PYTHONPATH set?

Mon, Nov 1, 5:31 PM · Restricted Project
jingham added a comment to D112973: [lldb] make it easier to find LLDB's python.

Since you do need to match the lldb module to the python it was built against, this seems a useful addition.

Mon, Nov 1, 5:21 PM · Restricted Project

Oct 28 2021

jingham committed rGd1e9514ac89b: To avoid the obvious problem, use a different port... (authored by jingham).
To avoid the obvious problem, use a different port...
Oct 28 2021, 5:45 PM
jingham committed rGe655769c4a7b: Fix a bug in Launch when using an async debugger & remote platform. (authored by jingham).
Fix a bug in Launch when using an async debugger & remote platform.
Oct 28 2021, 5:03 PM
jingham closed D112747: Restore process events after a launch that stopped at the entry point.
Oct 28 2021, 5:03 PM · Restricted Project
jingham added inline comments to D112747: Restore process events after a launch that stopped at the entry point.
Oct 28 2021, 4:23 PM · Restricted Project
jingham added inline comments to D112747: Restore process events after a launch that stopped at the entry point.
Oct 28 2021, 4:19 PM · Restricted Project
jingham committed rG1227fa7e9040: Remove unused ValueObjectDynamicValue::SetOwningSP & backing ivar (authored by jingham).
Remove unused ValueObjectDynamicValue::SetOwningSP & backing ivar
Oct 28 2021, 4:07 PM
jingham closed D112677: Remove unused ValueObjectDynamicValue::SetOwningSP & backing ivar.
Oct 28 2021, 4:07 PM · Restricted Project
jingham added a comment to D112747: Restore process events after a launch that stopped at the entry point.

Thanks for pointing these out.

Oct 28 2021, 3:53 PM · Restricted Project
jingham updated the diff for D112747: Restore process events after a launch that stopped at the entry point.

Address Ismail's comments.

Oct 28 2021, 3:53 PM · Restricted Project
jingham added a comment to D112691: Include target settings in "statistics dump" output..

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 28 2021, 3:11 PM · Restricted Project
jingham requested review of D112747: Restore process events after a launch that stopped at the entry point.
Oct 28 2021, 12:23 PM · Restricted Project

Oct 27 2021

jingham added a comment to D112587: Add breakpoint resolving stats to each target..

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.

Good to know!

So if you want to record the locations (for instance to make sure that your recreation actually did the same work) then you will have to record the locations some other way. However, you do record the number of locations, which is probably a good enough signal that you aren't recreating what you thought you were. So maybe you don't need to worry about this.

I am mainly concerned with the overall details of a breakpoint, and usually for the breakpoints that have no locations due to a source mapping error, or lack of debug information. Both stats are available (debug info size + breakpoint filter/resolver details) so that will help us figure out why things are not working for people. I had locations before, but I am not sure we need that information just yet so I will leave that for later if we do end up needing it for some reason.

Oct 27 2021, 6:06 PM · Restricted Project
jingham added a comment to D112587: Add breakpoint resolving stats to each target..

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 27 2021, 5:47 PM · Restricted Project
jingham added a comment to D105470: [lldb] Clear children of ValueObject on value update.

Thanks for the explanation! But at this point I feel I'm a bit confused about how it all _supposed_ to work in the current design :)

Oct 27 2021, 5:33 PM · Restricted Project
jingham added a reviewer for D112677: Remove unused ValueObjectDynamicValue::SetOwningSP & backing ivar: clayborg.
Oct 27 2021, 5:00 PM · Restricted Project
jingham requested review of D112677: Remove unused ValueObjectDynamicValue::SetOwningSP & backing ivar.
Oct 27 2021, 4:59 PM · Restricted Project

Oct 26 2021

jingham accepted D111899: LLDB tests modification for hardware breakpoints.

There's one comment typo, but you can fix that on submission. Otherwise, LGTM.

Oct 26 2021, 11:35 AM · Restricted Project

Oct 21 2021

jingham added inline comments to D112222: [LLDB] libcxx summary formatters for std::string_view.
Oct 21 2021, 10:07 AM · Restricted Project

Oct 19 2021

jingham added a comment to D111899: LLDB tests modification for hardware breakpoints.

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 19 2021, 11:42 AM · Restricted Project
jingham committed rGa66798cd67fe: Remove unneeded variable num_found. (authored by jingham).
Remove unneeded variable num_found.
Oct 19 2021, 9:57 AM

Oct 18 2021

jingham committed rGf24532ae91d5: Follow-on to fix a test from c5011aed9c297d6ddd8ee4f77453b215aa27554a. (authored by jingham).
Follow-on to fix a test from c5011aed9c297d6ddd8ee4f77453b215aa27554a.
Oct 18 2021, 4:54 PM
jingham committed rGc5011aed9c29: Add a "command container" hierarchy to allow users to add container nodes. (authored by jingham).
Add a "command container" hierarchy to allow users to add container nodes.
Oct 18 2021, 3:29 PM
jingham closed D110298: Add "command multiword add" and the ability to add script commands into a user multiword hierarchy.
Oct 18 2021, 3:29 PM · Restricted Project
jingham committed rG1ff367dbb02d: Fix Linux error in TestBreakInLoadedDylib.py. (authored by jingham).
Fix Linux error in TestBreakInLoadedDylib.py.
Oct 18 2021, 11:31 AM
jingham committed rG9a2e9c5db692: Add tests for the other variants of BreakpointCreateBySourceRegex. (authored by jingham).
Add tests for the other variants of BreakpointCreateBySourceRegex.
Oct 18 2021, 10:59 AM
jingham closed D111920: Test untested variants of BreakpointCreateBySourceRegex.
Oct 18 2021, 10:59 AM · Restricted Project
jingham added a comment to D111965: [lldb] improve the help strings for gdb-remote and kdp-remote.

I've seen commands print that they are an alias automatically:

(lldb) help rbreak
Sets a breakpoint or set of breakpoints in the executable.

Syntax: rbreak <cmd-options>

<...>

'rbreak' is an abbreviation for 'breakpoint set -r %1'

Is this not the case for regex commands? Showing the user a cryptic regex isn't going to help them that much anyway, so nothing against doing this change.

Oct 18 2021, 10:39 AM · Restricted Project
jingham updated subscribers of D111899: LLDB tests modification for hardware breakpoints.

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 18 2021, 10:11 AM · Restricted Project

Oct 15 2021

jingham requested review of D111920: Test untested variants of BreakpointCreateBySourceRegex.
Oct 15 2021, 6:45 PM · Restricted Project
jingham added a comment to D111899: LLDB tests modification for hardware breakpoints.

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.

Oct 15 2021, 11:38 AM · Restricted Project
jingham added a comment to D111899: LLDB tests modification for hardware breakpoints.

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 15 2021, 11:16 AM · Restricted Project

Oct 12 2021

jingham added inline comments to D110298: Add "command multiword add" and the ability to add script commands into a user multiword hierarchy.
Oct 12 2021, 12:26 PM · Restricted Project
jingham updated the diff for D110298: Add "command multiword add" and the ability to add script commands into a user multiword hierarchy.

I changed the command name from "command multiword" to "command container".

Oct 12 2021, 12:25 PM · Restricted Project

Oct 11 2021

jingham added a comment to D109738: [lldb] Fix bug 38317 - Address breakpoints don't work if set before the process launches.

Is it possible to get your IDE to record the module & the address?

I don't control the definition of the protocol (FWIW it's Microsoft's Debug Adapter Protocol), but I'll look into that, maybe there's a way to sneak in extra information. In which case, what is the correct way to save/restore instruction breakpoints across lldb runs? Even if I save module path or UUID, there seems to be no SB API to create a breakpoint using this information.

Oct 11 2021, 9:19 AM · Restricted Project

Oct 8 2021

jingham added a comment to D109738: [lldb] Fix bug 38317 - Address breakpoints don't work if set before the process launches.

That is clearly wrong: an address is never enough to restore a breakpoint from one session to the next even if you can turn ASLR off. After all, the breakpoint could be in a dlopened library.

This is usually true.

In cases where you're debugging an embedded application that doesn't involve shared libraries, it's not. The main area where I see this is when debugging application startup code. My coworker who's responsible for that is the one who reported the problem originally. In that case, the address of the breakpoint is always valid on subsequent runs.

On the other hand, when you're running under our RTOS, the application is a PIE, and we support shared libraries, so the address is never valid on subsequent runs.

Maybe we need a flag to "break set" that means "this address is always valid; don't garbage collect it".

Oct 8 2021, 4:56 PM · Restricted Project
jingham added a comment to D109738: [lldb] Fix bug 38317 - Address breakpoints don't work if set before the process launches.

I realized I typed this all down a while ago but forgot to hit submit. I think I still agree with former me...

Oct 8 2021, 2:44 PM · Restricted Project
jingham added a comment to D111409: proposed support for Java interface to Scripting Bridge.

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.

Oct 8 2021, 2:41 PM
jingham added a comment to D111409: proposed support for Java interface to Scripting Bridge.

Just to clarify my use case: I am one of the developers for a reverse-engineering tool called Ghidra. Part of the tool is debugging-support to allow cross-over between dynamic and static analysis. We currently support windbg/kd, the gcc MI2 interface, and direct Java debugging. I have a working prototype for lldb, but it requires users to patch and rebuild lldb, which may be a heavy lift for some. While it falls outside of my use case, I am quite willing to attempt implementing pieces 2 & 3 from Mr. Ingham's post.

Oct 8 2021, 2:32 PM
jingham added a comment to D111409: proposed support for Java interface to Scripting Bridge.

Support for a scripting language in lldb has three distinct pieces.

Oct 8 2021, 11:31 AM

Sep 30 2021

jingham updated the diff for D110298: Add "command multiword add" and the ability to add script commands into a user multiword hierarchy.

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...

Sep 30 2021, 6:37 PM · Restricted Project
jingham added a comment to D110298: Add "command multiword add" and the ability to add script commands into a user multiword hierarchy.

Addresses Jonas' first round of comments.

Sep 30 2021, 6:06 PM · Restricted Project
jingham updated the diff for D110298: Add "command multiword add" and the ability to add script commands into a user multiword hierarchy.

Addressed Jonas' review comments.

Sep 30 2021, 6:03 PM · Restricted Project
jingham added a comment to D110804: Add a new command "target metrics"..

Ok, so how does this sound:

  • This new command will replace the existing "statistics dump" command and emit JSON only by default
  • The "statistics enable/disable" will stay in place and allow expensive metric gathering to be enabled/disabled if needed. If the gathering is really cheap for certain metrics, then nothing needs to happen for "statistics enable/disable"
  • By default we will emit stats for the current target, and options can be added in the future to enable dumping for all targets and for debugger level stuff

Let me know if this sounds ok to everyone!?

Sep 30 2021, 3:11 PM · Restricted Project
jingham added a comment to D110804: Add a new command "target metrics"..

I would like to get a consensus on if we should move this functionality to "statistics" or not. The reasons that I didn't do it were:

  • "statistics" as a top level method doesn't really do what I wanted here, which was "target statistics", or stats that are related only to the current target. I am not all that interested in overall stats over all targets
Sep 30 2021, 2:55 PM · Restricted Project

Sep 29 2021

jingham committed rG2303391d1f54: Make "process attach -c" work correctly, and add a test for it. (authored by jingham).
Make "process attach -c" work correctly, and add a test for it.
Sep 29 2021, 7:40 PM
jingham closed D110787: Make "process attach -c" work again, add a test for it..
Sep 29 2021, 7:40 PM · Restricted Project
jingham committed rG3bf3b96629e8: Add the --relative-to-command-file to "command source" so you can (authored by jingham).
Add the --relative-to-command-file to "command source" so you can
Sep 29 2021, 7:37 PM
jingham closed D110601: Add -relative-to-command-file to "command source".
Sep 29 2021, 7:37 PM · Restricted Project
jingham updated the diff for D110601: Add -relative-to-command-file to "command source".

Made -C with an absolute path an error, added at test for it.

Sep 29 2021, 5:57 PM · Restricted Project
jingham requested review of D110787: Make "process attach -c" work again, add a test for it..
Sep 29 2021, 4:33 PM · Restricted Project
jingham added a comment to D110298: Add "command multiword add" and the ability to add script commands into a user multiword hierarchy.

The changes look fine from a quick read. See inline comments

Sep 29 2021, 9:54 AM · Restricted Project

Sep 27 2021

jingham added a comment to D110601: Add -relative-to-command-file to "command source".

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".

Sep 27 2021, 6:02 PM · Restricted Project
jingham requested review of D110601: Add -relative-to-command-file to "command source".
Sep 27 2021, 5:57 PM · Restricted Project
jingham added a comment to D110115: [lldb] Remove Expression's dependency on CPlusPlusLanguagePlugin.

I'm fine with this, but I'll defer to Jonas since he had the last significant comments.

Sep 27 2021, 12:12 PM · Restricted Project
jingham accepted D110553: [lldb] Remove non-stop mode code.

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 27 2021, 12:11 PM · Restricted Project

Sep 22 2021

jingham added inline comments to D110298: Add "command multiword add" and the ability to add script commands into a user multiword hierarchy.
Sep 22 2021, 6:12 PM · Restricted Project
jingham added a comment to D110298: Add "command multiword add" and the ability to add script commands into a user multiword hierarchy.

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.:

Sep 22 2021, 5:48 PM · Restricted Project
jingham added a comment to D110298: Add "command multiword add" and the ability to add script commands into a user multiword hierarchy.

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.

Sep 22 2021, 5:36 PM · Restricted Project
jingham added a comment to D110298: Add "command multiword add" and the ability to add script commands into a user multiword hierarchy.

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 22 2021, 5:35 PM · Restricted Project
jingham requested review of D110298: Add "command multiword add" and the ability to add script commands into a user multiword hierarchy.
Sep 22 2021, 5:30 PM · Restricted Project

Sep 15 2021

jingham added a comment to D109738: [lldb] Fix bug 38317 - Address breakpoints don't work if set before the process launches.

In the future, can you generate patches with context (i.e. pass -U9999 to
"git diff" or "git show"), it's a lot easier to read patches with context.

Sure thing, will do.

This doesn't seem like the right solution to the problem to me. The way

address breakpoints SHOULD work is that when you set the breakpoint, we
resolve the address you pass in against the images in the target. If the
address is in an image in the list, then we convert the load address to a
section relative address before making the breakpoint. Address breakpoints
made that way will always return true for m_addr.GetSection(), and so set
re_resolve to true before getting to the test you modified.

No, actually. In my scenario, the breakpoint is being created by load
address (via SBTarget::BreakpointCreateByAddress(addr_t address)), right
after the target has been created. Since the process hasn't been launched
at this point yet, there are no code sections mapped, so the breakpoint
address stays non-section-relative. My understanding is that in such a
case, LLDB should attempt to re-resolve it upon loading each new module.

Sep 15 2021, 10:32 AM · Restricted Project

Sep 14 2021

jingham added a comment to D109738: [lldb] Fix bug 38317 - Address breakpoints don't work if set before the process launches.

BTW, do you know what change made this stop working?

Sep 14 2021, 3:39 PM · Restricted Project