Page MenuHomePhabricator

jingham (Jim Ingham)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 23 2014, 5:24 PM (287 w, 6 d)

Recent Activity

Fri, Mar 27

jingham added inline comments to D76814: Preserve ThreadPlanStacks for unreported threads.
Fri, Mar 27, 10:55 AM · Restricted Project

Thu, Mar 26

jingham added a comment to D76814: Preserve ThreadPlanStacks for unreported threads.

Addressed most of the review comments. I don't see that using file check would make the plan output checking any easier. The current function seems clear and easy to use. Trying to dynamically insert the passed in matches into a filecheck test file and then run file check on that doesn't seem like it would be any easier than what is here. I don't need any of the substitutions or any of the other fancy things FileCheck provides, that just seems like overkill. But maybe I'm misunderstanding what you meant.

Thu, Mar 26, 2:42 PM · Restricted Project
jingham updated the diff for D76814: Preserve ThreadPlanStacks for unreported threads.

Addressed most of Pavel's review comments.

Thu, Mar 26, 2:42 PM · Restricted Project
jingham added inline comments to D76758: Augment lldb's symbol table with external symbols in Mach-O's dyld trie.
Thu, Mar 26, 11:22 AM · Restricted Project

Wed, Mar 25

jingham created D76814: Preserve ThreadPlanStacks for unreported threads.
Wed, Mar 25, 5:53 PM · Restricted Project
jingham added a parent revision for D76814: Preserve ThreadPlanStacks for unreported threads: D75880: [NFC} Move ThreadPlans stacks into their own class, store it in Process by TID.
Wed, Mar 25, 5:53 PM · Restricted Project
jingham added a child revision for D75880: [NFC} Move ThreadPlans stacks into their own class, store it in Process by TID: D76814: Preserve ThreadPlanStacks for unreported threads.
Wed, Mar 25, 5:53 PM · Restricted Project
jingham added a comment to D76805: Fix SourceManager::SourceFileCache insertion.

IIUC, you should be able to test the actual effect you are trying to achieve by having a large source file, running "source list" to get it into the File Manager, then try to open the source file for writing in a process in a subshell that won't have the same mmap. That should fail without the patch (or with the setting turned to cache-on) and succeed with it.

Wed, Mar 25, 5:21 PM · Restricted Project
jingham added a reviewer for D75750: [lldb] integrate debuginfod: clayborg.

Greg originally designed the macOS equivalent of this, so I've added him.

Wed, Mar 25, 2:38 PM · Restricted Project

Tue, Mar 24

jingham added a comment to D76672: [lldb/Reproducers] Always collect the whole dSYM in the reproducer.

Also, the dSYM bundle for a binary contains both the debug information and scripts that may be useful in debugging that binary - data formatters and the like. So even if the original debugging session crashed before they got around to using any of the facilities in their dSYM, they are still likely to come in handy in analyzing the state of the app in the reproducer. For this reason, I think we should treat dSYM's as special and capture them in full.

Tue, Mar 24, 12:21 PM · Restricted Project

Mon, Mar 23

jingham committed rG67d67ebe8f25: Internal expressions shouldn't increment the result variable numbering. (authored by jingham).
Internal expressions shouldn't increment the result variable numbering.
Mon, Mar 23, 1:39 PM
jingham closed D76532: Internal expressions should not increment the next result variable numbering.
Mon, Mar 23, 1:39 PM · Restricted Project

Fri, Mar 20

jingham added a comment to D76470: [lldb/Target] Rework the way the inferior environment is created.

Except for the suggestion to use eCommandRequiresTarget for the new command, this looks good.

Fri, Mar 20, 5:22 PM · Restricted Project
jingham created D76532: Internal expressions should not increment the next result variable numbering.
Fri, Mar 20, 3:45 PM · Restricted Project
jingham added a comment to D76470: [lldb/Target] Rework the way the inferior environment is created.

Is there any command-based way to see the entire environment that a process will get when it launches? By populating target.env-vars with the inherited environment there was a way to mostly do that, but I don't see that anymore. It seems a shame not to be able to see that...

No, there is no way to do this, but it's not really a regression. If you do settings show target.env-vars today before running then you won't see what is going to be passed. Only after the first run will it be populated. This was a surprise to me and I find it highly inconsistent. Maybe I can add another property that would get updated with the computed environment. Do we have anything like read-only properties?

Fri, Mar 20, 11:55 AM · Restricted Project
jingham added a comment to D76470: [lldb/Target] Rework the way the inferior environment is created.

Is there any command-based way to see the entire environment that a process will get when it launches? By populating target.env-vars with the inherited environment there was a way to mostly do that, but I don't see that anymore. It seems a shame not to be able to see that...

Fri, Mar 20, 10:17 AM · Restricted Project

Thu, Mar 19

jingham added a comment to D76407: [lldb/testsuite] Remove "healthy process" test from TestProcessCrashInfo.py.

This test just seems wrong to me. We can pretend that processes that haven't crashed don't have crash_info, and manually suppress the output in lldb if the stop state isn't a crash. But it seems useful to ask "what are the crash_info bits in flight right now" even if you haven't crashed. So that doesn't seem right to me. Or you have to acknowledge that you have no way of knowing whether there is crash info data hanging around, in which case any test that depends on there being none is an invalid test. I'd delete this whole test, since it seems to me like it will only ever succeed by accident.

Thu, Mar 19, 9:45 AM · Restricted Project
jingham accepted D76216: Improve step over performance.

Sorry, forgot to select the action...

Thu, Mar 19, 9:45 AM · Restricted Project

Tue, Mar 17

jingham added inline comments to D76188: [lldb/Target] Support more than 2 symbols in StackFrameRecognizer.
Tue, Mar 17, 12:56 PM · Restricted Project
jingham added inline comments to D76188: [lldb/Target] Support more than 2 symbols in StackFrameRecognizer.
Tue, Mar 17, 11:49 AM · Restricted Project
jingham added inline comments to D76188: [lldb/Target] Support more than 2 symbols in StackFrameRecognizer.
Tue, Mar 17, 10:10 AM · Restricted Project

Mon, Mar 16

jingham added a comment to D76216: Improve step over performance.

This looks fine to me.

Mon, Mar 16, 10:52 AM · Restricted Project
jingham added a comment to D76188: [lldb/Target] Support more than 2 symbols in StackFrameRecognizer.

Being able to match multiple symbols sounds useful in its own right, but the motivating case is a bit shady. raise, __GI_raise and gsignal are all aliases to one another (they have the same address). The reason you're sometimes getting gsignal here is not because some glibcs really call this function from their assert macro. It's because we happen to pick that symbol (maybe because it comes first in the symtab, depending on how the library was linked) when doing address resolution.

I'm wondering if that doesn't signal a flaw in the recognizer infrastructure. If we changed the matching logic so that it resolves the name it is supposed to search for, and then does a match by address, then only one name would be sufficient here.

Mon, Mar 16, 10:20 AM · Restricted Project

Fri, Mar 13

jingham added a comment to D76105: [lldb/settings] Reset the inferior environment when target.inherit-env is toggled.

Yeah, this shortcoming was pretty obvious while writing the patch. I don't like it very much, it seems like the inherit behavior should be handled closer to the point we launch, Or at least the inherited environment should be stored separately from the one you set manually. Comparing values would solve one class of issues, but what about explicitly setting one variable to the same value it has in your environment. Then you would delete it when changing the inherit-env property. Also not very intuitive.

Yeah, I've been thinking about that too. Is it now too late to do anything about that?

In my ideal world the "env-vars" setting would only hold the values that the user has set, and nothing more. And there would be a separate way to get the effective enviroment used for launches and what not. Maybe something like:

  • platform environment => get the current platform's environment -- this is the thing that would get inherited
  • process environment => get the effective environment -- if a process is running it's the environment the process was launched with, if it's not yet started, it's the (merged) environment it would be launched with. Or maybe these should be separate commands?
Fri, Mar 13, 10:11 AM · Restricted Project
jingham added a comment to D76111: Create basic SBEnvironment class.

Yes, a key/value approach would be a better API. Looks like the Environment class would make that pretty easy to wrap up, as well.

Fri, Mar 13, 9:39 AM · Restricted Project

Thu, Mar 12

jingham requested changes to D76111: Create basic SBEnvironment class.

Thanks for doing this, it will be useful!

Thu, Mar 12, 7:47 PM · Restricted Project
jingham added a comment to D75711: [NFC] Have ThreadPlans hold onto the Process & TID, rather than the Thread.

Everything looks good, just a question in inlined comment about having a thread plan hold onto a pointer to a thread. Seems dangerous

Thu, Mar 12, 5:23 PM · Restricted Project
jingham added a comment to D76105: [lldb/settings] Reset the inferior environment when target.inherit-env is toggled.

If I'm following the logic correctly, if you run a target with inherit-env off, and then do:

Thu, Mar 12, 4:18 PM · Restricted Project
jingham added a comment to D75711: [NFC] Have ThreadPlans hold onto the Process & TID, rather than the Thread.

Is this one okay as well?

Thu, Mar 12, 3:11 PM · Restricted Project
jingham added a comment to D75880: [NFC} Move ThreadPlans stacks into their own class, store it in Process by TID.

Removed Update till the next patch, and fixed a bug in PushPlan that preparing the next patch uncovered.

Thu, Mar 12, 12:31 PM · Restricted Project
jingham updated the diff for D75880: [NFC} Move ThreadPlans stacks into their own class, store it in Process by TID.

Removed ThreadPlanStackMap::Update, fixed a bug in PushPlan (can't std::move the ThreadPlan, THEN log it...)

Thu, Mar 12, 12:29 PM · Restricted Project
jingham added a comment to D76080: Adjust error_msg handling for expect_expr in lldbtest.py.

The result from EvaluateExpression is pretty much always going to be valid, since the result holds the error. The correct way to do the first check is:

Thu, Mar 12, 11:23 AM
jingham added a comment to D76009: [lldb/Target] Initialize new targets environment variables from target.env-vars.

BTW, don't worry about the OS_ACTIVITY_DT_MODE env var. That's something you have to set when debugging or the Foundation loggging method (NSLog) will go to the system console not to the current terminal, so we force it on all processes on macOS...

Thu, Mar 12, 10:51 AM · Restricted Project
jingham added a comment to D76009: [lldb/Target] Initialize new targets environment variables from target.env-vars.

Actually, hang on.

One existing test had to be modified, because the initialization of
the environment properties now take place at the time the target is
created, not at the first use of the environment (usually launch
time).

Just to be clear, in that test the host environment was modified between the creation of the target and the launch which I think is pretty unlikely to matter in practice.

Does this mean that the target-local value of the target.inherit-env setting no longer has any effect? Currently I can set it after creating a target (but before launching) to stop the process inheriting the default environment:

(lldb) file /usr/bin/env
Current executable set to '/usr/bin/env' (x86_64).
(lldb) set set target.inherit-env false
(lldb) pr la
Process 26684 launched: '/usr/bin/env' (x86_64)
Process 26684 exited with status = 0 (0x00000000)

I take it that after this, that will no longer be possible? It would still be possible to do that by setting the global value of the setting before creating a target, but the target-local setting would be useless (?)

The inherited environment is collected the first time the property is accessed and running the callback triggers this. For some reason just doing set show target.env-vars doesn't trigger it though, I'd need to debug this to understand. So yes, with this change, the target.inherit-env setting will take effect at target creation time and not at whatever next point in time the lazy logic is triggered.

One way to fix this is to add another callback for the target.inherit-env property and remove the variables that are in the environment.

I'm not really sure how these settings are supposed to work, but this does not seem ideal to me.

Jim would certainly be able to explain the intent here.

Thu, Mar 12, 10:51 AM · Restricted Project

Wed, Mar 11

jingham accepted D76011: Add a verification mechanism to CompilerType..

Seems fine to me.

Wed, Mar 11, 12:29 PM
jingham added inline comments to D75880: [NFC} Move ThreadPlans stacks into their own class, store it in Process by TID.
Wed, Mar 11, 11:18 AM · Restricted Project
jingham updated the diff for D75880: [NFC} Move ThreadPlans stacks into their own class, store it in Process by TID.

more reformatting

Wed, Mar 11, 10:45 AM · Restricted Project

Tue, Mar 10

jingham updated the diff for D75880: [NFC} Move ThreadPlans stacks into their own class, store it in Process by TID.

Fix some clang-format nits. I didn't change:

Tue, Mar 10, 5:33 PM · Restricted Project
jingham added a comment to D75880: [NFC} Move ThreadPlans stacks into their own class, store it in Process by TID.

Addressed Pavel's review comments.

Tue, Mar 10, 4:26 PM · Restricted Project
jingham updated the diff for D75880: [NFC} Move ThreadPlans stacks into their own class, store it in Process by TID.

Addressed Pavel's review comments.

Tue, Mar 10, 4:26 PM · Restricted Project
jingham added a comment to D75711: [NFC] Have ThreadPlans hold onto the Process & TID, rather than the Thread.

So this is technically not "our" fault. The plugin is doing something unsupported and gets weird behavior as a result. Now, I am not saying we should throw this plugin (maybe the only actually used os plugin) overboard. However, I was hoping that the process of supporting it would involve some improvements to the os plugin interface (so that it can do what it wants in a supported way), rather than declaring support for whatever it is the plugin is doing, even though we know it to be problematic (like, not being able to tell when we can throw thread plans away).

I think when presenting an OS's threads through the funnel of a monitor stub that only knows about cores and not about the OS's concept of threads, you will very likely run into the problem where reconstructing all the threads at every stop is expensive and not particularly desirable. Two of the three instances of OS plugins that I know about do it this way. The other is for wrapping a cooperative multi-threading library, where fetching all the threads was pretty easy and there weren't too many. But for instance if getting all the threads really is slow, for the OS plugin to be useful we need to allow it to do partial reporting to be useful.

Anyway, I don't know that we need anything more added to the OS interface. There's a "make a thread for a TID" interface, which we can use to reap the persistent part of the thread, for instance when we make a public stop. If that's fast enough (I haven't gotten to that point with the xnu plugin) we can us that. We can add an API to query whether the plugin returns all threads, and gate our behavior on that rather than on "Has OS Plugin". But that's about all I can think of.

Yes, that's sort of what I was expecting. Another idea I had was to have a way to avoid reading the register context initially, and only have it be fetched on demand.

I also have to support all the OS plugins in all the xnu dSYM's that are currently in the wild, too. So whatever we do has to work without requiring added behaviors.

Yep, anything we do shouldn't break existing plugins, but I think its fair to require some modification to an existing plugin in order to enable a new feature (such as stepping across thread reschedules).

That said, I don't really use or care about the os plugins, so if using it results in leaking thread plans, then whatever. The thing I want to understand is what kind of requirements does this place on the rest of lldb. Which leads me to my next comment..

but I think it fits the kernel folks workflow, since the actual number of threads in the kernel is overwhelming and not terribly useful. But you are right, if we wanted to delay getting threads on private stops, but make UpdateThreadListIfNeeded force a fetch, then we would have to add some API to allow us to fetch only the "on core" threads along the code path that handles internal stops.

In any case, in order to support having internal stops not fetch all threads - which we are currently doing with OS Plugins - we need to be able to persist the stateful data lldb holds for that thread across stops where we don't realize the Thread object. That is the sole ambition of this set of patches.

We've agreed that we would like to avoid gathering the thread data for internals stops that don't need it. Avoiding creating (updating) lldb_private::Thread objects, and having the not-(yet)-realized threads live as a tid_t is one way to achieve this goal, but I am not sure if it is the right one. I am not really against the idea -- since threads can come and go between stops pretty much arbitrarily (even without os plugins), it may make sense to have everything hold onto them as simple handles, and force a fetch through some Process api when one needs to access them may simplify some things (for one, the lifetime of Threads could become much stricter).

I can certainly believe that changing thread plans to use a tid_t instead of a Thread& is simpler than making Thread object management lazy, but I am not sure that is a fair comparison. Thread plans are only a single component which juggles Thread objects. There are many other pieces of code, which assume that the lifetime of a Thread object matches that of the entity it describes. SBThread is one such thing. If SBThread keeps referring to the actual thread as a pointer, then it will lose track of it when the actual thread goes away and reappears. Same goes for ValueObjects and SBValues -- if lldb_private::Thread is meant to be transient, then those should also be updated to use a tid_t(*), but then the changes required to do that become much larger, and it's not clear to me whether that is really worth it, since this is just kind of reinventing weak_ptr<Thread> (but with the downside mentioned in (*)).

Everything that holds onto its state using an ExecutionContextRef already holds onto the m_tid as the real entity, and the Thread is only a cache (see ExecutionContextRef::GetThreadSP). That probably hasn't been pushed hard, and I've only written a few tests with actually vanishing threads so far. But if it doesn't work properly then that's a bug to fix not a design to change.

ValueObjects (and therefore SBValues) use an ExecutionContextRef in their UpdatePoint, they don't hold onto a thread. SBThread's also hold ExecutionContextRef's not ThreadWP's to get back to their actual thread. In general, anything that wants to know "where am I in the state space of the process" is supposed to use an ExecutionContextRef to store its state not individual target/process/thread/etc. So I'm not sure this is so far off, but we would have to write some more tests with threads coming and going to make sure this is used consistently.

This sounds very convincing to me. I was under the impression that this is kind of inventing something new, but as there already is a notion of floating Thread objects, then I don't have a problem with that. I'd say the existing ExecutionContextRef mechanism is working fairly well, as I haven't yet noticed how it re-fetches threads.

Tue, Mar 10, 10:52 AM · Restricted Project

Mon, Mar 9

jingham updated the diff for D75880: [NFC} Move ThreadPlans stacks into their own class, store it in Process by TID.
Mon, Mar 9, 6:22 PM · Restricted Project
jingham added parent revisions for D75880: [NFC} Move ThreadPlans stacks into their own class, store it in Process by TID: D75711: [NFC] Have ThreadPlans hold onto the Process & TID, rather than the Thread, D75720: Make ThreadPlanTracer use {Process,TID} rather than Thread to find it's underlying thread....
Mon, Mar 9, 4:12 PM · Restricted Project
jingham created D75880: [NFC} Move ThreadPlans stacks into their own class, store it in Process by TID.
Mon, Mar 9, 4:12 PM · Restricted Project
jingham added a child revision for D75711: [NFC] Have ThreadPlans hold onto the Process & TID, rather than the Thread: D75880: [NFC} Move ThreadPlans stacks into their own class, store it in Process by TID.
Mon, Mar 9, 4:12 PM · Restricted Project
jingham added a child revision for D75720: Make ThreadPlanTracer use {Process,TID} rather than Thread to find it's underlying thread...: D75880: [NFC} Move ThreadPlans stacks into their own class, store it in Process by TID.
Mon, Mar 9, 4:12 PM · Restricted Project
jingham added a comment to D75711: [NFC] Have ThreadPlans hold onto the Process & TID, rather than the Thread.

Thanks for the detailed response. I'll try to be equally understandable.

I am somewhat worried about the direction this is taking. Maybe vanishing threads are fine for os-level debugging (because there the "real" threads are the cpu cores, and the os threads are somewhat imaginary), but I definitely wouldn't want to do it for regular debugging. If my process has 1000 threads (not completely uncommon) then I don't think my debugger would be doing me a service showing me just the three it thinks are interesting and completely disregarding the others. Having a mode (it could even be default) which only highlights the interesting ones -- yes, that would definitely be useful. Making sure it doesn't touch the other threads when it doesn't need to -- absolutely. But I don't think it should pretend the other threads don't exist at all, because I may still have a reason to inspect those threads (or just to know that they're there). In fact, I wouldn't be surprised if this happened to the said kernel folks too, and they end up adding a custom command, which would enumerate all "threads".

First off, I also think that hiding some threads from users, or forcing people to say "thread list --force" or something like that to see them all would not be a user-friendly design. I'm not suggesting imposing that on general debugging. But if an OS plugin, for instance, decides it is too expensive to do this except as an explicit gesture, we need to support that decision.

But lldb stops many times during the course of a step where it doesn't return control to the user - all the step into code with no debug info, step out again, step across dynamic linker stubs or step through ObjC message dispatch dances we do involve many private stops per public spot... If a thread has not stopped "for a reason" at one of those stops, it takes no part in the "should stop" and "will resume" negotiations and reconstructing it for that stop really does serve no purpose. There's no way that the user will get a chance to see it.

So if we could avoid reading them in, that might make stepping go faster in the presence of lots of threads. BTW, with the accelerated stop packets where we get all the threads and their stop reasons on stop, I'm not sure this would really save all that much time. But talking to less capable stubs, it really can make a difference. Greg did a bunch of work to make sure stepping stopped as few times as possible because the Facebook gdb-remote stub didn't support these accelerated packets and getting all the thread info on each private stop was making stepping go really slowly. I didn't understand the motivation at first because for debugserver the changes didn't really make any difference.

Also, I don't think these two desires: not fetching threads when we don't need them (e.g. private stops) and showing them all to the user whenever they ask, are contradictory. After all we still have the gate of UpdateThreadListIfNeeded. All the commands that touch the thread list go through this. So even if we don't gather all the threads when we stop, we already have a mechanism in place whereby any command that actually touches the thread list can fetch them on demand.

Yes, I agree with everything above. If lldb does not need information about all threads in order to service a internal stop, then it shouldn't read it in, and the os plugin (or the remote stub) should not need to provide it. In fact I would even go so far as to say that we shouldn't need to read in this information for a public stop until a user performs an operation (e.g. thread list) that requires information about all threads.

What was actually the slow part here? Was it the os plugin reading the thread lists from the kernel memory? Or the fact that lldb is slow when it is confronted with them?

If it is the latter, then maybe we should fix that. For example, I think I remember lldb likes to check the PC of every thread before it does any kind of a resume -- I am not convinced that is really needed.

If fetching the threads is slow, then perhaps we could have an api, where the os threads are produced incrementally, and we ensure that lldb does not query for os threads needlessly during the hot paths (e.g. internal stops for servicing thread plans).

The thing that is slow is gathering all the threads. On Darwin, there's a kernel activation per user space thread and then a whole bunch of kernel threads. On a busy system, that ends up being a whole lot of threads. Producing them through the OS plugin interface involves a bunch of memory reads to fetch the thread data, and following pointers around to get the register state for the threads, etc.

Note also, the way the OS Plugins currently work, when the process stops the OS Plugin just gets asked to produce the threads backing the "on core" threads. That's all UpdateThreadListIfNeeded will fetch, and there's no lldb way to get the OS Plugin to fetch all the threads it can produce. The only way to do that with the Darwin kernel is to run other commands in the set of Python commands provided by the kernel team that either fetches a non-current thread by ID or forces fetching all the threads.

So the current state when working with OS Plugins violates your principle given above,

IIUC, this principle is only violated for this particular os plugin, which violates the plugin contract and does not return all threads through the get_thread_info interface. I don't know whether this contract is spelled out anywhere, but it seems like at least the code in lldb behaves that way.

So this is technically not "our" fault. The plugin is doing something unsupported and gets weird behavior as a result. Now, I am not saying we should throw this plugin (maybe the only actually used os plugin) overboard. However, I was hoping that the process of supporting it would involve some improvements to the os plugin interface (so that it can do what it wants in a supported way), rather than declaring support for whatever it is the plugin is doing, even though we know it to be problematic (like, not being able to tell when we can throw thread plans away).

Mon, Mar 9, 10:46 AM · Restricted Project

Fri, Mar 6

jingham updated the diff for D75720: Make ThreadPlanTracer use {Process,TID} rather than Thread to find it's underlying thread....
Fri, Mar 6, 6:11 PM · Restricted Project
jingham updated the diff for D75711: [NFC] Have ThreadPlans hold onto the Process & TID, rather than the Thread.
Fri, Mar 6, 5:41 PM · Restricted Project
jingham added a comment to D75711: [NFC] Have ThreadPlans hold onto the Process & TID, rather than the Thread.

As long as the keeping thread plans around for threads that aren't there is only enabled when OS plug-in is around, I am fine with this direction. One questions: do we currently only allow stepping on real threads or OS threads that are backed by real threads right now? And if an OS thread is backed by a real thread, do we associate the step plan with the OS thread ID or the real thread ID (the core)? If we use the OS thread ID, another way to potentially fix this is to know in a thread plan that we are stepping an OS thread, and avoid fetching the complete OS list of threads for non public stops, but only fetch the thread list for any OS threads that have active thread plans and any OS threads for threads that are active on a core. Then we don't end up fetching all OS threads all the time for stepping, but it does allow us to maintain and track our OS thread correctly. This would require changes to the OS plug-ins where getting the thread list needs to be able to grab only the OS threads that are on core, and we can manually fetch the OS thread info for any thread plans, and the plug-in would still need to be able to fetch the complete list on public stop events.

Fri, Mar 6, 12:08 PM · Restricted Project
jingham added a comment to D75711: [NFC] Have ThreadPlans hold onto the Process & TID, rather than the Thread.

I am somewhat worried about the direction this is taking. Maybe vanishing threads are fine for os-level debugging (because there the "real" threads are the cpu cores, and the os threads are somewhat imaginary), but I definitely wouldn't want to do it for regular debugging. If my process has 1000 threads (not completely uncommon) then I don't think my debugger would be doing me a service showing me just the three it thinks are interesting and completely disregarding the others. Having a mode (it could even be default) which only highlights the interesting ones -- yes, that would definitely be useful. Making sure it doesn't touch the other threads when it doesn't need to -- absolutely. But I don't think it should pretend the other threads don't exist at all, because I may still have a reason to inspect those threads (or just to know that they're there). In fact, I wouldn't be surprised if this happened to the said kernel folks too, and they end up adding a custom command, which would enumerate all "threads".

Fri, Mar 6, 11:35 AM · Restricted Project

Thu, Mar 5

jingham created D75720: Make ThreadPlanTracer use {Process,TID} rather than Thread to find it's underlying thread....
Thu, Mar 5, 6:35 PM · Restricted Project
jingham updated the diff for D75711: [NFC] Have ThreadPlans hold onto the Process & TID, rather than the Thread.

Attempt to appease clang-format.

Thu, Mar 5, 6:03 PM · Restricted Project
jingham added a comment to D75711: [NFC] Have ThreadPlans hold onto the Process & TID, rather than the Thread.

Another point to make clear here. You might wonder whether ThreadPlan::GetThread should return an Optional<Thread> or something like that, since there might not be a Thread associated with the ThreadPlan.

Thu, Mar 5, 4:26 PM · Restricted Project
jingham created D75711: [NFC] Have ThreadPlans hold onto the Process & TID, rather than the Thread.
Thu, Mar 5, 3:53 PM · Restricted Project

Wed, Mar 4

jingham accepted D74903: [lldb][testsuite] Create a SBDebugger instance for each test.

Seems great to me. Getting the host platform is a generally useful thing to be able to do, and I can't think of anything you would want to add to this, so that is fine to.

Wed, Mar 4, 10:38 AM · Restricted Project
jingham accepted D75597: Update the current execution context at the beginning of tab completions.

LGTM, thanks for fixing this.

Wed, Mar 4, 10:38 AM · Restricted Project

Tue, Mar 3

jingham added a comment to D75418: tab completion for process signal.

I think this is just a bug in argument completion.

Tue, Mar 3, 10:56 AM · Restricted Project
jingham accepted D74557: [lldb] Make BreakpointResolver hold weak_ptr instead of raw pointer to breakpoint.

You mean for BreakpointResolverScripted::CreateImplementationIfNeeded? Passing in the BreakpointSP is fine.

Tue, Mar 3, 10:01 AM · Restricted Project

Feb 25 2020

jingham committed rG3cd13c4624b5: Fix a race between lldb's packet timeout and the profile thread's usleep. (authored by jingham).
Fix a race between lldb's packet timeout and the profile thread's usleep.
Feb 25 2020, 11:17 AM
jingham closed D75004: Fix a race between lldb's packet timeout and killing the profile thread.
Feb 25 2020, 11:17 AM · Restricted Project

Feb 21 2020

jingham created D75004: Fix a race between lldb's packet timeout and killing the profile thread.
Feb 21 2020, 5:06 PM · Restricted Project

Feb 13 2020

jingham committed rG4570f2c7cf35: Add a test for debugserver handling threads suspended from within a program. (authored by jingham).
Add a test for debugserver handling threads suspended from within a program.
Feb 13 2020, 4:04 PM
jingham accepted D74558: [lldb] Make shared_from_this-related code safer .

Targets and Breakpoints should only ever be managed by SP in lldb, so enforcing that seems like a good cleanup.

Feb 13 2020, 10:33 AM · Restricted Project
jingham added a comment to D74557: [lldb] Make BreakpointResolver hold weak_ptr instead of raw pointer to breakpoint.

I wonder if it wouldn't be better to assert in GetBreakpoint. Except when you are making the resolver, you should never have a breakpoint resolver without a valid breakpoint. And there's no point in calling GetBreakpoint when you know you haven't set it yet. You assert after most of the calls to GetBreakpoint, but not all. Of the ones you don't, I think most of them should be.

Feb 13 2020, 10:33 AM · Restricted Project
jingham accepted D74556: [lldb] Don't call CopyForBreakpoint from a Breakpoint's constructor.

LGTM

Feb 13 2020, 10:15 AM · Restricted Project
jingham added a comment to D74478: [lldb] Let TypeSystemClang::GetDisplayTypeName remove anonymous and inline namespaces..

The only hesitation I have about this is if we are still printing this noise in demangled names, then the name of the type you see for a variable will be different from what you see when a method of that type ends up in a backtrace. That might be confusing. OTOH, the correct solution to that problem, IMO, is to remove the noise from backtraces, not keep it in the types...

Feb 13 2020, 10:06 AM · Restricted Project

Feb 12 2020

jingham added a comment to D74136: [LLDB] WIP: Follow DW_AT_decl_file when setting breakpoint.
In D74136#1869622, @kwk wrote:

@labath @jingham to summarize from what I read here and what I chatted about with @labath , the following is a possible way to go for now, right?

  1. We're not going to introduce my flag.
  2. You're both not perfectly happy with the way things are documented at the moment and dislike some of the implementation as in in LLDB but chaning it should not be part of this patch.
  3. @jingham wouldn't want to introduce --compile-unit as a flag that @labath proposed.
  4. You want breakpoint set --file to search everything, that is to say compilation units and files referenced in DW_AT_decl_file.

    If you can confirm that this is correct, then I can refactor this patch to remove the switch and change the default behavior for breakpoint set --file. Especially point 4. is important I guess.

There's a point (5) which is that the search in 4 should be conditioned on the setting of the "target.inline-breakpoint-strategy". That way if people have big projects but don't ever #include source files, and don't build with LTO, they can turn off these extra searches, which might end up being costly and to no purpose for them.

I think we're on the same page, but I'll just try to rephrase this in my own words.

Basically, I'm hoping that we could get the --file+--function combo to behave the same way --file+--line. I.e., that the --file argument should specify the file that the function is defined in (i.e. the DW_AT_decl_file attribute of the function), and not the compile unit (DW_AT_name of the compile unit containing the function). The way the inline-breakpoint-strategy setting comes into play is that if the setting value is "headers" and the user specifies a .cpp file, then we will assume that the .cpp file also matches the name of one of the compile units, and we will not attempt to search compile units with different names (which is the same thing we do for line breakpoints, I believe).

Feb 12 2020, 10:42 AM · Restricted Project
jingham added a comment to D74136: [LLDB] WIP: Follow DW_AT_decl_file when setting breakpoint.

This way the only bit of the matrix of "restricting function name breakpoints by containing source file" that we don't support is "set a breakpoint on this function, but ONLY when it is inlined into another CU, NOT when it is in its defining CU." That seems the least useful of the options, and I'm happy to trade off not adding another option to the already overstuffed "break set" list of options. And remember, if you actually needed this it would be straightforward to write a scripted breakpoint resolver to do that job.

Feb 12 2020, 10:42 AM · Restricted Project

Feb 11 2020

jingham added a comment to D74136: [LLDB] WIP: Follow DW_AT_decl_file when setting breakpoint.
In D74136#1869622, @kwk wrote:

@labath @jingham to summarize from what I read here and what I chatted about with @labath , the following is a possible way to go for now, right?

  1. We're not going to introduce my flag.
  2. You're both not perfectly happy with the way things are documented at the moment and dislike some of the implementation as in in LLDB but chaning it should not be part of this patch.
  3. @jingham wouldn't want to introduce --compile-unit as a flag that @labath proposed.
  4. You want breakpoint set --file to search everything, that is to say compilation units and files referenced in DW_AT_decl_file.

    If you can confirm that this is correct, then I can refactor this patch to remove the switch and change the default behavior for breakpoint set --file. Especially point 4. is important I guess.
Feb 11 2020, 9:45 AM · Restricted Project

Feb 10 2020

jingham added a comment to D73921: Assert that a subprogram should have a name when parsing DWARF.

DWARFASTParserClang looks to me the wrong layer to fix this. Why can't this be caught in the generic DWARF Parser?
I also believe that it's better if dwarfdump -verify crashes on this, rather than lldb.

Feb 10 2020, 3:28 PM

Feb 7 2020

jingham committed rGd62a343db23b: Add a test for adding and removing Listeners from a BroadcasterManager. (authored by jingham).
Add a test for adding and removing Listeners from a BroadcasterManager.
Feb 7 2020, 6:02 PM
jingham added a comment to D74255: [LLDB] Add support for AVR breakpoints.

Might also be good to fix the crash. If you don't support software breakpoints, you shouldn't get asked what your breakpoint opcode is.

Feb 7 2020, 3:02 PM · Restricted Project
jingham added inline comments to D74157: [lldb/API] NFC: Reformat SBThread::GetStopDescription().
Feb 7 2020, 11:12 AM · Restricted Project
jingham added a comment to D74157: [lldb/API] NFC: Reformat SBThread::GetStopDescription().

Looks better. TBH, I'm not sure why/if we really need the case handling the situation where the thread does not have a stop description. Ideally I'd just move this code there (or delete it).

A thread that stops for no reason will have an empty StopInfoSP. So somebody has to check for that. I also don't want a thread that hasn't stopped for a reason to have a placeholder description like "no reason" because if you are using the API's you'd like to just print whatever the description is, and if you stop one thread at a breakpoint, having all the others say "no reason" is noisy and unhelpful.

I don't understand this comment. The fallback code we are talking about only ever triggers if there's a StopInfo. If the StopInfoSP was empty we wouldn't return a description.

Feb 7 2020, 10:45 AM · Restricted Project

Feb 6 2020

jingham added a comment to D74157: [lldb/API] NFC: Reformat SBThread::GetStopDescription().

Looks better. TBH, I'm not sure why/if we really need the case handling the situation where the thread does not have a stop description. Ideally I'd just move this code there (or delete it).

Feb 6 2020, 6:38 PM · Restricted Project
jingham added a comment to D74153: [lldb] Delete the SharingPtr class.

Actually, it looks like we were getting away with that because the whole ValueObjectRegisterContext (*not* RegisterSet) class is unused. In fact the whole concept of having the entire register context as a "value" seems fairly odd to me. Can we just delete it?

Feb 6 2020, 5:02 PM · Restricted Project
jingham added a comment to D74153: [lldb] Delete the SharingPtr class.

We were probably getting away with this because ValueObjectRegisterSet's children don't really need their parent to construct their values? But still, this is not how ValueObject children should work...

Feb 6 2020, 12:52 PM · Restricted Project
jingham added a comment to D74153: [lldb] Delete the SharingPtr class.

I think the ValueObjectRegisterSet::CreateChildAtIndex was wrong originally. It doesn't make it's children by passing itself in as the parent of the child, but just makes an independent ValueObject. You can fix that in your version by passing the ValueObjectRegisterSet's manager.

Feb 6 2020, 12:43 PM · Restricted Project
jingham added a comment to D74136: [LLDB] WIP: Follow DW_AT_decl_file when setting breakpoint.

Is there ever a reason other than performance why you would want NOT to consult both the Compile Unit name and also look for DW_AT_decl_file? That doesn't seem clear to me.

If the only worry is performance, and except for that you would really always want the full search, then I don't see what we gain by adding --compile-unit as distinct from --file. We should use target.inline-breakpoint-strategy to allow people to turn on or off the searching for DW_AT_decl_file for folks whose projects are complicated enough AND who aren't doing a lot of inlining. "break set" is already over-stuffed, I don't want to add to it unless we get a positive benefit.

Well, conceptually these are two different things. --compile-unit would restrict the search to the given compile unit no matter what is the file where the function was defined in. And --file would consider the defining file, and not care which compile unit hosts that. In theory, both can be useful, but I'm not sure if people would really use that flexibility.

My proposal was based on the fact that we want to preserve the ability to filter according to the defining cu. If we don't need to do that, then simply just redefining the meaning of the file+function combo is fine by me..

Feb 6 2020, 11:45 AM · Restricted Project
jingham added a comment to D74126: [lldb] Remove all 'clean' targets from test Makefiles.

The one use I had for these clean targets was when you are making a new test and getting the test source to compile it is really tempting to run make in the test directory. It's a lot slower to go run the test, see the compile fail, fix something, etc... than just run make. But if you do that and don't clean, then when you actually go to run the test, it will fail because make sees the product in the source directory and won't build it in the out-of-line directory, but the test will look for it there.

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

Is there ever a reason other than performance why you would want NOT to consult both the Compile Unit name and also look for DW_AT_decl_file is performance? That doesn't seem clear to me.

Feb 6 2020, 11:10 AM · Restricted Project

Feb 5 2020

jingham accepted D72513: Don't fail step out if remote server doesn't implement qMemoryRegionInfo.

Oops, sorry. Looks fine.

Feb 5 2020, 2:07 PM · Restricted Project

Jan 31 2020

jingham added a comment to D73766: [RFC] Make substrs argument to self.expect ordered by default..

The current behavior does make tests less dependent on the exact details of lldb's command output. If there were two independent pieces of data in a command output that you wanted to compare against, you would have to fix some tests if you ever changed the ordering in the result. It was the case that the gdb test suite hampered the evolution of the command output because it was just too much of a pain to go change the tests. For instance, you really couldn't muck with breakpoint reporting output or stop notifications; the pain of fixing up the testsuite was way too high. But in this case, the fix would just be reordering the substrings in the Python array. So though this sort of thing makes me a little uneasy, in this case I think it's fine.

Jan 31 2020, 10:54 AM · Restricted Project

Jan 28 2020

jingham requested changes to D72513: Don't fail step out if remote server doesn't implement qMemoryRegionInfo.

I don't think you want to put anything in m_constructor_errors in this case either. It's fine to log something out, but ValidatePlan uses m_constructor_errors to help report a problem. If the breakpoint failed to take for some other reason, then we would report a permissions problem which is not the reason for the failure.

Jan 28 2020, 11:46 AM · Restricted Project

Jan 24 2020

jingham added inline comments to D73303: [lldb/Target] Add Assert StackFrame Recognizer.
Jan 24 2020, 5:30 PM · Restricted Project
jingham added a comment to D69273: ValueObject: Fix a crash related to children address type computation.

We've been off all the past week. I'll circle back with Jim about this once I get to the office.

Jan 24 2020, 5:21 PM · Restricted Project
jingham added inline comments to D73303: [lldb/Target] Add Assert StackFrame Recognizer.
Jan 24 2020, 9:46 AM · Restricted Project

Jan 23 2020

jingham committed rG29c7e6c8c97f: 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…
Jan 23 2020, 12:47 PM
jingham closed D73225: Handle the new objc direct dispatch accelerator functions for uncommonly overridden methods.
Jan 23 2020, 12:46 PM · Restricted Project
jingham added a comment to D73225: Handle the new objc direct dispatch accelerator functions for uncommonly overridden methods.

Addressed remaining review comments.

Jan 23 2020, 12:46 PM · Restricted Project
jingham updated the diff for D73225: Handle the new objc direct dispatch accelerator functions for uncommonly overridden methods.

Addresses the rest of Jonas' comments.

Jan 23 2020, 12:46 PM · Restricted Project
jingham accepted D73284: [lldb/Commands] Fix, rename and document column number argument to breakpoint set..

LGTM. This is my bad. I put in the parsing for -C in CommandObjectBreakpointSet in an (apparently futile) attempt to remind myself to reserve -C for column number...

Jan 23 2020, 11:24 AM · Restricted Project

Jan 22 2020

jingham added a comment to D73225: Handle the new objc direct dispatch accelerator functions for uncommonly overridden methods.

Thanks for the suggestions! A couple were not to my taste, but I fixed all the others.

Jan 22 2020, 3:43 PM · Restricted Project
jingham updated the diff for D73225: Handle the new objc direct dispatch accelerator functions for uncommonly overridden methods.

Addressed Jonas' review comments.

Jan 22 2020, 3:40 PM · Restricted Project
jingham created D73225: Handle the new objc direct dispatch accelerator functions for uncommonly overridden methods.
Jan 22 2020, 1:37 PM · Restricted Project
jingham committed rG89c8866c0417: Convert AssertTrue( A == B) to AssertEqual(A, B) in TestObjCStepping.py. (authored by jingham).
Convert AssertTrue( A == B) to AssertEqual(A, B) in TestObjCStepping.py.
Jan 22 2020, 1:28 PM

Jan 21 2020

jingham accepted D73053: [lldb/DataFormatters] Fix the `$$deference$$` synthetic child.

Except for one typo in the comment (thanks for adding that BTW) looks good.

Jan 21 2020, 10:19 AM · Restricted Project

Jan 16 2020

jingham added a comment to D72898: [lldb] add to gdb to lldb doc.

There's a copy paste error in the first header. Other than that, this is fine, thanks for adding the entries!

Jan 16 2020, 7:22 PM · Restricted Project

Jan 15 2020

jingham added a comment to D71575: [LLDB] Add ObjectFileWasm plugin for WebAssembly debugging.

BTW, I had to fix this patch (cd9e5c32302cd3b34b796683eedb072c6a1cfdc1) to build on macOS. uint64_t and size_t are differently spelled (though I think otherwise equivalent.) One is "long long unsigned int", the other "long unsigned int". I have no idea why that's true, but std::min refuses to compare a size_t and a unit64_t. Anyway, I fixed this by casting one of the two sides of the comparison. But this was causing problems because we have an api (ReadImageData) that takes a uint64_t for the offset and a size_t for the size. That seems a little weird to me, why are these different types?

Jan 15 2020, 6:24 PM · Restricted Project