- User Since
- Mar 2 2013, 8:12 AM (272 w, 5 d)
The missing context here is that the lldb-mi -target-select command currently calls HandleCommand("target modules search-paths add", ...).
Is adding a new SBAPI the right approach to implementing this functionality without going through HandleCommand? Or is HandleCommand appropriate in this case?
Remove unused include.
Remove whitespace changes.
Once we encounter a # directive we are (most likely) looking at some form of preprocessed source and that means that the checksum will inevitably be different than what we would have gotten were we reading the file directly, because of the preprocessing. At this point the value of the hash approaches zero. I think dropping all checksums is reasonable in that situation.
Forgot to update unit tests.
Updated patch according to Zachary's suggestion.
Tue, May 22
This would be very useful to find bugs in LLVM. That said, this looks like it is more a quality-of-implementation issue, and not a DWARF correctness issue. Should we separate the two? Or does DWARF mandate this?
Adding Duncan to the review list who (presumably) added the TODO and may have some thoughts on this.
Mon, May 21
Seems reasonable to me.
Thanks, this was very confusing!
getVariableLocation() assumes all DbgInfoIntrinsic has the address value as its first operand. It is not the case for DILabel.
Sat, May 19
Fri, May 18
Thanks for jumping on this Amara — I just wanted to point out that we ususally don't revert lldb changes that only break the lldb-xcode bot if they pass on the lldb-cmake bot at the same time. When this happens it usually means that the lldb Xcode project must be updated and it's too much to ask from all open source contributors to get access to a machine running Xcode to do this. Instead one of the Apple LLDB developers usually goes in and updates the Xcode project for them.
Thu, May 17
Okay, that sounds promising! Then let's proceed this way:
For the experiment you can probably just stick it into CMICmnLLDBDebugger::InitSBDebugger().
My overall mental image of lldb-mi is very incomplete, but I'm imagining lldb-mi as having one thread that wakes up every n milliseconds checks for command input and then calls into the SBAPI to handle the commands. If that is how it works, then one very simple thing we can try is to call SBDebugger::SetAsync(false), thus forcing all the SBAPI calls to block until completion.
Wed, May 16
The advantage of the second one is that we will have the ability to inject commands which depend on the results of previous commands (something that I think we will need, sooner or later).
Does that mean we can now also remove the #ifdef APPLE from the objectfile unit tests?
Alexander, I don't want this discussion to block you progress too much. It's important that we sort out the best way to test lldb-mi early on (and I hope this doesn't take too long), but while we are debating this you could also land the patch with the classic expect-based testcase that you had and go back and convert it later on. It's a bit more work, but converting the testcases shouldn't be too difficult.
Tue, May 15
I couldn't find any additional uses of FindGlobalVariables in swift-lldb either.
If we're not able to come up with a command to make lldb-mi wait until the target stops (maybe there is one already? I know very little about lldb-mi), we may have to revisit the whole testing strategy...
It would be good to add both a testcase for your intended use-case and one with an assembler label (if that doesn't exist already).
I'm not sure that stripping the leading _ unconditionally is correct either. This code exists to emit DWARF for labels in assembler source code and I'm assuming that this patch will break that use-case.
Mon, May 14
Thanks for converting the test!
Neither the PR nor this review contain any information on why removing the assertion is the correct action here. Can you explain why this is the right fix, and what the MDNode is when the assertion fires?
Sat, May 12
Since we regressed on this before: could you come up with a testcase for this?
Fri, May 11
I suppose we could add a test for the erroneous case?
Vectorized loop by nature executes multiple iterations of the sequential loop at the same time. So, the same variable X (say, i32 type) is widened to widenedX (4 x i32 type) to represent 4 different values of X, say, for iterations i, i+1, i+2, and i+3 executing together. In terms of debugging vectorized code, when the programmer points to X during vector execution, debugger needs to show 4 different values of X in this scenario. It's different from placing one value in a larger sized register.
Thu, May 10
Added a REQUIRES: darwin
Much smaller test thanks to Pavel's suggestion!
The code looks good — I think it would be worthwhile to try and convert this test to a LIT test. You can try to model it after lit/Expr/TestCallUserDefinedFunction.test. You will also need to modify lit/lit.cfg to define a %lldb-mi variable, but that should be straightforward. Let me know if that works. If we run into too many problems we can stick to the expect-style tests for now, but I'd like to do this experiment at the very beginning so all the subsequent patches can benefit from it.
If dwarfdump can't find a problem with the output (and there is a problem with it) then that's a bug in dwarfdump. If you want to do functional testing, you'll probably want to pipe the result through something like llvm-symbolizer or lldb as an end-to-end test.
Wed, May 9
Actually, the example I posted was not a good one. I'm fairly certain that that was de facto static information or otherwise not particularly important information.
Got it. We certainly do have dependent checks in our current tests:
This patch is making it so the order of the generated MIR instructions is closer to the order of the IR instructions they were generated from, which is the intended / documented behavior of SelectionDag. My vote is to land this change unless one of the stakeholders of the respective targets objects.
Can you give an example of what you mean by "specifying the lldb-mi commands up-front"? it's not immediately obvious to me how that would look like.
Okay, that's no good. What would you think about writing tests using lit & FileCheck? The lldb-mi tests should not be in the debuginfo category anyway, since they only test the alternative driver.
So IIUC you are remembering the SlotIndex that the interval was trimmed to instead of assuming that it was the one immediately preceding the beginning to the interval?
Looks like this is bringing the interface a *little* closer to the llvm one.
Could you please update the comment explaining what the new check is checking for?
Tue, May 8
Thanks, the new test looks much better!
How expensive is this check when running it on a program the size of clang?
I've slowly come to always prefer the smallest possible test case, since anything else makes it impossible for future readers to determine what the test is supposed to be testing and it is very easy to end up with cargo-cult testcases that don't test anything interesting after a while.
Mon, May 7
That testcase is huge. Do all instructions need !dbg locations?
Thu, May 3
As Greg suspected, the problem was that target was null. I rewrote that patch to not use a SymbolContext in the first place.
Yes, I must have added a mistake in my last edit before landing this. I reverted that patch for now.
Thanks for the excellent feedback! Addressed comments.
If by as-is you mean with a big comment in the config file before the creation of check-single that it is broken, then yes, let's get the bots unblocked and then fix the issue.
If that's okay with you I would suggest landing this as is to unblock the bots, and we'll keep working on fixing the check-single target ASAP, too.
Is there a way to land a partial version of this patch to un-block the green dragon bots and keep iterating on the check-single target separately? Or do other bots depend on check-single working correctly? I'm really uneasy about the fact that we are loosing signal from the bots while they are red. (This is not meant to say that one platform is more important than another, I'm just searching for a way to get all the bots functioning again as quickly as possible, and "no" is an acceptable answer :-).
So this is making the assertion even stricter. Thanks!