- User Since
- Sep 23 2014, 5:24 PM (351 w, 6 d)
Are the Symbol ID's for unnamed symbols the same each time you read in a symbol file? While the unnamed_symbol symbol names are not significant, it would be good if you were crashing in __lldb_unnamed_symbol111 on one lldb run, you would also crash in the same named symbol when you ran again.
It's quite common to use the lldbtest.TestBase.runCmd or lldbtest.TestBase.expect to test command line commands in the API tests. The latter has expect-like results checking which makes writing these checks easy. Even if you are testing a command-line command, driving to the point where you want to do the check is often easier with the API & the lldbutil helpers, so it's okay to intersperse these among the API uses.
Tue, Jun 15
Fri, Jun 11
Thu, Jun 10
This looks pretty good to me.
Tue, Jun 8
I don't think it's right to do it in GetStackFrameAtIndex. For instance, suppose lldb stops somewhere, and the user does a backtrace, then they select a different frame on the stack, and then invoke some other command that happens to ask for the frame at index 0. If that subsequent GetStackFrameAtIndex(0) request triggers the recognizer to select a different frame, that would be surprising and not desirable.
"Unwinding" the zero-th frame is really just getting the PC & FP. Since you are very likely to need this information on any given stop, lldb has accelerated versions of the gdb-remote stop reply packets that provide this information up front so this sort of request is cheap to fulfill. I know that debugserver & lldbserver both provide these accelerated stop-reply packet. If you are using either of these debug stubs, this shouldn't be a performance problem. Are you using a different stub, and if so and you can switch to using the lldb-server stub - or fixing your stub to provide the accelerated stop reply packets, your lldb sessions will be a lot more efficient...
Fri, Jun 4
HandleCommands really does need to do this suppression, and this is a fine way to do it. I can't think of any other place where you would need to do this suppression after an admittedly non-exhaustive search. So LGTM.
This seems like a good change to me. And even if you wanted to do something cheesy like put an error into the result on spec, then later decide it was successful after all, you can always reset the result's status to the appropriate success status. This just makes the common practice less error prone. We should also remove all the redundant settings, leaving them in would be confusing. So I'd just make the change then fix whatever the bots bring up.
Tue, Jun 1
One of the "frame recognizers" - probably the one that's causing this particular problem - is the "assert" recognizer. The idea is that when you call "assert" in user code, you pretty much always end up with a 4 or 5 frames deep stack that wanders it's way from assert to __pthread_kill. But you pretty much never care about any of those stack frames. The stack frame you really want to see is the caller of the assert. So the assert recognizer detects a thread in that situation and resets the selected frame to the assert caller.
Thanks for taking this on. The in-class initialization is so much less boiler-plate and easier to read!
Fri, May 28
A command that knew it was streaming a lot of output is supposed to be able to choose to have the CommandInterpreter directly stream the results while the command is executing. That's good for something that is likely to print a lot of output, since then you don't have to wait for the command to be done to start seeing results. Of course, you still have to gather the command into the Return object as well since that's part of the CommandInterpreter contract. So you have to be careful not to print it twice. Maybe this code is part of the implementation of that?
Thu, May 27
It is incorrect to say that SelectMostRelevantFrame is only relevant for asserts. The FrameRecognizer is a generic mechanism, and you have no a priori way to know what kinds of thread stops it wants to operate on. So this patch is not correct.
Wed, May 26
The idea seems good, but I'm not sure how you are going to extend it? "Selector" seems a pretty ObjC specific term to me. Will this be what you use to get the "method name" part of the C++ names? That would look a bit odd to me, but OTOH having a GetSelector in all the subclasses, most of which don't have selectors also seems a little odd. This might be solved by just thinking of a better name, however. Also note, in the C++ case, it would be useful to distinguish between the method name, like Bar in Foo::Bar, and the fully qualified method name (like Bar(int, double) in Foo::Bar(int, double). That might effect the choice of names. The selector is the equivalent of a C++ fully qualified method name.
May 17 2021
May 15 2021
It's less error-prone if we just reset the timeout to the default when we wake up from ReadPacket. This isn't an expensive operation...
May 11 2021
May 10 2021
Was that what you had in mind, Greg?
When the ReadPacket call in SendContinuePacketAndWaitForResponse wakes up due to the wakeup timeout, and there's an interrupt which hasn't reached it's endpoint, shorten the wakeup interval so we don't wait longer than the interrupt endpoint.
May 7 2021
BTW, I generally agree that passing values in parameters that are always going to be used internally anyway is ugly and it's better to store them in an ivar. But in this case, the value was going to get used in some cases and not in others, which storing it as a parameter would have hidden. That's what I was trying to avoid.
I coalesced the timeout & no-timeout versions of GDBRemoteClientBase::SendPacketAndWaitForResponse and GDBRemoteClientBase::Lock::Lock as you suggested. I didn't use an Optional. That seems overkill when there's a perfectly good sentinel value for "don't interrupt": timeout of 0 seconds. So I just made the timeout in these two cases have a default value of 0 seconds, and used that to mean no interrupt. That way passing no timeout means "don't interrupt" and passing a non-zero one means interrupt.
I don't see the change as "adding a bunch of timeouts", since what it mostly did was remove a bunch of send_async = false's. For all the clients that don't care about a timeout, I removed the send_async parameter, and for those that did I replaced it with a timeout. There were a few cases at the GDBRemoteClientBase layer where routines were calling the SendPacket routines with send_async = true w/o requesting that info from ProcessGDBRemote. To those I added a timeout parameter. That I consider a plus since it made explicit which packet request functions were interrupting and which were not.
May 6 2021
I moved the eStateExited test as Greg suggested on commit.
May 5 2021
May 3 2021
Apr 30 2021
I would resist this change. It's unnecessarily disruptive, would again break git archeology, and really have no significant benefit. I also think the lldb conventions for naming things are much clearer than the llvm ones. Knowing that something is a ivar by looking at the name is a real timesaver, especially for people new to the code. Being able to tell local variables from other entities by looking also makes reading code much easier. Etc...
Apr 29 2021
GetPointerValue strips sign bits, so formally it should be used whenever you are getting a pointer that might be signed. OTOH, GetPointerValue is pretty new and since plain data pointers aren't going to be signed, there was no reason to go convert all the uses. We probably should use GetPointerValue. Also, using GetValueAsUnsigned with a 0 fallback value is not a great idea, though it seems to have been pretty common practice. That means if we are unable to read the pointer value for some reason, we'll say it is NULL. I think it would be better to use LLDB_INVALID_ADDRESS in these cases.
Apr 28 2021
Apr 27 2021
Apr 22 2021
Apr 21 2021
It seems like most of the places where you use the SourceLocationSpec, you also pass "check_inlines" and "exact". Those seem to me natural parts of a source location search specification class, and including them in the Spec would clean up the API's even further. Not necessary if it gets ugly for some reason, but it would look nicer.
Apr 16 2021
Since you just mainly want to run this past the bots again, probably best to just resubmit. You could update the patch here or not at your convenience, but I don't see it would serve any purpose to wait on approval.
Apr 15 2021
A bunch of little comments. I was also curious why you directly set the running private state, but use the async thread to generate the stopped events?
Apr 13 2021
Sorry for the delay!
Apr 6 2021
I don't understand how DiscardPlansConsultingMasterPlan in this patch works.
Apr 5 2021
This seems okay to me. Putting it in the process is right. We do "disable X" in a bunch of other places when enabled is the default, so that seems right. And I don't think there's much benefit to trying to squeeze the description into fewer letters, since you're either cutting & pasting into a script or using completion anyway.
Apr 1 2021
Mar 31 2021
Mar 25 2021
Raphael is right. The default values of the target.process.thread.step-avoid-regexp are set such that if lldb finds itself in a function inlined from std:: it will automatically step back out. This test is testing that (among other things). The fact that the test is failing means that something in how your compiler is generating symbols or debug info is defeating this. Maybe it's emitting the std:: symbols in such a way that the regexp isn't matching them? Maybe the inline records in the DWARF are confusing lldb somehow?
Mar 24 2021
Mar 23 2021
This looks good to me.
Mar 22 2021
This seems like the right way to go. I made a couple trivial comment comments.
Mar 19 2021
I added a couple more tests for having a breakpoint at the load site before committing.
The gdb model - since gdb only supports one debugee per gdb - is to either follow the fork or follow the parent. It would be more in keeping with lldb's model to make a new target for the child side of the fork, and use that to follow the child. That way you can continue to debug both the parent and the child processes. It doesn't look like you've gotten that far yet, but IMO that's the direction we should be going for lldb.
Mar 18 2021
Thanks for doing this! The event version looks pretty clean to me. I think we should go that way. I don't think we should have two ways, that seems confusing and still leaves us calling unknown user code in the middle of symbol updates...
Mar 16 2021
Mar 8 2021
I bet gdb uses the debugger's CWD because that's the practice working ON gdb (at least back in the day when I worked on it). You built gdb, cd'ed into the gdb build directory and ran the debugger on it from there. gdb's build also produced a debugging-gdb specific .gdbinit file in the build directory that would also get read in if you did it that way. When using command-line debuggers, running from the build directory is probably a fairly common practice. So there are merits to the cwd way of doing things. These merits fade when using IDE's which generally support debugging more than one thing at a time, so changing the working directory of the IDE makes little sense.
Mar 5 2021
This way of doing progress is going to look odd in anything that uses multiple debuggers. If I'm reading the code aright, if I have two debuggers, and a target in Debugger A starts doing something that would cause progress traffic, both debuggers will show activity. Given that the candidate for progress that is most common is down in SymbolFile, it's going to be hard to fix that. You'd have to have some way of marking the API boundary from "where you have a Target" to "where you don't have a Target" and record (maybe with TSD) that this target is using this API on this thread... Then you could use that to send the progress to the right actor.
Mar 4 2021
Yeah, apparently I got tired of typing Fundamental pretty quickly...
Mar 2 2021
While next to the binary seems like a better guess than the cwd, it does seem awkward that if the binary & dwo's aren't in the same place, you have to move files to get that heuristic to work, whereas you could just "cd" into wherever the dwo's got dumped to make the cwd version work.
Mar 1 2021
I haven't thought about the details of this implementation closely yet. How will this handled nested progress bars? For instance, if I'm Indexing (maybe even on multiple threads) and then one of the threads results in a debug symbols fetch request (e.g. dsymForUUID). Is the consumer just supposed to check the string that comes into the progress callback to match the now two simultaneous progress elements?
Feb 24 2021
I am pretty sure other Unix-es share the code to dlopen images that backs "Process::LoadImage". It is trivial compared to the ObjC ones, but is does have a couple of lines of code, 'cause it tries the dlopen and then if that fails, calls dlerror to fetch the error. That is done with a UtilityFunction because it turns out that's performance sensitive for swift.
Feb 23 2021
This will be really handy! The code for UserExpressions ends up doing this same thing (over in ClangExpressionParser::ParseInternal. But given how light-weight creating the file is I'm pretty sure it isn't worth trying to activate that code starting from the UtilityExpression. LGTM.
Feb 22 2021
LGTM at this point.
This looks great. It's a little weird to use "SBValue.value" to get a string version of something that's a pointer and then have to do regex compares. It's more direct to use "unsigned" and compare against 0x0.
Another thing that slightly bugs me about this patch is now we have the Architecture with special purpose code to modify the stop reason, and the Platform ditto. I wonder if it wouldn't be better to have a way to register interest in modifying stop infos, and then let the target & architecture sign up for that. That way the next time somebody else needs to do this we won't have to add more special purpose code to Thread.cpp.
Feb 19 2021
This seems like a fine improvement. One little nit, I would ask the current plan ShouldAutoContinue before popping it. Popping the plan does call WillPop, so the plan does have a chance to react to being popped, and you don't know what it will do. So to be on the safe side it would be better to ask questions of it as an active plan before you pop it.