In D104488#2831818, @clayborg wrote:

In D104488#2830992, @jingham wrote:

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 unnamed symbol when you crashed again.

Yes, symbol IDs are consistent as they encode the UserID of the symbol as the number which will be the same on each run as long as the binary doesn't change. The UserID for synthetic symbols always start with the last valid actual symbol index from the main symbol table. So the numbers are just as good as they are before, they just don't start at 1 anymore, the start at the size of the actual symbol table.

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.

This looks pretty good to me.

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

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.

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!

In D103349#2791614, @JDevlieghere wrote:

Use in-class initializers

In D103439#2791324, @teemperor wrote:

In D103439#2791298, @JDevlieghere wrote:

LGTM. Are there other places where we check this, either in lldbutil or maybe more generally a pattern in the tests that could be extracted into a helper?

From what I can see all our lldbutil functions that run to a breakpoint end up calling this function so this should cover everything beside the tests that manually do stuff like "continue" and so on. But there might be some utility function for going to the next breakpoint that are missing this. I'll check, thanks!

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?

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.

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.

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

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.

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.

I moved the eStateExited test as Greg suggested on commit.

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

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.

In D100965#2722149, @teemperor wrote:

In D100965#2720235, @jingham wrote:

In D100965#2716239, @teemperor wrote:

I am wondering how SourceLocationSpec is related to lldb's Declaration class (which is FileSpec + line + column and describes a location in source code)? It seems like the current SourceLocationSpec is just a Declaration with the two additional search variables (and the first iteration was exactly the same as Declaration).

Could we maybe turn SourceLocationSpec to store a lldb::Declaration instead of file/line/column? I'm aware that the Declaration class first needs some cleanup (and a removal of that #ifdef around column....), but right now we are already using Declarations in a bunch of places to describe source locations.

Seems to me Declarations and SourceLocationSpec's are different things. A Declaration describes just where something is in a source file. And moreover, there need to be a lot of them, since every function, type etc has one. So you might be concerned about size for this entity.

A SourceLocationSpec (maybe we should make a better name?) is about specifying how you search for a source location (e.g. to set a breakpoint on it.) And there are never going to be lots of them in flight, since they are tied to user actions. So we are fairly free to add extra fields to this if we have other ways of specifying the match to a source location. OTOH, Declarations don't need "include_inlines" or "exact_match" or anything else we might end up adding to specify how to look for matches against a source line specification somebody used in break set or other places.

So I don't think it makes sense to conflate the two.

My point was that Declaration should be a member of SourceLocationSpec (which will then be a Declaration + the search parameters). I agree that those two should stay two different classes. From what understand from Ismail this patch is (was?) going in the direction you're describing of having SourceLocationSpec taking over Declaration (which would then raise the memory concerns you mentioned).

In other words: If the file/line/column members could just be a Declaration (assuming we remove that ifdef around Declarations column member) then IMHO this would be nicer. I would also like the idea of maybe typedef'ing the line/column member types in addition to that. Not sure if we need uint32_t for columns or maybe we need one day uint64_t for lines.

In D100965#2716239, @teemperor wrote:

I am wondering how SourceLocationSpec is related to lldb's Declaration class (which is FileSpec + line + column and describes a location in source code)? It seems like the current SourceLocationSpec is just a Declaration with the two additional search variables (and the first iteration was exactly the same as Declaration).

Could we maybe turn SourceLocationSpec to store a lldb::Declaration instead of file/line/column? I'm aware that the Declaration class first needs some cleanup (and a removal of that #ifdef around column....), but right now we are already using Declarations in a bunch of places to describe source locations.

In D98370#2710058, @werat wrote:

In D98370#2709828, @jingham wrote:

Be careful here. There are really two kinds of persistent variables: "expression results" and variables created in the expression parser. The former are by design constant. The idea is that you can use them to checkpoint the state of a value and refer to it later. You can use their values in expressions, but they aren't supposed to be modifiable. Those are the ones called $NUM.

The ones you make in an expression, like:

(lldb) expr int $myVar = 20

on the other hand are modifiable.

What about the values created via SBValue::Persist() then? They have names like $NUM, but they can be either of the two you mentioned and more (values created via CreateValueFromData or values acquired via FindVariable):

In D98370#2708672, @werat wrote:

In D98370#2705741, @jingham wrote:

Sure. But but when I was poking around at it a little bit, it seems like the other use cases already work, and the only one that was failing was the case where you call persist on a persistent variable. If that is really true, then maybe we should fix the failing case directly.

Right now Persist() doesn't really work for values created via CreateValueFromData. You can read them, but can't modify:

>>> data = lldb.SBData.CreateDataFromUInt64Array(lldb.process.GetByteOrder(), lldb.process.GetAddressByteSize(), [42])
>>> v = lldb.target.CreateValueFromData('v', data, lldb.target.GetBasicType(lldb.eBasicTypeUnsignedLong))
>>> v.value
'42'
>>> vp = v.Persist()
>>> vp.name
'$3'
>>> lldb.frame.EvaluateExpression('$3').value
'42'
>>> lldb.frame.EvaluateExpression('++$3 + 1').value
>>> lldb.frame.EvaluateExpression('++$3 + 1').error.GetCString()
"error: supposed to interpret, but failed: Interpreter couldn't read from memory\n"

However I realized my patch doesn't completely fixes it either:

>>> data = lldb.SBData.CreateDataFromUInt64Array(lldb.process.GetByteOrder(), lldb.process.GetAddressByteSize(), [42])
>>> v = lldb.target.CreateValueFromData('v', data, lldb.target.GetBasicType(lldb.eBasicTypeUnsignedLong))
>>> vp = v.Persist()
>>> vp.name
'$0'
>>> lldb.frame.EvaluateExpression('$0').value
'42'
>>> lldb.frame.EvaluateExpression('++$0').value
'43'
>>> lldb.frame.EvaluateExpression('++$0').value
'44'
>>> vp.value
'42'

In D98370#2705741, @jingham wrote:

Not sure why? The API is a request: "I made a variable somehow, and I would like you to make it persist so I can use its value later on even if the underlying data has changed." Why do you care whether you get a copy of an already persistent or just a shared value?

You're right, I got confused by something else. I don't care if I get a new name/copy, as long as I can use it by the returned name it's fine. However I want to point out that the current API does generate a new name every time (but the it points to the same data):

>>> x = lldb.frame.FindVariable('x')
>>> x.value
'1'
>>> xp1 = x.Persist()
>>> xp1.name
'$0'
>>> xp2 = x.Persist()
>>> xp2.name
'$1'
>>> lldb.frame.EvaluateExpression('++$0 + ++$1').value
'3'
>>> xp1.value
'3'
>>> xp2.value
'3'

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.

In D98370#2704597, @werat wrote:

In D98370#2686515, @jingham wrote:

If persisting already persistent variables is indeed the only failure case, then I wonder if it wouldn't be more straightforward to just see if the ValueObject is already a persistent variable and have Persist just return the incoming variable.

Persisting already persistent variables is not the only valid use case, one might also want to persist variables created via SBTarget::CreateValueFromData().

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.

In D97786#2693381, @cmtice wrote:

I had to revert this change because the test case broke the windows builder. What's the right way to update/mark the test case so that it is only run on appropriate architectures & operating systems?

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?

Sorry for the delay!

I don't understand how DiscardPlansConsultingMasterPlan in this patch works.

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.

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?

This looks good to me.

This seems like the right way to go. I made a couple trivial comment comments.

In D98822#2640881, @labath wrote:

In D98822#2637871, @jingham wrote:

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.

This is no longer true -- gdb supports multiple inferiors these days (I'm not sure when exactly did it grow that support). The automatic detaching of the fork child can be disabled by the "detach-on-fork" setting, at which point you get two inferiors for each fork.

Michal doesn't appear to be interested in this feature right now, just the ability to follow the child process. I think that's fair, as the two inferior setup would be more complicated. However, I am keeping this scenario in mind and making sure that it's possible to support it in the future. The idea is that at the gdb protocol level we will have the notion of two inferiors (using the same syntax that gdb does), but the only thing that the client (and server, mostly) support is to immediately detach from one of the processes after a fork. Later this could be changed to do something more interesting.

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.

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

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.

In D97739#2610521, @clayborg wrote:

In D97739#2608510, @friss wrote:

In D97739#2607961, @clayborg wrote:

In D97739#2607869, @jingham wrote:

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.

that is true, but it is a global module repository that benefits both debuggers. And I very rarely debug two things at the same time, so most of the time for most people this will be beneficial and shouldn't cause too much confusion.

Just one tidbit here. Most users are actually routinely running tens of debuggers at the same time, because tests run in parallel and they have a debugger attached by default. Now if you have a long running operation kick in in your unit tests, you might already have a different kind of issue, but I’d like to avoid a world where the IDE displays spurious and wrong information because of this.

But you wouldn't actually hookup any progress callbacks on any of these debuggers right? You don't make a window for each test, IIRC you are only running it under the debugger so you can report issues by using the debugger. Am I remember this correctly? What happens when a test crashes? If you are running 100 tests in parallel and 10 of them crash, do you open a window for each one that does crash? Or do you manually have to debug the test in order to stop at breakpoints or at a crash?

If we truly need debugger specific task progress, that is a lot more work and we have no great solution. One idea is we could end up having progress items take a SymbolContextScope pointer as an optional parameter that would allow us to grab the module pointer from it and then ask the debugger if any targets contain this module prior to reporting progress. This would be a bit expensive code for a lot of quick progress updates as we would need to iterate over each target and each target's module list and we would still have many system libraries reporting progress to all debuggers when targets contain the same system libraries.

The threading overhead and expense of delivering SBEvents also seems like overkill as threading itself and waiting using a mutex + condition will slow down progress delivery especially if we have a progress that does a bunch of updates. And once we receive the event we will need to make a static function call to extract all progress variables (total, completed, progress_id, baton, etc).

In D97739#2607993, @clayborg wrote:

I agree that we should avoid SBEvent after thinking about it.

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.

Yeah, apparently I got tired of typing Fundamental pretty quickly...

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.

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?

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.

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.

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.

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.