Page MenuHomePhabricator

jingham (Jim Ingham)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 23 2014, 5:24 PM (306 w, 11 h)

Recent Activity

Yesterday

jingham accepted D85235: [lldb] Make SBTarget::LaunchSimple start form the target's LaunchInfo.

LGTM

Tue, Aug 4, 6:44 PM · Restricted Project
jingham requested review of D85265: Add a setting to always run all threads when stepping.
Tue, Aug 4, 6:16 PM · Restricted Project
jingham added a comment to D85106: Move TestGuiBasicDebug.py to lldb/test and update it.

lldb/test/API is usually for testing the lldb::SB* interfaces IIRC. So not sure this move makes sense?

Tue, Aug 4, 4:19 PM · Restricted Project

Wed, Jul 29

jingham accepted D84527: Rename StoppointLocation to StoppointSite and drop its relationship with BreakpointLocation.

Remember to do the action thingie...

Wed, Jul 29, 10:12 AM · Restricted Project

Tue, Jul 28

jingham added inline comments to D84809: [lldb] Fix invalid error message in TargetList::CreateTargetInternal.
Tue, Jul 28, 5:02 PM · Restricted Project
jingham added a comment to D84809: [lldb] Fix invalid error message in TargetList::CreateTargetInternal.

This LGTM with a couple of quibbles but I haven't been working on the platform support recently so we should wait till people who have done so weight in.

Tue, Jul 28, 5:01 PM · Restricted Project
jingham accepted D84800: [lldb] Remove unused option '--platform-path' for 'target create'.

I'm a little curious how you would specify the path to your binary on a remote platform using the command line. But this way certainly wasn't it...

Tue, Jul 28, 3:05 PM · Restricted Project

Fri, Jul 24

jingham added a comment to D84498: [lldb] Add a --dummy option to the frame recognizer commands.

This is doing the right thing. Ismail and I talked a little off-line and I allayed his concerns.

Fri, Jul 24, 11:32 AM
jingham added a comment to D84527: Rename StoppointLocation to StoppointSite and drop its relationship with BreakpointLocation.

This overall change makes sense to me.

Fri, Jul 24, 11:16 AM · Restricted Project
jingham accepted D84475: [lldb] Inform every language runtime of the modified modules.

LGTM

Fri, Jul 24, 10:50 AM · Restricted Project

Wed, Jul 22

jingham added inline comments to D84257: [lldb] Don't use hardware index to determine whether a breakpoint site is hardware.
Wed, Jul 22, 3:40 PM · Restricted Project
jingham added a comment to D84307: [lldb/interpreter] Move the history subcommand to session (NFCI).

The current lldb behavior is that when you start up lldb, it reads the editline command-history file (so the history of the previous session), and loads it into it's internal command history, which you can get to by up and down arrows or ^R searches. But it doesn't prime the list that the "command history" command has access to. So command history starts out empty. But IMO, that's just bug. If a command shows up in scrolling through with up-arrow, it should also be listed in the history command wherever it lives. This really reduces the utility of the "history" command... I think we do want to show this. We might want mark in the "command history" output where the imported commands end and the new commands start, but we really should make them available.

Wed, Jul 22, 12:17 PM · Restricted Project

Tue, Jul 21

jingham accepted D84267: Thread ExecutionContextScope through GetByteSize where possible (NFC-ish).

I think you missed two more places to pass a target, then this looks good to me. How tedious, thanks for doing it...

Tue, Jul 21, 8:03 PM · Restricted Project
jingham added a comment to D84267: Thread ExecutionContextScope through GetByteSize where possible (NFC-ish).

I pointed out a couple of places where you have either at hand or near at hand a useful exe_ctx that you could pass instead of nullptr.

Tue, Jul 21, 4:54 PM · Restricted Project
jingham added a comment to D84272: Add checks for ValueObjectSP in Cocoa summary providers.

The !text test is correct, since you intend to pass *text in as a ValueObject &. But I wouldn't add the GetValueAsUnsigned check, that seems confusing. The NSStringSummaryProvider is returning a bool to tell you whether it succeeded or not, so it seems odd to pre-judge one of it's error states.

Tue, Jul 21, 3:36 PM · Restricted Project
jingham added a comment to D84267: Thread ExecutionContextScope through GetByteSize where possible (NFC-ish).

The patch lost seems to have lost everything but the PPC, SystemZ and MIPS code...

Tue, Jul 21, 3:21 PM · Restricted Project
jingham committed rG8d6aa688eeff: Remove the "bool" return from OptionValue::Clear and its subclasses. (authored by jingham).
Remove the "bool" return from OptionValue::Clear and its subclasses.
Tue, Jul 21, 11:33 AM
jingham closed D84253: OptionValue::Clear should return void not bool.
Tue, Jul 21, 11:33 AM · Restricted Project
Herald added a project to D84253: OptionValue::Clear should return void not bool: Restricted Project.
Tue, Jul 21, 11:01 AM · Restricted Project

Mon, Jul 20

jingham committed rGbc0a9a17a4a6: Add an option (-y) to "break set" and "source list" that uses the same file… (authored by jingham).
Add an option (-y) to "break set" and "source list" that uses the same file…
Mon, Jul 20, 9:19 PM
jingham closed D83975: Add an option to "break set" and "source list" that takes a line spec in the form file:line:column.
Mon, Jul 20, 9:19 PM · Restricted Project
jingham added a comment to D84210: [lldb] Use a weak pointer to hold on to the underlying thread plan in SBThreadPlan.

Yes, the only way you can get your hands on an SBThreadPlan is in the process of queueing one, so the ThreadPlan stack will always be the one to manage the lifecycle of a plan. And if an SBThreadPlan represents a ThreadPlan that has been discarded, it should convert back to an empty SBThreadPlan rather than keeping the underlying plan alive. So this is correct.

Mon, Jul 20, 9:16 PM · Restricted Project
jingham accepted D84210: [lldb] Use a weak pointer to hold on to the underlying thread plan in SBThreadPlan.

LGTM

Mon, Jul 20, 9:16 PM · Restricted Project

Fri, Jul 17

jingham accepted D84083: [lldb/ObjectFileMachO] Correctly account for resolver symbols.

LGTM.

Fri, Jul 17, 6:09 PM · Restricted Project
jingham added inline comments to D83975: Add an option to "break set" and "source list" that takes a line spec in the form file:line:column.
Fri, Jul 17, 4:25 PM · Restricted Project
jingham updated the diff for D83975: Add an option to "break set" and "source list" that takes a line spec in the form file:line:column.

Address review comments

Fri, Jul 17, 4:21 PM · Restricted Project

Thu, Jul 16

jingham added inline comments to D83975: Add an option to "break set" and "source list" that takes a line spec in the form file:line:column.
Thu, Jul 16, 5:50 PM · Restricted Project
jingham added inline comments to D83975: Add an option to "break set" and "source list" that takes a line spec in the form file:line:column.
Thu, Jul 16, 5:42 PM · Restricted Project
jingham updated the diff for D83975: Add an option to "break set" and "source list" that takes a line spec in the form file:line:column.

Address review comments.

Thu, Jul 16, 5:36 PM · Restricted Project
jingham added a comment to D83933: [lldb] Don't delete orphaned shared modules in SBDebugger::DeleteTarget.

BTW, note that target delete has a -c option which does:

Thu, Jul 16, 2:20 PM
jingham added a comment to D83933: [lldb] Don't delete orphaned shared modules in SBDebugger::DeleteTarget.

Who is calling delete target anyway? Can't be Xcode, because we have huge performance issues right now with Xcode 11.4.1 and later due to the "parse all line tables (and link them all if DWARF in .o files is being used) everywhere on the first source file and line breakpoint being set" and we don't see slowdowns in subsequent runs with Xcode (lldb-rpc-server).

Thu, Jul 16, 2:14 PM
jingham added a comment to D83933: [lldb] Don't delete orphaned shared modules in SBDebugger::DeleteTarget.

And if you really are trying to keep out-of-date modules from building up, DeleteTarget is not the correct time to do this. After all, this most commonly happens in command-line lldb when people do:

(lldb) file foo
(lldb) run
...exited
<go to another window, change some file & rebuild>
(lldb) run

Then you do that a whole bunch of times. You haven't ever deleted the Target in this scenario.

You are correct. That isn't what this patch is trying to do. The global file cache might already be doing what I suggested (ejecting older versions from the global cache), so there may be nothing to do.

But I do think deleting a target is a nice time to do module some cleanup if possible, though it might be better controlled with a bool argument to delete target instead of always being done.

If you are shared library linking against LLDB.framework and you rarely use LLDB, but you use it for symbolication every once and a while, it might be nice to be able to regain memory by deleting the target with an argument instead of now having to call SBDebugger::MemoryPressureDetected() and hope it cleans things up.

Thu, Jul 16, 12:30 PM
Herald added a project to D83975: Add an option to "break set" and "source list" that takes a line spec in the form file:line:column: Restricted Project.
Thu, Jul 16, 11:50 AM · Restricted Project
jingham added a comment to D83933: [lldb] Don't delete orphaned shared modules in SBDebugger::DeleteTarget.

And if you really are trying to keep out-of-date modules from building up, DeleteTarget is not the correct time to do this. After all, this most commonly happens in command-line lldb when people do:

Thu, Jul 16, 11:14 AM
jingham added a comment to D83933: [lldb] Don't delete orphaned shared modules in SBDebugger::DeleteTarget.

Note, you want to be careful how you reap executables whose binary has changed. If you are debugging with the debug info in .o files, and you change one .cpp file & rebuild, you can throw away the Module for the executable, but you don't want to throw away the modules that back the .o files that haven't changed.

Thu, Jul 16, 11:11 AM

Wed, Jul 15

jingham requested changes to D83876: [lldb] Add SBModule::ClearCachedModules and clear modules after each test run.

We certainly don't want to clear the shared module cache when the Debugger or the SBDebugger is destroyed. Most IDE's that use LLDB as a library use a debugger per debugging session, and destroy the debugger when the debugging session ends. As it stands now, when the first debugging session ends we don't remove any of the already parsed modules, so that the second debugging session is much faster. That would not be true if you did RemoveOrphanedSharedModules in Debugger::Destroy. 99% of all the libraries that get loaded by an app don't change over an Xcode run, and are in fact shared among most of the targets you might debug on a given platform. So clearing the cache after every run would result in a lot of pointless reparsing of system libraries.

Wed, Jul 15, 10:10 AM
jingham accepted D83757: [lldb] Store StackFrameRecognizers in the target instead of a global list.

This looks good. In the breakpoint case, I added a --dummy option so that you could set breakpoints directly on the dummy target. You would do that if you were going to do something that would make new targets, and want to ensure that the breakpoints get set in all the new targets. That's probably a very little used feature, so while it would be nice to maintain consistency amongst commands that add to the dummy target, that's definitely extra-credit.

Wed, Jul 15, 9:55 AM · Restricted Project

Tue, Jul 14

jingham committed rG2ad1fc0bd2cf: Fix the crashlog.py script's use of the load_address property. (authored by jingham).
Fix the crashlog.py script's use of the load_address property.
Tue, Jul 14, 4:53 PM
jingham committed rG79faa6349d74: Handle eExpressionThreadVanished in error switch to handle covered switch… (authored by echristo).
Handle eExpressionThreadVanished in error switch to handle covered switch…
Tue, Jul 14, 4:52 PM
jingham committed rG9c8d20432423: [lldb] Increase timeout in TestExitDuringExpression (authored by labath).
[lldb] Increase timeout in TestExitDuringExpression
Tue, Jul 14, 4:51 PM
jingham committed rG7606653e3ada: Maybe I need ENABLE_THREADS in the Makefile. (authored by jingham).
Maybe I need ENABLE_THREADS in the Makefile.
Tue, Jul 14, 4:51 PM
jingham committed rGdd82f99fecb1: Handle the case where a thread exits while we are running a function on it. (authored by jingham).
Handle the case where a thread exits while we are running a function on it.
Tue, Jul 14, 4:51 PM
jingham committed rGd526af3c318a: This very simple .c file is failing on the Debian bot wit the error (authored by jingham).
This very simple .c file is failing on the Debian bot wit the error
Tue, Jul 14, 4:51 PM
jingham committed rGf4db8e04fce5: [lldb/DWARF] Fix a leak in line table construction (authored by labath).
[lldb/DWARF] Fix a leak in line table construction
Tue, Jul 14, 4:49 PM
jingham committed rG0bdf7efda250: Fix LLDB debug builds (authored by Walter Erquinigo <wallace@fb.com>).
Fix LLDB debug builds
Tue, Jul 14, 4:42 PM
jingham committed rGe1771f53f6f6: Fix a couple of problems caused by merging ThreadPlanStack changes to this… (authored by jingham).
Fix a couple of problems caused by merging ThreadPlanStack changes to this…
Tue, Jul 14, 4:41 PM
GitHub <noreply@github.com> committed rG5e13d780be3c: Merge pull request #1025 from jimingham/thread-plans-outlive-threads (authored by jingham).
Merge pull request #1025 from jimingham/thread-plans-outlive-threads
Tue, Jul 14, 4:39 PM
jingham committed rG0349f1ec31d2: Allow the ThreadPlanStackMap to hold the thread plans for threads that were not… (authored by jingham).
Allow the ThreadPlanStackMap to hold the thread plans for threads that were not…
Tue, Jul 14, 4:39 PM
jingham committed rG96399ae57991: Move thread plan stacks into the Process, indexed by TID. (authored by jingham).
Move thread plan stacks into the Process, indexed by TID.
Tue, Jul 14, 4:39 PM
jingham committed rGf9e807282756: Make ThreadPlanTracers use TID & Process rather than Thread *. (authored by jingham).
Make ThreadPlanTracers use TID & Process rather than Thread *.
Tue, Jul 14, 4:39 PM
jingham committed rG3b84a5f76052: Make ThreadPlans use TID and Process, rather than Thread *. (authored by jingham).
Make ThreadPlans use TID and Process, rather than Thread *.
Tue, Jul 14, 4:39 PM
GitHub <noreply@github.com> committed rG25ad65c52aa6: Merge pull request #653 from jimingham/direct-dispatch-b (authored by jingham).
Merge pull request #653 from jimingham/direct-dispatch-b
Tue, Jul 14, 4:16 PM
jingham committed rGb168295f1951: Clang added a new feature to the ObjC compiler that will translate method calls… (authored by jingham).
Clang added a new feature to the ObjC compiler that will translate method calls…
Tue, Jul 14, 4:16 PM
jingham added a comment to D83757: [lldb] Store StackFrameRecognizers in the target instead of a global list.

The change looks pretty straightforward, and having the recognizers be per target seems like a better model to me. The one difficulty with adding a target to the recognizers is that you can't add a recognizer in your .lldbinit file, since you don't have a target yet. If anybody has gotten around to doing that yet (the recognizers aren't brand new at this point) will be broken by this change.

Tue, Jul 14, 10:55 AM · Restricted Project

Mon, Jul 13

jingham accepted D83728: [lldb] Make `process connect` blocking in synchronous mode. .

LGTM

Mon, Jul 13, 4:41 PM · Restricted Project
jingham added a comment to D83728: [lldb] Make `process connect` blocking in synchronous mode. .

Couple of minor comments.

Mon, Jul 13, 4:16 PM · Restricted Project

Fri, Jul 10

jingham committed rGe337350be9d6: This is a refinement on 96601ec28b7efe5abf3479a1aa91bcedb235bbbd. The intent of… (authored by jingham).
This is a refinement on 96601ec28b7efe5abf3479a1aa91bcedb235bbbd. The intent of…
Fri, Jul 10, 11:12 AM
jingham closed D83450: Delegate UpdateChildrenPointerType to the Root ValueObject.
Fri, Jul 10, 11:12 AM · Restricted Project
jingham added a comment to D83450: Delegate UpdateChildrenPointerType to the Root ValueObject.

Right. It does bug me a little that this has no test, but I don't think it's the worst that could happen. And you're the one who's going to have to debug this all over again if the next value object change breaks swift again.

Fri, Jul 10, 10:23 AM · Restricted Project
jingham added a comment to D83446: [WIP][lldb/Reproducers] Synchronize the command interpreter with asynchronous events.

Seems like my thoughts got lost a bit with all the inline replies: we can solve this particular issue by making process connect block in synchronous mode. The fact that that's not happening today is a bug beyond the reproducers. I don't think we should change the current behavior in asynchronous mode. I think it's probably worthwhile to do that even if we decide to add extra synchronization for the reproducers in which case this would no longer be an example of a problem solved by the current patch.

I'm starting to understand why you say "process connect" is not blocking/synchronous. Although it is waiting for the "stopped" event to be sent to the public queue (here), it is not waiting for it to be processed by the public queue. The "process continue" command OTOH, is kind of waiting for the public queue processing to finish. I say only "kind of" because it does not actually process it on the _real_ public queue -- instead it "hijacks" the events and processes then on its own little event loop. The part that I got wrong was that I was assuming that the hijacked event loop would rebroadcast the stop event to the real public queue. In fact, it doesn't, which means that the stop event processing is completely finished by the time that the "process continue" command returns.

It seems reasonable that "process connect" (and it's SB equivalent) should behave the same way. This would also explain why so many of the "gdb-client" tests need to use lldbutil.expect_state_changes when connecting to the mock server, even though they are running in synchronous mode (the only other tests using this function explicitly choose to use the async mode).

For async mode, we can say that the user needs to ensure that the event is handled before he can issue other commands -- this is the same situation as with "process continue". Of course, in the asynch case, the reproducers are "the user" and they will need to do something about this. But maybe we don't need to cross that bridge right now?

Fri, Jul 10, 8:56 AM

Thu, Jul 9

jingham added a comment to D83433: Fix how we handle bit-fields for Objective-C when creating an AST.

IIRC from when Shafik and I were looking at this the ObjC runtime data lists the same offset for all the members of the bitfield: the offset of the first member of the bitfield. Apparently the ObjC runtime doesn't need to know where the individual members are dynamically - the offsets are baked into the code or something? So to find where the member actually is you have to take the offset into the field - which is given in the DWARF, and add it to the dynamic offset you get from the runtime for the first member.

Thu, Jul 9, 2:25 PM · Restricted Project
jingham added a comment to D83446: [WIP][lldb/Reproducers] Synchronize the command interpreter with asynchronous events.

I want to be careful not to conflate lldb's general event behavior with the way the command interpreter happens to run things.

Thu, Jul 9, 1:46 PM
jingham added a comment to D83446: [WIP][lldb/Reproducers] Synchronize the command interpreter with asynchronous events.

I think the proper way to gate this sort of "race" is to use the process state. If the current process state is "stopped" then that means we're done with the stop processing and ready to allow commands that require the process to be stopped.

When I originally did this, the plan was that the the Public state would not be set to stopped until the stop event was consumed by the process listener. So if you tried to "continue" the process before the work to handle the stop event was done, you would see the state as "running" and the continue would not succeed.

However, that ran up against the problem that part of the "asynchronous" work that you might do on stop can involve running commands and SB API. Those need to see a public stop state or they won't work correctly. So the public state gets set to stopped too early, and then it's possible for this to be racy. IMO the solution to this problem should be for the Event Handler to be able to run its commands "as if" the public state were stopped, but everybody else who is trying to run commands will still see it as running. Then when the event handler is done with it's job, the process Public state is switched to stopped, and now commands coming from other sources will run in the stopped state.

How would that work with the reproducer issue? How would you prevent the command interpreter from processing the next command before the event handler thread issues the process info packet?

Thu, Jul 9, 1:36 PM
jingham added a comment to D83446: [WIP][lldb/Reproducers] Synchronize the command interpreter with asynchronous events.

I think the proper way to gate this sort of "race" is to use the process state. If the current process state is "stopped" then that means we're done with the stop processing and ready to allow commands that require the process to be stopped.

Thu, Jul 9, 11:54 AM
jingham added a comment to D83450: Delegate UpdateChildrenPointerType to the Root ValueObject.

I am not selling this as the correct long-term structure for ValueObjects. So far as I can see, in all cases it should be possible to tell a ValueObject where it's going to find both its own store and the storage for pointer children when the ValueObject is created. It's not like that's a feature we discover as we go through the process of creating the ValueObject. That isn't how it is currently done. Now we almost always start off an incorrect setting, and often goes through several rounds of resetting before it settles on the final value. That's what I meant by saying it is "frustratingly self-healing".

Thu, Jul 9, 10:24 AM · Restricted Project

Wed, Jul 8

Herald added a project to D83450: Delegate UpdateChildrenPointerType to the Root ValueObject: Restricted Project.
Wed, Jul 8, 7:51 PM · Restricted Project

Tue, Jul 7

jingham accepted D83327: [lldb/Core] Fix incomplete type variable dereferencing crash..

Other than a quibble about using _sp suffix, this is fine.

Tue, Jul 7, 11:33 AM · Restricted Project
jingham added a comment to D83327: [lldb/Core] Fix incomplete type variable dereferencing crash..

Aside from cosmetics, I'm not entirely sure this is the correct fix. Why are we calling this code _at all_ if the type is incomplete?

Tue, Jul 7, 11:05 AM · Restricted Project
jingham accepted D83306: [lldb/API] Overwrite variables with SBLaunchInfo::SetEnvironment(append=true).

This is fine. If there were a universal convention (e.g. : separated lists) for environment variables, it would be reasonable for "append" behavior to extend rather than replace extant variables, but in the absence of the universality of such a convention, the best we can do is overwrite.

Tue, Jul 7, 9:46 AM · Restricted Project

Jun 16 2020

jingham added a comment to D81810: LLDB step-instruction gets stuck on jump to self.

Yes, I wasn't sure what the exact semantics were for step-inst for LLDB. I think the issue is not mainly the mimicking of GDB behavior, but rather that it is inconvenient for some use-cases.

To give some context on why I propose this change: the current behavior is pretty cumbersome when doing automated testing with LLDB of unknown/ generated programs. For example making use of the Python API, AFAIK, there is no interface where you can run a bounded number of instructions if lldb.SBThread.StepInstruction is not guaranteed to return (the current hack around it would be run this in a separate thread with a timeout).

Jun 16 2020, 9:54 AM · Restricted Project

Jun 15 2020

jingham added a comment to D81810: LLDB step-instruction gets stuck on jump to self.

Just to be clear, the lldb -> gdb command map doesn't prescribe behavior for lldb commands. It just suggests the analogous command in gdb. We are still free to implement lldb behavior however seems best to us.

Jun 15 2020, 3:29 PM · Restricted Project

Jun 11 2020

jingham accepted D80112: Check if thread was suspended during previous stop added..

LGTM

Jun 11 2020, 11:32 AM · Restricted Project
jingham added a comment to D74136: [LLDB] WIP: Follow DW_AT_decl_file when setting breakpoint.

I need to read this in more detail at this point but I'm caught up in something else.

One of the not so great design decisions I made was to try to have "break set" be a single command governed by flags. The way the command turned out, there are some flags that tell you what kind of breakpoint you are setting, and then other flags that qualify that kind of search. But that means --file can mean "I'm specifying the file to use to set a file & line breakpoint" or it can be a narrowing specifier for a source regex or function name based breakpoint. Doing it this way makes it hard to have consistent meanings for the flags in all contexts.

If I had it to do over, I would make:

(lldb) break set source
(lldb) break set symbol
(lldb) break set source-pattern
(lldb) break set address

Jun 11 2020, 9:53 AM · Restricted Project
jingham added a comment to D74136: [LLDB] WIP: Follow DW_AT_decl_file when setting breakpoint.

I need to read this in more detail at this point but I'm caught up in something else.

Jun 11 2020, 9:53 AM · Restricted Project

Jun 10 2020

jingham accepted D81499: [Debugger] Use FileSystem instead of calling llvm::sys::fs::openFileForWrite directly..

LGTM then.

Jun 10 2020, 3:39 PM · Restricted Project
jingham added a comment to D81499: [Debugger] Use FileSystem instead of calling llvm::sys::fs::openFileForWrite directly..

This looks okay to me though I'm not very familiar with the llvm file system interfaces.

Jun 10 2020, 3:35 PM · Restricted Project
jingham added a comment to D80112: Check if thread was suspended during previous stop added..

Thanks, this is looking good. I have a bunch of nits, but nothing substantial.

Jun 10 2020, 3:35 PM · Restricted Project

Jun 8 2020

jingham added a comment to D80112: Check if thread was suspended during previous stop added..

I thought I was just repeating your original description of the problem in a scenario orchestrated by breakpoint actions. I must have missed something in your description if it's true that the scenario I described doesn't match your initial problem. But regardless, if there was a problem you should be able to actually drive lldb, either through the command-line or the SB API to the point of failure. And if so, you should be able to write a test that does the same thing.

Jun 8 2020, 4:39 PM · Restricted Project
jingham added a comment to D80112: Check if thread was suspended during previous stop added..

Humm... So you'll have to test the continue behavior instead, which after all was your original issue. That shouldn't be too hard, however. Just make a breakpoint action that calls "thread suspend" on its thread and returns false for should_stop the first time it is called, and just returns true every time thereafter. Then the program should stop at the second hit of the breakpoint rather than continuing to the exit.

Jun 8 2020, 2:58 PM · Restricted Project
jingham added a comment to D80112: Check if thread was suspended during previous stop added..

If I understand the problem you are describing, it is that you suspended a thread as a user-level suspend, so it's stop reason got stuck. That seems okay, its probably useful to keep the stop reason of the thread at what it was when you suspended it. But that means when gets asked to do its action again, the action is stale and does the wrong thing. If that's what's going wrong,

Yes, you understood right, that's exactly what was going wrong.

Actually, you can more easily do that. The thread iteration where we call PerformAction etc. works on a copy of the thread list (since one thread's action could cause the thread list to change). So if you just change the copy operation to only copy over threads which aren't user-suspended, then you should be set.

I don't see that's a copy, it seems that's a reference on the actual thread list:

ThreadList &curr_thread_list = process_sp->GetThreadList();
Jun 8 2020, 12:08 PM · Restricted Project

Jun 4 2020

jingham added a comment to D80112: Check if thread was suspended during previous stop added..

Adding a ShouldStop to that if test doesn't seem right. You should still run the stop actions even if the thread doesn't want to stop.

If I understand the problem you are describing, it is that you suspended a thread as a user-level suspend, so it's stop reason got stuck. That seems okay, its probably useful to keep the stop reason of the thread at what it was when you suspended it. But that means when gets asked to do its action again, the action is stale and does the wrong thing. If that's what's going wrong, then it would make more sense short-circuit suspended threads earlier on in the iteration, like:

lldb::ThreadSP thread_sp = curr_thread_list.GetThreadAtIndex(idx);
// This thread was suspended by the user during this run, so it's actions are stale:
if (thread_sp->GetResumeState() == eStateSuspended)
    continue;

That mirrors what the ThreadList::ShouldStop does. You actually probably don't want to do it quite this way, because it is possible that one thread's breakpoint action could resume another thread. So you should probably first run through the thread list and get all the suspended threads, then run through again doing the actions and skipping the suspended threads you found in the first iteration.

Jun 4 2020, 6:46 PM · Restricted Project
jingham added a comment to D80112: Check if thread was suspended during previous stop added..

Adding a ShouldStop to that if test doesn't seem right. You should still run the stop actions even if the thread doesn't want to stop.

Jun 4 2020, 6:14 PM · Restricted Project
jingham added a comment to D80112: Check if thread was suspended during previous stop added..

It bugs me a little bit that we are doing work masking the stop info when in fact the ShouldStop mechanism is the one that shouldn't be consulting threads that were suspended by the user during the last run.

Jun 4 2020, 2:23 PM · Restricted Project
jingham added a comment to D80112: Check if thread was suspended during previous stop added..

Sorry, now that I'm thinking about this more, I'm confused as to why you are seeing the symptoms you describe. Thread::ShouldStop starts with:

Jun 4 2020, 2:23 PM · Restricted Project
jingham committed rGa976a7fcae44: Disable this test for Windows. (authored by jingham).
Disable this test for Windows.
Jun 4 2020, 11:03 AM

Jun 3 2020

jingham committed rGf4d427326539: Add a test for preserving state on the non-expr thread across expression… (authored by jingham).
Add a test for preserving state on the non-expr thread across expression…
Jun 3 2020, 2:56 PM
jingham added a comment to D80112: Check if thread was suspended during previous stop added..

The one scenario I can think of where this might do the wrong thing is:

Jun 3 2020, 2:55 PM · Restricted Project

May 29 2020

jingham added a comment to D80848: [lldb/Bindings] Raise a Runtime error when using SBAddress properties that rely on lldb.target.

Looks even better.

May 29 2020, 4:24 PM · Restricted Project
jingham accepted D80848: [lldb/Bindings] Raise a Runtime error when using SBAddress properties that rely on lldb.target.

I think "This resolves the SBAddress" is better than "This resolves SBAddress". Other than that LGTM.

May 29 2020, 3:52 PM · Restricted Project
jingham added a comment to D80848: [lldb/Bindings] Raise a Runtime error when using SBAddress properties that rely on lldb.target.

Ooh, test would be good. Just use it in a python command.

May 29 2020, 3:52 PM · Restricted Project
jingham added a comment to D78801: [LLDB] Add class WasmProcess for WebAssembly debugging.

Hello @clayborg , @labath,
Any thoughts on this latest patch? :-)

Sorry about the delay. I think that in terms of the design we've come as far as we can without making substantial changes to a lot of lldb interfaces. The question on my mind is.. is that enough?

Here, I am mainly thinking about the introduction of the ExecutionContext argument to ReadMemory. In a universe with static flat address spaces, the argument looks flat out wrong. If one thinks of "memory" as something more dynamic, it does not seem to be that bad, as it can be viewed as the context in which to interpret the memory addresses. However, I am having trouble assigning a semantic to it besides saying "it does what webassembly needs". Maybe it's just because I live in a flat universe and lack address space intuition...

Anyway, I think it would be good to get more people's opinions on this. For start, I nominate Jim. :)

The problem with "looking forward to implementing more cleanly in the future with AddressSpecifiers" is that _everyone_ is looking forward to having address spaces, but noone is actually working on implementing them. And I want to be careful about accumulating technical debt like this upstream, because it's the technical debt which makes future implementations hard.

May 29 2020, 12:00 PM · Restricted Project
jingham added inline comments to D80724: [lldb] Only set the executable module for a target once.
May 29 2020, 10:53 AM · Restricted Project

May 28 2020

jingham added inline comments to D74136: [LLDB] WIP: Follow DW_AT_decl_file when setting breakpoint.
May 28 2020, 3:25 PM · Restricted Project
jingham requested changes to D80724: [lldb] Only set the executable module for a target once.

I don't think that test is right. There's no guarantee that your GetExecutable().GetFilename() runs before or after the dlopen in the process has run. If it runs before you aren't testing anything.

May 28 2020, 3:25 PM · Restricted Project
jingham committed rG723a1caa377b: Fix the crashlog.py script's use of the load_address property. (authored by jingham).
Fix the crashlog.py script's use of the load_address property.
May 28 2020, 10:25 AM
jingham closed D80680: save_crashlog should not be using the load_addr property of an SBAddress.
May 28 2020, 10:25 AM · Restricted Project
jingham added a comment to D79929: [lldb] Tab completion for process plugin name.

That does make sense. I will split it into eArgTypeProcessPlugin and eArgTypeDisassemblePlugin along with the corresponding completion function names at the time I implement the disassemble related completion function.

May 28 2020, 9:50 AM · Restricted Project

May 27 2020

jingham created D80680: save_crashlog should not be using the load_addr property of an SBAddress.
May 27 2020, 6:02 PM · Restricted Project
jingham added a comment to D79929: [lldb] Tab completion for process plugin name.

This change makes the eArgTypePlugin be the completion for process plugins. Shouldn't we change the enum name and completion name to indicate that?

May 27 2020, 10:17 AM · Restricted Project

May 26 2020

jingham added a comment to D79823: [lldb][Core] Remove dead codepath in Mangled.

Note that there is no guarantee that ObjC names won't be mangled. They aren't on macOS because the macOS linker grew up along with objC so all the weird characters it introduces into the symbol namespace ([,],-,+, ,etc...) are allowed. Way back in the day when WebObjects ran on Windows ObjC names weren't legal, and were mangled. Just saying...

May 26 2020, 5:28 PM · Restricted Project