User Details
- User Since
- Sep 23 2014, 5:24 PM (405 w, 2 d)
Yesterday
Wed, Jun 29
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.
Tue, Jun 28
Mon, Jun 27
Fri, Jun 24
Why is it desirable to have the debugger not see signals sent to the process?
Thu, Jun 23
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)?
Wed, Jun 22
Tue, Jun 21
Address review comments
Thu, Jun 16
Wed, Jun 15
For some reason I'm not getting mail notifications for review changes, sorry about that.
ping
Tue, Jun 14
Sorry, I missed the "ping" notification. Still LGTM.
Fri, Jun 10
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.
Thu, Jun 9
Excellent, thanks for doing this, these are really handy asserts!
Wed, Jun 8
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.
Tue, Jun 7
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.
Allow for specifying breakpoint locations as well as breakpoints.
Also fix the sync setting & resetting.
Fri, Jun 3
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...
Wed, Jun 1
Jun 1 2022
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.
May 31 2022
May 26 2022
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...
May 25 2022
Address Jonas' review comments
May 24 2022
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.
Don't allow setting signal actions by signal number before you have a process.
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.
May 23 2022
May 20 2022
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".
May 18 2022
Address review comments
Addressed review comments.
May 17 2022
May 12 2022
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
May 11 2022
This needs a doctoring for the API and some test.
May 10 2022
May 6 2022
May 5 2022
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.
May 4 2022
Apr 29 2022
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...