Use sleep_for instead of usleep. Man, that API isn't winning any beauty contests!

Thanks for doing this. The test suite was clean after my original changes, which means whatever is using CommandObjectMultipleThreads must not have had any tests where arguments were passed to it? Ditto for the trace command. Might be good to add those as well.

In D50304#3592849, @RamNalamothu wrote:

In D50304#3586710, @jingham wrote:

For some reason I'm not getting mail notifications for review changes, sorry about that.

This is certainly better than the original implementation. Among other things, if we find an exact match, we really shouldn't be doing any more inexact matches, so after the first exact hit it should have switched exact to true.

But if you had line tables laid out like (this is in increasing order of address:

10
20
30
10
16

and you did "thread until 15", this would find the inexact match at 20, switch to an exact match for line 20 and find no other matches. But the gap between 10 & 16 in the line table is maybe an even more plausible place to put the line 15 until breakpoint, so maybe we did want to throw a breakpoint there as well?

@jingham
Nope, with the current patch, we would find the inexact match at 16.

Why is it desirable to have the debugger not see signals sent to the process?

This categorization seems like it is relevant for any instruction, not just instructions found in a trace buffer. Is there any reason why this isn't just an annotation added by the standard instruction printer, that you then turn on with the -k flag to thread trace dump (but which might also be useful for the regular disassemble command)?

Address review comments

In D127605#3580123, @kastiglione wrote:

This is good, but it also illustrates how the strings "->" and ".'" should actually come from the typesystem and not be hardcoded. We're just lucky that all languages have a "." operator.

More elegant would be to just add an API to TypeSystem to get the operator to access ivars.

Initially I was thinking along these lines. But I became less sure. The "variable expression path" syntax is C-like but independent of the source language. For example, in Swift, there is no ->. So my rhetorical question is, should the Language or TypeSystem, etc, know about "variable expression path" syntax, as you suggest? Or should we rely on there being a shared subset between source language and variable expression path syntax, and allow the two subsystems to think they're speaking the same language even though we know they're each actually speaking a different but partially compatible language (source vs variable expression path).

I decided to keep the ->/. choice inside GetValueForVariableExpressionPath because I feel that kind of logic belongs closer to other variable expression path code.

For some reason I'm not getting mail notifications for review changes, sorry about that.

ping

Sorry, I missed the "ping" notification. Still LGTM.

This looks good to me as well. The job of "recreating the object from an eval of the repr output" is not even possible for many SB objects, by the time the object was made it's lost the factory invocation that produced it. And anyway, I also can't see a good use case for this. The fact that this gives us good output in lists of objects is also pretty persuasive.

Excellent, thanks for doing this, these are really handy asserts!

Fixed the option definition to specify this is a bkpt_id_list not a breakpoint id (since that's what it is now that we're using the breakpoint id list parser), and removed the verbiage that that made redundant.

One grammatical correction and a couple of questions. But I'm also fine with the way it is.

I have to fix the doc string for the -b option since it's now a bkpt_id_list. But that won't affect the logic so I'll do that and update the patch tomorrow.

Added the ability to specify breakpoint locations to run to as well as breakpoints and breakpoint names.

In D126513#3550588, @kastiglione wrote:

I noticed that the example python command and this feature don't support specifying breakpoint locations. Should it?

Allow for specifying breakpoint locations as well as breakpoints.
Also fix the sync setting & resetting.

Mostly reworking the paragraph on breakpoint hiding.

Address review comments.

More generally, you can tell which elements of the exe_ctx you need to check in any given command by looking at requirements for the command which are passed into the constructor for the command. Any entity that's required there, you don't need to check for, but otherwise, you do have to check. "CommandObjectMemoryRead" has:

Fix a comment that hadn't been correct for a while...

In D126789#3550572, @kastiglione wrote:

thank you for fixing!

Q: why does this test have this thin test_X/do_x_test separation?

Note, the original code tries to finesse this by passing eLazyBoolCalculate, which then resolves to a check against the current command depth. But that check was wrong for SBDebugger.HandleCommand, which is considered at depth 0, and anyway, I don't think it ever makes sense to have the regex command add the resolved command to the history.

Just for history's sake, this change was committed (738af7a6241c9). I put in the Differential Revision line in the changelog, so I'm not sure why this didn't get marked as closed...

Address Jonas' review comments

The patch gets a little hard to read with all the no-longer in the right places comments, but I think I addressed everything.

Added support for breakpoint names as well as ID's.
Addressed other review comments.

In D126259#3535004, @clayborg wrote:

In D126259#3534997, @jingham wrote:

Don't allow setting signal actions by signal number before you have a process.

I understand

We don't know what signal 20 is going to end up being till we have a process, so allowing this by number doesn't make sense.

I am saying that it would be a good idea to make sure an error is returned when you do try and set a signal by number before a process exists and make sure there is a test that covers this if it isn't already in the current patch.

In D126259#3534944, @clayborg wrote:

In D126259#3534919, @jingham wrote:

In D126259#3534729, @clayborg wrote:

So the "process handle" command allows us to set signals by signal number as well. Does this patch support this? It seems like it wouldn't be too hard to do if we wanted to handle this. Lemme know what you think, other than that LGTM.

Ah, I forgot about specifying the signal by number. Before you have a process I don't think we should allow signals by number. The mapping signal number -> "signal name for any platform" is not 1-1 so we couldn't guarantee we were doing the right thing here. I'll put in a check for "specified by number with no process" and error out.

I would add a test for this and make sure it fails gracefully in that case

Don't allow setting signal actions by signal number before you have a process.

In D126259#3534729, @clayborg wrote:

So the "process handle" command allows us to set signals by signal number as well. Does this patch support this? It seems like it wouldn't be too hard to do if we wanted to handle this. Lemme know what you think, other than that LGTM.

This patch treats the Signal structure held in the target differently from UnixSignals::Signal. The latter always has to have values for all three actions and needs to have a signal number. The former doesn't actually claim to know a signal number. After all, it might get set before we can know what signal number a given signal string has (signals with the same name do have different numbers on different platforms). So pretending it knows a signal number would just be confusing. Also, it doesn't record actual values, it records user intents, so it needs to be tri-state - thus the LazyBools.

GetPropertyAtIndexAsArg isn't actually documented, so we don't know what the original intent for the return value was. But in almost all the cases where we look up a property using this function, when we use the enum directly we discard the result. In the only other case (GetUserSpecifiedTrapHandlerNames) were we do return the result of the GetProperty... function, the return result of the GetUser... function is always discarded. So it seems pretty clear that the result was just supposed to be "couldn't find the property". It's also weird to conflate "couldn't find property" with "property has an empty value".

Address review comments

Addressed review comments.

I don't think the cache belongs in a global in the TildeExpressionResolver class. The derived ExpressionResolvers might very well resolve the same ~foo to different paths (e.g. if there were a RemoteTildeExpressionResolver.) So the cache needs to be kept in the derived class rather than in a global object. Maybe just add an abstract GetTildeCache method to TildeExpressionResolver to fetch the cache from the subclass?

LGTM

This needs a doctoring for the API and some test.

In D125325#3505305, @PatriosTheGreat wrote:

Hi Greg.

Thanks for the review.
I fixed the feedback.
I forgot the default initialization of plugin_name parameter in SBProcess.i in previous version, but I assume it's still better to explicitly create a separate method.

This seems like a reasonable fallback, and the implementation looks fine.
You need to add a test case setting an address breakpoint in the executable and making sure that works. Should be easy, set a line breakpoint in the main executable, get the resolved address and set an address breakpoint with that and make sure that breakpoint got a location.
You should also add some words to the "break set" command help saying that this is the fallback behavior.

Address review comments.

This was submitted (dd8490d207d3a1612091abbea04bf660f133a89f) but I neglected to put the "Differential Revision" tag in the commit message so automation didn't pick that up...

The change seems correct. Since the test only requires -01, and the test code is quite straightforward, the test seems fine.

Sorry for the delay here.

Thanks for these cleanups. In actual fact, the values set for options in the constructor for CommandOptions are never used, only the values in OptionParsingStarted matter. But it's still confusing to have them be wrong...