Page MenuHomePhabricator

jingham (Jim Ingham)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 23 2014, 5:24 PM (351 w, 6 d)

Recent Activity

Today

jingham added a comment to D104488: Create synthetic symbol names on demand to improve memory consumption and startup times..

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.

Mon, Jun 21, 4:25 PM · Restricted Project
jingham added a comment to D104488: Create synthetic symbol names on demand to improve memory consumption and startup times..

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.

Mon, Jun 21, 10:12 AM · Restricted Project
jingham added a comment to D104437: Add test for functions with extended characters..

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.

Mon, Jun 21, 9:52 AM · Restricted Project

Tue, Jun 15

jingham committed rG80b2da42d284: Don't depend on the "run" alias doing shell expanding. (authored by jingham).
Don't depend on the "run" alias doing shell expanding.
Tue, Jun 15, 4:38 PM
jingham committed rG479c3577fb82: Missed a Windows use of ValidForThisThread in the changes for (authored by jingham).
Missed a Windows use of ValidForThisThread in the changes for
Tue, Jun 15, 3:44 PM
jingham committed rGcfb96d845a68: Convert functions that were returning BreakpointOption * to BreakpointOption &. (authored by jingham).
Convert functions that were returning BreakpointOption * to BreakpointOption &.
Tue, Jun 15, 2:34 PM
jingham closed D104162: NFC Fix the handling of BreakpointOptions - return references to make it clear when you will get a valid object.
Tue, Jun 15, 2:34 PM · Restricted Project

Fri, Jun 11

jingham requested review of D104162: NFC Fix the handling of BreakpointOptions - return references to make it clear when you will get a valid object.
Fri, Jun 11, 5:15 PM · Restricted Project

Thu, Jun 10

jingham added a comment to D104067: [lldb] Decouple ObjCLanguage from Symtab.

This looks pretty good to me.

Thu, Jun 10, 4:15 PM · Restricted Project

Tue, Jun 8

jingham added a comment to D103271: [lldb/Target] Select most relevant frame only in case of signal.

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.

Tue, Jun 8, 12:05 PM · Restricted Project
jingham added a comment to D103271: [lldb/Target] Select most relevant frame only in case of signal.

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

Tue, Jun 8, 11:42 AM · Restricted Project

Fri, Jun 4

jingham accepted D103349: [lldb] Don't print script output twice in HandleCommand.

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.

Fri, Jun 4, 3:36 PM · Restricted Project
jingham added a comment to D103701: [lldb] Set return status to failed when adding a command error.

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.

Fri, Jun 4, 10:03 AM · Restricted Project

Tue, Jun 1

jingham committed rG658f6ed1523b: Make ignore counts work as "after stop" modifiers so they play nicely with… (authored by jingham).
Make ignore counts work as "after stop" modifiers so they play nicely with…
Tue, Jun 1, 6:22 PM
jingham added a comment to D103271: [lldb/Target] Select most relevant frame only in case of signal.

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.

Tue, Jun 1, 3:56 PM · Restricted Project
jingham added a comment to D103483: [lldb] Convert the default constructor’s member initializers into default member initializers.

Thanks for taking this on. The in-class initialization is so much less boiler-plate and easier to read!

Tue, Jun 1, 3:22 PM · Restricted Project
jingham added a comment to D103349: [lldb] Don't print script output twice in HandleCommand.

Use in-class initializers

Tue, Jun 1, 11:35 AM · Restricted Project
jingham added a comment to D103439: [lldb] Print the backtrace for all threads if the test breakpoint can't be hit.

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!

Tue, Jun 1, 10:42 AM · Restricted Project

Fri, May 28

jingham added a comment to D103349: [lldb] Don't print script output twice in HandleCommand.

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?

Fri, May 28, 8:06 PM · Restricted Project

Thu, May 27

jingham requested changes to D103271: [lldb/Target] Select most relevant frame only in case of signal.

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.

Thu, May 27, 11:54 AM · Restricted Project

Wed, May 26

jingham added a comment to D103210: [lldb] Introduce Language::MethodNameInfo.

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.

Wed, May 26, 5:51 PM · Restricted Project
jingham requested review of D103217: Make ignore counts work as "after stop" modifiers so they play nicely with conditions.
Wed, May 26, 5:24 PM · Restricted Project

May 17 2021

jingham committed rG82a388371500: Revert "Reset the wakeup timeout when we re-enter the continue wait." (authored by jingham).
Revert "Reset the wakeup timeout when we re-enter the continue wait."
May 17 2021, 3:38 PM
jingham added a reverting change for rGbd5751f3d249: Reset the wakeup timeout when we re-enter the continue wait.: rG82a388371500: Revert "Reset the wakeup timeout when we re-enter the continue wait.".
May 17 2021, 3:38 PM
jingham added a reverting change for D102562: Fix to the interrupt timeout handling: reset the timeout on re-entry: rG82a388371500: Revert "Reset the wakeup timeout when we re-enter the continue wait.".
May 17 2021, 3:38 PM · Restricted Project
jingham committed rGbd5751f3d249: Reset the wakeup timeout when we re-enter the continue wait. (authored by jingham).
Reset the wakeup timeout when we re-enter the continue wait.
May 17 2021, 10:50 AM
jingham closed D102562: Fix to the interrupt timeout handling: reset the timeout on re-entry.
May 17 2021, 10:49 AM · Restricted Project

May 15 2021

jingham updated the diff for D102562: Fix to the interrupt timeout handling: reset the timeout on re-entry.

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 15 2021, 3:44 PM · Restricted Project
jingham requested review of D102562: Fix to the interrupt timeout handling: reset the timeout on re-entry.
May 15 2021, 3:05 PM · Restricted Project

May 11 2021

jingham committed rG10c309ad81e2: Removing test... (authored by jingham).
Removing test...
May 11 2021, 6:28 PM
jingham committed rG0f2eb7e6e5dc: This test is failing on Linux, skip while I investigate. (authored by jingham).
This test is failing on Linux, skip while I investigate.
May 11 2021, 6:14 PM
jingham committed rG9558b602b22c: Add an "interrupt timeout" to Process, and pipe that through the (authored by jingham).
Add an "interrupt timeout" to Process, and pipe that through the
May 11 2021, 11:57 AM
jingham closed D102085: Add an "interrupt timeout" to Process, fix a bug in handling interrupt timeouts in .
May 11 2021, 11:57 AM · Restricted Project

May 10 2021

jingham added a comment to D102085: Add an "interrupt timeout" to Process, fix a bug in handling interrupt timeouts in .

Was that what you had in mind, Greg?

May 10 2021, 2:57 PM · Restricted Project
jingham updated the diff for D102085: Add an "interrupt timeout" to Process, fix a bug in handling interrupt timeouts in .

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 10 2021, 2:55 PM · Restricted Project

May 7 2021

jingham added a comment to D102085: Add an "interrupt timeout" to Process, fix a bug in handling interrupt timeouts in .

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.

May 7 2021, 5:03 PM · Restricted Project
jingham updated the diff for D102085: Add an "interrupt timeout" to Process, fix a bug in handling interrupt timeouts in .

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.

May 7 2021, 5:01 PM · Restricted Project
jingham added a comment to D102085: Add an "interrupt timeout" to Process, fix a bug in handling interrupt timeouts in .

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 7 2021, 2:52 PM · Restricted Project
jingham requested review of D102085: Add an "interrupt timeout" to Process, fix a bug in handling interrupt timeouts in .
May 7 2021, 11:26 AM · Restricted Project

May 6 2021

jingham added a comment to D101933: If an interrupt fails, don't try to fetch any more packets from the server.

I moved the eStateExited test as Greg suggested on commit.

May 6 2021, 2:13 PM · Restricted Project
jingham committed rG72ba78c29e92: When SendContinuePacketAndWaitForResponse returns eStateInvalid, don't fetch… (authored by jingham).
When SendContinuePacketAndWaitForResponse returns eStateInvalid, don't fetch…
May 6 2021, 2:13 PM
jingham closed D101933: If an interrupt fails, don't try to fetch any more packets from the server.
May 6 2021, 2:12 PM · Restricted Project

May 5 2021

jingham requested review of D101933: If an interrupt fails, don't try to fetch any more packets from the server.
May 5 2021, 11:49 AM · Restricted Project

May 3 2021

jingham committed rG60ad0fd3c8bf: Clarify the help for "breakpoint command add" and "watchpoint command add". (authored by jingham).
Clarify the help for "breakpoint command add" and "watchpoint command add".
May 3 2021, 5:23 PM

Apr 30 2021

jingham updated subscribers of D101627: [lldb] More tests for DumpDataExtractor.

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 30 2021, 9:51 AM · Restricted Project

Apr 29 2021

jingham added a comment to D101537: [lldb] Make the NSSet formatter faster and less prone to infinite recursion.

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 29 2021, 10:22 AM · Restricted Project

Apr 28 2021

jingham added a comment to D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI).

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.

Apr 28 2021, 2:52 PM · Restricted Project

Apr 27 2021

jingham added a comment to D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI).

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.

Apr 27 2021, 10:17 AM · Restricted Project

Apr 22 2021

jingham added a comment to D98370: [lldb] Fix SBValue::Persist() for constant values.

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

Apr 22 2021, 5:17 PM · Restricted Project
jingham added a comment to D98370: [lldb] Fix SBValue::Persist() for constant values.

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'

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'
Apr 22 2021, 12:18 PM · Restricted Project

Apr 21 2021

jingham added a comment to D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI).

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 21 2021, 10:18 AM · Restricted Project
jingham added a comment to D98370: [lldb] Fix SBValue::Persist() for constant values.

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

Apr 21 2021, 10:14 AM · Restricted Project

Apr 16 2021

jingham added a comment to D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files..

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 16 2021, 9:59 AM · Restricted Project, Restricted Project, Restricted Project

Apr 15 2021

jingham added a comment to D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files..

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?

Apr 15 2021, 5:46 PM · Restricted Project, Restricted Project, Restricted Project
jingham added a comment to D100384: [lldb/Plugins] Update ScriptedProcess Process Plugin.

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 15 2021, 12:02 PM · Restricted Project

Apr 13 2021

jingham added a comment to D98370: [lldb] Fix SBValue::Persist() for constant values.

Sorry for the delay!

Apr 13 2021, 11:24 AM · Restricted Project

Apr 6 2021

jingham added a comment to D96715: [lldb] Decouple IsMasterPlan and OkayToDiscard (NFC).

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

Apr 6 2021, 12:03 PM · Restricted Project

Apr 5 2021

jingham added a reverting change for rG602ab188a7e1: Revert "Add support for fetching signed values from tagged pointers.": rGbe0ced03ba9b: Revert "Revert "Add support for fetching signed values from tagged pointers."".
Apr 5 2021, 6:21 PM
jingham committed rGbe0ced03ba9b: Revert "Revert "Add support for fetching signed values from tagged pointers."" (authored by jingham).
Revert "Revert "Add support for fetching signed values from tagged pointers.""
Apr 5 2021, 6:21 PM
jingham accepted D99828: Create setting to disable LanguageRuntime provided UnwindPlans.

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 5 2021, 10:55 AM · Restricted Project

Apr 1 2021

jingham committed rG4d9039c8dc2d: Add support for fetching signed values from tagged pointers. (authored by jingham).
Add support for fetching signed values from tagged pointers.
Apr 1 2021, 10:59 AM
jingham closed D99694: Add support for getting signed ObjC tagged pointer values.
Apr 1 2021, 10:59 AM · Restricted Project

Mar 31 2021

jingham requested review of D99694: Add support for getting signed ObjC tagged pointer values.
Mar 31 2021, 5:19 PM · Restricted Project

Mar 25 2021

jingham requested changes to D99331: [TESTS] Fix TestInlineStepping with ccac compiler.

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 25 2021, 10:45 AM · Restricted Project

Mar 24 2021

jingham committed rG3fd7d0d281a9: Disable the tests except on Darwin. (authored by jingham).
Disable the tests except on Darwin.
Mar 24 2021, 12:19 PM
jingham committed rGc8faa8c2669c: Make the stop-on-sharedlibrary-events setting work. (authored by jingham).
Make the stop-on-sharedlibrary-events setting work.
Mar 24 2021, 11:15 AM

Mar 23 2021

jingham accepted D97739: Add a progress class that can track and report long running operations that happen in LLDB..

This looks good to me.

Mar 23 2021, 10:12 AM · Restricted Project

Mar 22 2021

jingham added inline comments to D97739: Add a progress class that can track and report long running operations that happen in LLDB..
Mar 22 2021, 6:21 PM · Restricted Project
jingham added inline comments to D97739: Add a progress class that can track and report long running operations that happen in LLDB..
Mar 22 2021, 4:17 PM · Restricted Project
jingham added a comment to D97739: Add a progress class that can track and report long running operations that happen in LLDB..

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

Mar 22 2021, 4:00 PM · Restricted Project
jingham added a comment to D98822: [lldb] [Process] Watch for fork/vfork notifications.

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.

Mar 22 2021, 10:38 AM · Restricted Project

Mar 19 2021

jingham added a reverting change for rG9406d4313881: Make the stop-on-sharedlibrary-events setting work.: rG9d081a7ffe5c: Revert "Make the stop-on-sharedlibrary-events setting work.".
Mar 19 2021, 12:40 PM
jingham committed rG9d081a7ffe5c: Revert "Make the stop-on-sharedlibrary-events setting work." (authored by jingham).
Revert "Make the stop-on-sharedlibrary-events setting work."
Mar 19 2021, 12:40 PM
jingham added a reverting change for rGa8d62fc8ff1c: Skip all the tests for Windows.: rGe8e07b3a5e60: Revert "Skip all the tests for Windows.".
Mar 19 2021, 12:40 PM
jingham added a reverting change for D98914: Make target.process.stop-on-sharedlibrary-events setting work: rG9d081a7ffe5c: Revert "Make the stop-on-sharedlibrary-events setting work.".
Mar 19 2021, 12:40 PM · Restricted Project
jingham committed rGe8e07b3a5e60: Revert "Skip all the tests for Windows." (authored by jingham).
Revert "Skip all the tests for Windows."
Mar 19 2021, 12:40 PM
jingham committed rGa8d62fc8ff1c: Skip all the tests for Windows. (authored by jingham).
Skip all the tests for Windows.
Mar 19 2021, 12:06 PM
jingham added a comment to D98914: Make target.process.stop-on-sharedlibrary-events setting work.

I added a couple more tests for having a breakpoint at the load site before committing.

Mar 19 2021, 12:02 PM · Restricted Project
jingham committed rG9406d4313881: Make the stop-on-sharedlibrary-events setting work. (authored by jingham).
Make the stop-on-sharedlibrary-events setting work.
Mar 19 2021, 12:02 PM
jingham closed D98914: Make target.process.stop-on-sharedlibrary-events setting work.
Mar 19 2021, 12:02 PM · Restricted Project
jingham added a comment to D98822: [lldb] [Process] Watch for fork/vfork notifications.

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 19 2021, 10:03 AM · Restricted Project

Mar 18 2021

jingham added a comment to D97739: Add a progress class that can track and report long running operations that happen in 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...

Mar 18 2021, 7:27 PM · Restricted Project
jingham requested review of D98914: Make target.process.stop-on-sharedlibrary-events setting work.
Mar 18 2021, 7:19 PM · Restricted Project
jingham committed rG71c4da83b67a: Don't assume that stepping out of a function will land on the next line. (authored by jingham).
Don't assume that stepping out of a function will land on the next line.
Mar 18 2021, 5:48 PM

Mar 16 2021

jingham added inline comments to D98653: [lldb] Refactor variable paths to support languages with non-pointer "this" (NFC).
Mar 16 2021, 11:42 AM · Restricted Project

Mar 8 2021

jingham added a comment to D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files..

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 8 2021, 10:19 AM · Restricted Project, Restricted Project, Restricted Project
jingham added a comment to D97739: Add a progress class that can track and report long running operations that happen in LLDB..

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

Mar 8 2021, 10:07 AM · Restricted Project

Mar 5 2021

jingham added a comment to D97739: Add a progress class that can track and report long running operations that happen in LLDB..

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

Mar 5 2021, 3:29 PM · Restricted Project
jingham added a comment to D97739: Add a progress class that can track and report long running operations that happen in LLDB..

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 5 2021, 2:20 PM · Restricted Project

Mar 4 2021

jingham accepted D97985: [lldb] Rename QueueFundamentalPlan to QueueBasePlan (NFC).

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

Mar 4 2021, 3:39 PM · Restricted Project

Mar 2 2021

jingham added a comment to D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files..

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 2 2021, 11:44 AM · Restricted Project, Restricted Project, Restricted Project

Mar 1 2021

jingham added a comment to D97739: Add a progress class that can track and report long running operations that happen in LLDB..

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?

Mar 1 2021, 6:07 PM · Restricted Project

Feb 24 2021

jingham added a comment to D97249: [lldb] Support debugging utility functions.

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 24 2021, 11:11 AM · Restricted Project

Feb 23 2021

jingham accepted D97249: [lldb] Support debugging utility functions.

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 23 2021, 9:28 AM · Restricted Project

Feb 22 2021

jingham accepted D97165: [lldb] Add deref support and tests to shared_ptr synthetic.

LGTM at this point.

Feb 22 2021, 5:36 PM · Restricted Project
jingham added a comment to D97165: [lldb] Add deref support and tests to shared_ptr synthetic.

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.

Feb 22 2021, 12:16 PM · Restricted Project
jingham added a comment to D48177: Suppress SIGSEGV on Android when the program will recover.

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 22 2021, 10:14 AM

Feb 19 2021

jingham added inline comments to D97076: [lldb] Refine ThreadPlan::ShouldAutoContinue.
Feb 19 2021, 5:06 PM · Restricted Project
jingham accepted D97076: [lldb] Refine ThreadPlan::ShouldAutoContinue.
Feb 19 2021, 4:28 PM · Restricted Project
jingham added a comment to D97076: [lldb] Refine ThreadPlan::ShouldAutoContinue.

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.

Feb 19 2021, 4:28 PM · Restricted Project