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.

Addressed most of Pavel's review comments.

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.

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

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.

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

In D76470#1934146, @friss wrote:

In D76470#1933910, @jingham wrote:

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?

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

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.

Sorry, forgot to select the action...

This looks fine to me.

In D76188#1924083, @labath wrote:

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.

In D76105#1921273, @labath wrote:

In D76105#1920753, @friss wrote:

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?

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.

Thanks for doing this, it will be useful!

In D75711#1920642, @clayborg wrote:

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

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

Is this one okay as well?

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

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

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:

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

In D76009#1919554, @friss wrote:

In D76009#1919103, @labath wrote:

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.

Seems fine to me.

more reformatting

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

Addressed Pavel's review comments.

Addressed Pavel's review comments.

In D75711#1914001, @labath wrote:

In D75711#1912902, @jingham wrote:

In D75711#1912230, @labath wrote:

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.

In D75711#1912230, @labath wrote:

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

In D75711#1910155, @jingham wrote:

In D75711#1909055, @labath wrote:

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

In D75711#1910156, @clayborg wrote:

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.

In D75711#1909055, @labath wrote:

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

Attempt to appease clang-format.

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.

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.

LGTM, thanks for fixing this.

I think this is just a bug in argument completion.

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

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

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.

LGTM

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

In D74136#1871751, @labath wrote:

In D74136#1870029, @jingham wrote:

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

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.

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.

In D73921#1855961, @davide wrote:

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.

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.

In D74157#1863126, @friss wrote:

In D74157#1863069, @jingham wrote:

In D74157#1862537, @labath wrote:

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.

In D74157#1862537, @labath wrote:

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

In D74153#1862554, @labath wrote:

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?

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

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.

In D74136#1862246, @labath wrote:

In D74136#1862200, @jingham wrote:

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

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.

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.

Oops, sorry. Looks fine.

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.

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.

In D69273#1765235, @davide wrote:

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

Addressed remaining review comments.

Addresses the rest of Jonas' comments.

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

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

Addressed Jonas' review comments.

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

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

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?