aprantl (Adrian Prantl)
User

Projects

User Details

User Since
Mar 2 2013, 8:12 AM (272 w, 5 d)

Recent Activity

Today

aprantl added a comment to D47097: [DebugInfo] Preserve scope in auto generated StoreInst.

Can we step back a second and better explain what the problem is? With current Clang the debug info for this function looks okay to me.
The store that is "missing" a debug location is homing the formal parameter to its local stack location; this is part of prolog setup, not "real" code.

Isn't this the reason the artificial debug loc exists? The store in the prolog might not be real code but it should still have the scope that the rest of the function has.

Thu, May 24, 10:12 AM
aprantl accepted D47263: [dwarfdump] Have -c and -p work together.
Thu, May 24, 8:57 AM

Yesterday

aprantl added inline comments to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..
Wed, May 23, 5:16 PM · Restricted Project
aprantl added inline comments to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..
Wed, May 23, 5:14 PM · Restricted Project
aprantl added a comment to D47302: [lldb, lldb-mi] Add method AddCurrentTargetSharedObjectPath to the SBDebugger..

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?

Wed, May 23, 4:55 PM
aprantl added reviewers for D47302: [lldb, lldb-mi] Add method AddCurrentTargetSharedObjectPath to the SBDebugger.: clayborg, jingham.
Wed, May 23, 4:53 PM
aprantl added inline comments to D47097: [DebugInfo] Preserve scope in auto generated StoreInst.
Wed, May 23, 1:45 PM
aprantl added inline comments to D47097: [DebugInfo] Preserve scope in auto generated StoreInst.
Wed, May 23, 1:17 PM
aprantl added inline comments to D47235: Move ModuleList's dependency on clangDriver into Host.
Wed, May 23, 1:11 PM
aprantl added inline comments to D47235: Move ModuleList's dependency on clangDriver into Host.
Wed, May 23, 12:44 PM
aprantl added a comment to D46005: [test] Add a utility for dumping all tests in the dotest suite.

FWIW, I think the single biggest improvement one could make to the LLDB test suite runtime is to compile all inferiors up front as part of the CMake step. If you run the test suite twice every inferior is unnecessarily compiled twice.

I'm a big proponent of moving as much logic as possible into the configuration stage, but a common use case (at least for us) is testing a single build with a different compiler/configuration.

Then the configure / CMake stage could build all inferiors with every possible configuration you want to test, each one writing to a different output directory. Sorta like how in LLVM you can specify LLVM_TARGETS_TO_BUILD=X86,ARM,... you could specify something like LLDB_TEST_INFERIOR_CONFIGS=gcc-x64;gcc-x86;clang-arm64.

Obviously this is a big change and a lot of work, but I think it would make the test suite run in under 30 seconds

I don't think that moving this to the configure stage is the right choice. I'm also skeptical about your claim about the saved time (are you talking about Windows?).

In my opinion the configuration should configure what is being built, not what is being tested. I don't want to have to lock down at configure time which tests I will be running. As an analogy, this is similar to how LLVM has the in-tree regression tests but also the external test-suite that can be run with endless permutations of options, to test one specific build of LLVM, without having to reconfigure/rebuild.

This is a bit of a digression from this patch,

Agreed :-)

Wed, May 23, 11:58 AM
aprantl added inline comments to D47235: Move ModuleList's dependency on clangDriver into Host.
Wed, May 23, 11:51 AM
aprantl updated the diff for D47235: Move ModuleList's dependency on clangDriver into Host.

Remove unused include.

Wed, May 23, 11:47 AM
aprantl updated the diff for D47235: Move ModuleList's dependency on clangDriver into Host.

Remove whitespace changes.

Wed, May 23, 11:45 AM
aprantl added inline comments to D47097: [DebugInfo] Preserve scope in auto generated StoreInst.
Wed, May 23, 11:43 AM
aprantl added a comment to D47260: Testcase for dwarf 5 crash/assert when calculating a checksum for an expansion.

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.

Wed, May 23, 11:40 AM
aprantl added a comment to D46005: [test] Add a utility for dumping all tests in the dotest suite.

FWIW, I think the single biggest improvement one could make to the LLDB test suite runtime is to compile all inferiors up front as part of the CMake step. If you run the test suite twice every inferior is unnecessarily compiled twice.

I'm a big proponent of moving as much logic as possible into the configuration stage, but a common use case (at least for us) is testing a single build with a different compiler/configuration.

Then the configure / CMake stage could build all inferiors with every possible configuration you want to test, each one writing to a different output directory. Sorta like how in LLVM you can specify LLVM_TARGETS_TO_BUILD=X86,ARM,... you could specify something like LLDB_TEST_INFERIOR_CONFIGS=gcc-x64;gcc-x86;clang-arm64.

Obviously this is a big change and a lot of work, but I think it would make the test suite run in under 30 seconds

Wed, May 23, 11:33 AM
aprantl added inline comments to D47263: [dwarfdump] Have -c and -p work together.
Wed, May 23, 10:48 AM
aprantl updated the diff for D47235: Move ModuleList's dependency on clangDriver into Host.

Forgot to update unit tests.

Wed, May 23, 10:45 AM
aprantl updated the diff for D47235: Move ModuleList's dependency on clangDriver into Host.

Updated patch according to Zachary's suggestion.

Wed, May 23, 10:24 AM

Tue, May 22

aprantl created D47235: Move ModuleList's dependency on clangDriver into Host.
Tue, May 22, 4:32 PM
aprantl added a comment to D47209: [WIP] Add debug location check to verifier..

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?

Tue, May 22, 11:37 AM · debug-info
aprantl added a comment to D47179: [LLVM-C] Add Bindings For Named Metadata.

Adding Duncan to the review list who (presumably) added the TODO and may have some thoughts on this.

Tue, May 22, 9:09 AM
aprantl added a reviewer for D47179: [LLVM-C] Add Bindings For Named Metadata: dexonsmith.
Tue, May 22, 9:06 AM

Mon, May 21

aprantl added a comment to D47158: [DWARFv5] Put DWO ID in its place.

Seems reasonable to me.

Mon, May 21, 2:12 PM · debug-info
aprantl accepted D47152: [DebugInfo] Use absolute addresses in location lists.

Thanks, this was very confusing!

Mon, May 21, 11:22 AM
aprantl added a comment to D46738: [DebugInfo] Fix PR37395..

getVariableLocation() assumes all DbgInfoIntrinsic has the address value as its first operand. It is not the case for DILabel.

Mon, May 21, 8:52 AM

Sat, May 19

aprantl accepted D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..

LGTM!

Sat, May 19, 4:51 PM · Restricted Project
aprantl accepted D47110: [LLDB, lldb-mi] Add option --synchronous..

Looks good!

Sat, May 19, 4:49 PM

Fri, May 18

aprantl updated subscribers of D46889: [DWARF] Extract indexing code into a separate class hierarchy.

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.

Fri, May 18, 10:53 AM
aprantl added inline comments to D47062: Suggest lldb-dotest to reproduce a failure..
Fri, May 18, 10:47 AM
aprantl added inline comments to D47062: Suggest lldb-dotest to reproduce a failure..
Fri, May 18, 8:31 AM

Thu, May 17

aprantl added a comment to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..

Okay, that sounds promising! Then let's proceed this way:

Thu, May 17, 10:25 AM · Restricted Project
aprantl added a comment to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..

For the experiment you can probably just stick it into CMICmnLLDBDebugger::InitSBDebugger().

Thu, May 17, 9:51 AM · Restricted Project
aprantl added a comment to D46934: Make ObjectFileMachO work on non-darwin platforms.

Does that mean we can now also remove the #ifdef APPLE from the objectfile unit tests?

Which ones do you mean? I wasn't aware we had any. The thing I know of, which would be interesting to enable is the MachO core file tests. This can't be done yet because the respective process plugin is apple-only, but I haven't looked at what it would take to enable it everywhere.

Thu, May 17, 9:09 AM
aprantl added inline comments to D46934: Make ObjectFileMachO work on non-darwin platforms.
Thu, May 17, 9:08 AM
aprantl added a comment to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..

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.

Thu, May 17, 9:02 AM · Restricted Project

Wed, May 16

aprantl added a comment to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..
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).
Wed, May 16, 6:10 PM · Restricted Project
aprantl added a comment to D46934: Make ObjectFileMachO work on non-darwin platforms.

Does that mean we can now also remove the #ifdef APPLE from the objectfile unit tests?

Wed, May 16, 1:28 PM
aprantl added a comment to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..

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.

Wed, May 16, 9:08 AM · Restricted Project
aprantl added inline comments to D46934: Make ObjectFileMachO work on non-darwin platforms.
Wed, May 16, 8:53 AM
aprantl added inline comments to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..
Wed, May 16, 8:51 AM · Restricted Project

Tue, May 15

aprantl added inline comments to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..
Tue, May 15, 3:27 PM · Restricted Project
aprantl added inline comments to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..
Tue, May 15, 1:36 PM · Restricted Project
aprantl added reviewers for D46885: Remove append parameter to FindGlobalVariables: clayborg, jingham, labath.
Tue, May 15, 9:39 AM
aprantl added a comment to D46885: Remove append parameter to FindGlobalVariables.

I couldn't find any additional uses of FindGlobalVariables in swift-lldb either.

Tue, May 15, 9:38 AM
aprantl added a comment to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..

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

If one doesn't exist then I think it would be reasonable to invent one. Handling an additional command that is used in testing only should not break any existing clients.

I am not sure I like the direction of "lets test lldb-mi in a way that doesn't behave like a real debug session" by making new testing stuff so we can text scrape.

Tue, May 15, 9:23 AM · Restricted Project
aprantl added a comment to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..

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

Tue, May 15, 8:02 AM · Restricted Project
aprantl added a comment to D46878: Add DW_AT_linkage_name for DW_TAG_labels.

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

Tue, May 15, 7:53 AM
aprantl requested changes to D46878: Add DW_AT_linkage_name for DW_TAG_labels.

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.

Tue, May 15, 7:52 AM

Mon, May 14

aprantl accepted D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..

Thanks, LGTM.

Mon, May 14, 2:43 PM · Restricted Project
aprantl updated subscribers of D46850: [DebugInfo] Generate fixups as emitting DWARF .debug_line..
Mon, May 14, 2:17 PM
aprantl accepted D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..

Thanks for converting the test!

Mon, May 14, 12:51 PM · Restricted Project
aprantl added a comment to D46738: [DebugInfo] Fix PR37395..

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?

Mon, May 14, 8:57 AM
aprantl added a comment to D46787: [CommandLine] Error message for incorrect PositionalEatArgs usage.

Great, thanks!

Mon, May 14, 8:54 AM

Sat, May 12

aprantl added inline comments to D46792: [LLVM-C] Add Bindings For Module Flags.
Sat, May 12, 9:01 AM
aprantl added a comment to D46790: [bugpoint] Actually skip a modules that cause verifier errors.

Since we regressed on this before: could you come up with a testcase for this?

Sat, May 12, 8:59 AM

Fri, May 11

aprantl accepted D46787: [CommandLine] Error message for incorrect PositionalEatArgs usage.

I suppose we could add a test for the erroneous case?

Fri, May 11, 5:12 PM
aprantl updated subscribers of D44338: [LV][VPlan] Build plain CFG with simple VPInstructions for outer loops..
Fri, May 11, 2:37 PM
aprantl added a comment to D44338: [LV][VPlan] Build plain CFG with simple VPInstructions for outer loops..

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.

Fri, May 11, 2:01 PM
aprantl added a comment to D44338: [LV][VPlan] Build plain CFG with simple VPInstructions for outer loops..

Can you outline what would make updating dbg.value intrinsics to point to vector instructions special, such that it can't be handled immediately?

I keep pushing the implementers (Diego, Satish, etc.) very hard to maintain good correspondence between input IR and output IR. Assuming that dbg.value can handle widened values from scalar values, there shouldn't be anything special.

Fri, May 11, 8:57 AM
aprantl added inline comments to D46324: [BranchFolding] Allow hoisting to block with a single conditional branch..
Fri, May 11, 8:53 AM
aprantl accepted D46739: [DebugInfo] Only handle DBG_VALUE in InlineSpiller.
Fri, May 11, 7:50 AM

Thu, May 10

aprantl created D46736: HostInfoMacOSX: Share the clang resource directory with Swift..
Thu, May 10, 5:19 PM
aprantl updated the diff for D46669: Retrieve the deployment target when retrieving an object file's triple.

Added a REQUIRES: darwin

Thu, May 10, 2:53 PM
aprantl updated the diff for D46669: Retrieve the deployment target when retrieving an object file's triple.

Much smaller test thanks to Pavel's suggestion!

Thu, May 10, 2:51 PM
aprantl added inline comments to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..
Thu, May 10, 2:25 PM · Restricted Project
aprantl added a comment to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..

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.

Thu, May 10, 2:24 PM · Restricted Project
aprantl added a comment to D46628: [ELF] Add --strip-debug-non-line option.

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.

Thu, May 10, 9:21 AM
aprantl added inline comments to D46628: [ELF] Add --strip-debug-non-line option.
Thu, May 10, 9:20 AM
aprantl added a comment to D44338: [LV][VPlan] Build plain CFG with simple VPInstructions for outer loops..

Thanks, Florian! Some comments below.

One thing worth discussing briefly before this goes in may be what the plan for dealing with debug info will be with VPlan. Adding @aprantl in case he has some thoughts.

I'm not aware of any particular proposal for debug info in VPlan at this point but I will check with my team. Currently, DbgInfoIntrinsic would be represented as a regular VPInstruction. We could think about if a specific representation for this is necessary in VPlan.

Thu, May 10, 9:17 AM

Wed, May 9

aprantl created D46669: Retrieve the deployment target when retrieving an object file's triple.
Wed, May 9, 6:11 PM
aprantl added a comment to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..

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.

Wed, May 9, 1:10 PM · Restricted Project
aprantl added a comment to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..

Got it. We certainly do have dependent checks in our current tests:

Wed, May 9, 1:10 PM · Restricted Project
aprantl accepted D46158: [DAGCombiner] Set the right SDLoc on a newly-created sextload.

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.

Wed, May 9, 12:06 PM
aprantl added a comment to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..

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.

Wed, May 9, 11:16 AM · Restricted Project
aprantl added a comment to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..

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.

Wed, May 9, 9:31 AM · Restricted Project
aprantl added a comment to D46599: [DbgInfo] Attempt to fix bug 37149.

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?

Wed, May 9, 9:10 AM
aprantl added a comment to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..

Out of curiosity, are there any plans to improve the lldb-mi test reliability situation? As it stands now, most of the lldb-mi tests are disabled one way or another due to them being flaky.

Wed, May 9, 9:03 AM · Restricted Project
aprantl accepted D46606: General cleanup to minimize the .debug_types patch.

Looks like this is bringing the interface a *little* closer to the llvm one.

Wed, May 9, 8:51 AM
aprantl added a comment to D46599: [DbgInfo] Attempt to fix bug 37149.

Could you please update the comment explaining what the new check is checking for?

Wed, May 9, 8:00 AM
aprantl added reviewers for D46599: [DbgInfo] Attempt to fix bug 37149: probinson, wolfgangp.
Wed, May 9, 7:56 AM

Tue, May 8

aprantl added a comment to D46568: [SimplifyCFG] Fix a crash when folding PHIs.

Thanks, the new test looks much better!

Tue, May 8, 10:24 AM
aprantl accepted D46583: DWARFVerifier: Check "completeness" of .debug_names section.
Tue, May 8, 9:58 AM
aprantl added a comment to D46583: DWARFVerifier: Check "completeness" of .debug_names section.

Awesome.

Tue, May 8, 9:57 AM
aprantl added a comment to D46583: DWARFVerifier: Check "completeness" of .debug_names section.

How expensive is this check when running it on a program the size of clang?

Tue, May 8, 9:32 AM
aprantl added inline comments to D45842: [Reassociate] swap binop operands to increase factoring potential.
Tue, May 8, 9:30 AM
aprantl updated subscribers of D45842: [Reassociate] swap binop operands to increase factoring potential.
Tue, May 8, 9:29 AM
aprantl added a comment to D46568: [SimplifyCFG] Fix a crash when folding PHIs.

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.

Tue, May 8, 9:27 AM

Mon, May 7

aprantl added a comment to D46568: [SimplifyCFG] Fix a crash when folding PHIs.

That testcase is huge. Do all instructions need !dbg locations?

Mon, May 7, 5:38 PM

Thu, May 3

aprantl added a comment to D46362: DWARFExpression: Convert file addresses to load addresses early on.

As Greg suspected, the problem was that target was null. I rewrote that patch to not use a SymbolContext in the first place.

Thu, May 3, 5:00 PM
aprantl added a comment to D46362: DWARFExpression: Convert file addresses to load addresses early on.

Yes, I must have added a mistake in my last edit before landing this. I reverted that patch for now.

Thu, May 3, 1:24 PM
aprantl updated the diff for D46362: DWARFExpression: Convert file addresses to load addresses early on.

Thanks for the excellent feedback! Addressed comments.

Thu, May 3, 9:39 AM
aprantl added a comment to D46334: [CMake] Unify and relayer testing.

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.

Thu, May 3, 9:35 AM
aprantl added a comment to D46334: [CMake] Unify and relayer testing.

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.

Thu, May 3, 9:24 AM
aprantl added a comment to D46334: [CMake] Unify and relayer testing.

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

Thu, May 3, 8:57 AM
aprantl accepted D46391: [DebugInfo] Correction for an assert in DIExpression::createFragmentExpression.

So this is making the assertion even stricter. Thanks!

Thu, May 3, 8:39 AM
aprantl accepted D46384: Reapply "[SelectionDAG] Selection of DBG_VALUE using a PHI node result (pt 2)".
Thu, May 3, 8:34 AM

Wed, May 2

aprantl created D46362: DWARFExpression: Convert file addresses to load addresses early on.
Wed, May 2, 11:52 AM
aprantl added a reviewer for D46348: [SelectionDAG] Transfer DbgValues when casts are optimized in SelectionDAG::getNode: vsk.
Wed, May 2, 9:25 AM