aprantl (Adrian Prantl)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 2 2013, 8:12 AM (238 w, 1 d)

Recent Activity

Fri, Sep 22

aprantl created D38184: Move the stripping of malformed debug info from the Verifier to AutoUpgrade..
Fri, Sep 22, 11:15 AM

Thu, Sep 21

aprantl added reviewers for D38142: FreeBSD kernel debugging fixes: jingham, clayborg, jasonmolenda.
Thu, Sep 21, 10:08 AM
aprantl accepted D37740: [SelectionDAG] Pick correct frame index in LowerArguments.
Thu, Sep 21, 10:07 AM
aprantl added a comment to D37740: [SelectionDAG] Pick correct frame index in LowerArguments.

Seems ok to me.
@rnk: was your concern addressed?

Thu, Sep 21, 9:41 AM
aprantl added a reviewer for D38135: [dwarfdump] Skip 'stripped' sections: enderby.
Thu, Sep 21, 9:38 AM
aprantl added inline comments to D38135: [dwarfdump] Skip 'stripped' sections.
Thu, Sep 21, 9:37 AM
aprantl accepted D38125: [dwarfdump] Add support for redirecting output to a file.

LGTM with inline comments addressed.
Thanks!

Thu, Sep 21, 9:00 AM

Wed, Sep 20

aprantl updated the diff for D36627: dwarfdump: Add an option to collect debug info quality metrics.

Rebased and addressed additional review feedback.

Wed, Sep 20, 3:22 PM
aprantl accepted D38068: [DebugInfo] Use a DenseMap to coalesce MachineOperand locations.

Testcase is the same as in the original commit?

Wed, Sep 20, 10:13 AM

Tue, Sep 19

aprantl created D38064: llvm-dwarfdump: implement --recurse-depth=<N>.
Tue, Sep 19, 5:01 PM
aprantl added a comment to D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule..

Absolutely. I think the verifier should never, under any circumstances, mutate the IR. Think about it, with the current design if a pass corrupts the debug info the verifier may "hide" this by stripping it out rather than allowing us to find it.

Okay, I think I agree. Before I venture off implementing this, do you think that separating out a StripBrokenDebugInfoPass that depends on the Verifier is right way forward?

I don't know what you mean by "depends on" and I'm not sure I'm the right person to say what the exact best design is... But I'd throw something together and start that review on llvm-commits where we can get folks more familiar w/ these details involved to figure it out. It at least seems to be in the right direction.

My only concern is that passes are supposed to be able to rely on the verifier passing, and this wouldn't... Not sure how to handle that case.

But auto-upgrade happens on *read*. If you want to add the debug info stripping to auto-upgrade, that's a reasonable discussion to have, and no change here will be required. There might be concerns about that on the LLVM side, I don't know. But the verifier (as well as running it here) does not seem like the right solution.

Would splitting the VerifierPass into a VerifierPass and a StripBrokenDebugInfoPass *together* with adding a -enable-llvm-verifier (an explicit opposite of -disable-llvm-verifier) work for you?

If you want a way to use the clang binary to strip broken debug info from a bitcode input, I would add a flag that does exactly this, and leave the verifier as an independent component. I don't know whether such a flag makes sense or not (Richard or other more deep in Clang-land would have a better feel than I would).

But I think that whether the verifier is enabled or not should be orthogonal from any debug info stripping. Stripping the debug info might impact whether something verifies, but the flags should be completely independent.

However, if the debug info stripping ends up as part of auto upgrade, all of this becomes much simpler to think about.

Tue, Sep 19, 4:27 PM
aprantl accepted D37768: [IR] Add llvm.dbg.addr, a control-dependent version of llvm.dbg.declare.

Couple of (stylistic) inline comments, but otherwise looking good!

Tue, Sep 19, 3:17 PM
aprantl added inline comments to D37768: [IR] Add llvm.dbg.addr, a control-dependent version of llvm.dbg.declare.
Tue, Sep 19, 3:17 PM
aprantl added a comment to D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule..

But, the verifier itself will just "crash". It won't print a stack trace, but I don't see why that's much better? And this flag is supposed to be a developer option and not a user facing one, so I'm somewhat confused at what the intent is here...

No, it will report_fatal_error() instead of crashing the compiler later on.
In any case, that is not my primary motivation: The intent is exactly what is illustrated by the testcase, stripping malformed debug info metadata produced by older, buggy versions of clang. The backstory to this is that historically the Verifier was very weak when it came to verifying debug info. To allow us to make the Verifier better (stricter), while still supporting importing of older .bc files produced by older compilers, we added a mechanism that strips all debug info metadata if the verification of the debug info failed, but the bitcode is otherwise correct.

Ok, that use case makes way more sense. I'd replace the change description with some discussion of this use case.

My next question is -- why is this done by the verifier? It seems *really* bad that the verifier *changes the IR*. Don't get me wrong, what you're trying to do (strip malformed debug info) makes perfect sense. But I think that the thing which does that shouldn't be called a verifier. It might *use* the verifier of course.

That was a purely pragmatic decision: Most (but not all) LLVM-based tools are running the Verifier as an LLVM pass so adding the stripping into the pass was the least invasive way of implementing this feature. If we are truly bothered by this, I think what could work is to separate out a second StripBrokenDebugInfo pass that depends on the Verifier and is guaranteed to run immediately after it. I don't see this adding much value though, and we would have to modify all tools to schedule the new pass explicitly. Do you think that would be worth pursuing?

Absolutely. I think the verifier should never, under any circumstances, mutate the IR. Think about it, with the current design if a pass corrupts the debug info the verifier may "hide" this by stripping it out rather than allowing us to find it.

Tue, Sep 19, 12:01 PM
aprantl added a comment to D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule..

But, the verifier itself will just "crash". It won't print a stack trace, but I don't see why that's much better? And this flag is supposed to be a developer option and not a user facing one, so I'm somewhat confused at what the intent is here...

No, it will report_fatal_error() instead of crashing the compiler later on.
In any case, that is not my primary motivation: The intent is exactly what is illustrated by the testcase, stripping malformed debug info metadata produced by older, buggy versions of clang. The backstory to this is that historically the Verifier was very weak when it came to verifying debug info. To allow us to make the Verifier better (stricter), while still supporting importing of older .bc files produced by older compilers, we added a mechanism that strips all debug info metadata if the verification of the debug info failed, but the bitcode is otherwise correct.

Ok, that use case makes way more sense. I'd replace the change description with some discussion of this use case.

My next question is -- why is this done by the verifier? It seems *really* bad that the verifier *changes the IR*. Don't get me wrong, what you're trying to do (strip malformed debug info) makes perfect sense. But I think that the thing which does that shouldn't be called a verifier. It might *use* the verifier of course.

That was a purely pragmatic decision: Most (but not all) LLVM-based tools are running the Verifier as an LLVM pass so adding the stripping into the pass was the least invasive way of implementing this feature. If we are truly bothered by this, I think what could work is to separate out a second StripBrokenDebugInfo pass that depends on the Verifier and is guaranteed to run immediately after it. I don't see this adding much value though, and we would have to modify all tools to schedule the new pass explicitly. Do you think that would be worth pursuing?

Last but not least, I still suspect that this shouldn't be run here. If the user wants to disable LLVM passes *and emits LLVM IR*, they should get it unperturbed. The stripping of malformed debug info seems like it should happen later as part of the passes to emit code, and I'd actually expect the LLVM code generator to add the necessary pass rather than relying on every frontend remembering to do so?

The user wants to disable LLVM optimizations (-disable-llvm-optzns) not LLVM passes.

Tue, Sep 19, 11:47 AM
aprantl added a comment to D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule..

But, the verifier itself will just "crash". It won't print a stack trace, but I don't see why that's much better? And this flag is supposed to be a developer option and not a user facing one, so I'm somewhat confused at what the intent is here...

No, it will report_fatal_error() instead of crashing the compiler later on.
In any case, that is not my primary motivation: The intent is exactly what is illustrated by the testcase, stripping malformed debug info metadata produced by older, buggy versions of clang. The backstory to this is that historically the Verifier was very weak when it came to verifying debug info. To allow us to make the Verifier better (stricter), while still supporting importing of older .bc files produced by older compilers, we added a mechanism that strips all debug info metadata if the verification of the debug info failed, but the bitcode is otherwise correct.

Ok, that use case makes way more sense. I'd replace the change description with some discussion of this use case.

My next question is -- why is this done by the verifier? It seems *really* bad that the verifier *changes the IR*. Don't get me wrong, what you're trying to do (strip malformed debug info) makes perfect sense. But I think that the thing which does that shouldn't be called a verifier. It might *use* the verifier of course.

Tue, Sep 19, 11:34 AM
aprantl added a comment to D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule..

But, the verifier itself will just "crash". It won't print a stack trace, but I don't see why that's much better? And this flag is supposed to be a developer option and not a user facing one, so I'm somewhat confused at what the intent is here...

Tue, Sep 19, 10:41 AM
aprantl created D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule..
Tue, Sep 19, 9:35 AM
aprantl added a comment to D37971: [dwarfdump] Add verbose output for .debug-line section.

Did you accidentally upload the wrong patch?

Tue, Sep 19, 8:55 AM
aprantl added a comment to D37877: Update getMergedLocation to check the instruction type and merge properly..

Looks mostly good now. One more inline comment.

Tue, Sep 19, 8:52 AM

Mon, Sep 18

aprantl added inline comments to D37877: Update getMergedLocation to check the instruction type and merge properly..
Mon, Sep 18, 1:53 PM
aprantl added a comment to D37877: Update getMergedLocation to check the instruction type and merge properly..

I think this works for me.

Mon, Sep 18, 1:52 PM
aprantl added a comment to D37971: [dwarfdump] Add verbose output for .debug-line section.

Looks like I just typed up everything that David said :-)

Mon, Sep 18, 8:56 AM
aprantl added a comment to D37971: [dwarfdump] Add verbose output for .debug-line section.

Added a couple more inline comments.

Mon, Sep 18, 8:55 AM
aprantl added a comment to D37971: [dwarfdump] Add verbose output for .debug-line section.

Doing this during parsing doesn't seem too intrusive and looks like it is the right trade-off.
Do you have a test case, too?

Mon, Sep 18, 7:59 AM

Fri, Sep 15

aprantl added a comment to D37929: [DebugInfo] Add missing DW_OP_deref when an NRVO pointer is spilled.

Awesome.

Fri, Sep 15, 2:45 PM
aprantl added a comment to D37929: [DebugInfo] Add missing DW_OP_deref when an NRVO pointer is spilled.

Do you think you could craft a debuginfo-tests test from PR34513, so we can avoid regressing in the future?

Fri, Sep 15, 2:41 PM
aprantl accepted D37911: [DebugInfo] Insert DW_OP_deref when spilling indirect DBG_VALUEs.

Thanks! Are you running the debuginfo-tests on your changes prior to committing?

Fri, Sep 15, 2:39 PM
aprantl accepted D37929: [DebugInfo] Add missing DW_OP_deref when an NRVO pointer is spilled.
Fri, Sep 15, 2:34 PM
aprantl added inline comments to D37911: [DebugInfo] Insert DW_OP_deref when spilling indirect DBG_VALUEs.
Fri, Sep 15, 2:25 PM
aprantl added inline comments to D37911: [DebugInfo] Insert DW_OP_deref when spilling indirect DBG_VALUEs.
Fri, Sep 15, 1:23 PM
aprantl accepted D37852: [dwarfdump] Make .eh_frame an alias for .debug_frame.
Fri, Sep 15, 8:06 AM
aprantl added a comment to D37877: Update getMergedLocation to check the instruction type and merge properly..

Just wanted to say that I like the direction this is going and LGTM from my side once you sorted out the details David mentioned.

Fri, Sep 15, 8:05 AM

Thu, Sep 14

aprantl added inline comments to D37852: [dwarfdump] Make .eh_frame an alias for .debug_frame.
Thu, Sep 14, 4:33 PM
aprantl added a comment to D37852: [dwarfdump] Make .eh_frame an alias for .debug_frame.

My impression is that .eh_frame and .debug_frame are interchangeable and in fact you get only one or the other in any given compilation. So, if you ask for .debug_frame and the object has .eh_frame, I think we should still dump it; and vice versa.

Thu, Sep 14, 2:48 PM
aprantl added inline comments to D37852: [dwarfdump] Make .eh_frame an alias for .debug_frame.
Thu, Sep 14, 1:30 PM
aprantl requested changes to D37852: [dwarfdump] Make .eh_frame an alias for .debug_frame.

Sorry, I accidentally marked this as accepted.

Thu, Sep 14, 1:30 PM
aprantl accepted D37852: [dwarfdump] Make .eh_frame an alias for .debug_frame.
Thu, Sep 14, 1:11 PM

Wed, Sep 13

aprantl updated the diff for D37771: llvm-dwarfdump: automatically dump both regular and .dwo variant of sections.

Implement David's suggestion to skip empty non-dwo section headers in .dwo files.

Wed, Sep 13, 2:47 PM
aprantl added inline comments to D37771: llvm-dwarfdump: automatically dump both regular and .dwo variant of sections.
Wed, Sep 13, 2:40 PM
aprantl updated the diff for D37771: llvm-dwarfdump: automatically dump both regular and .dwo variant of sections.

Clarify comment.

Wed, Sep 13, 2:36 PM
aprantl added inline comments to D37771: llvm-dwarfdump: automatically dump both regular and .dwo variant of sections.
Wed, Sep 13, 2:35 PM
aprantl updated the diff for D37771: llvm-dwarfdump: automatically dump both regular and .dwo variant of sections.

use sys::path instead

Wed, Sep 13, 2:27 PM
aprantl updated the diff for D37771: llvm-dwarfdump: automatically dump both regular and .dwo variant of sections.

Now comes with ROCK-SOLID .dwo detection!

Wed, Sep 13, 2:19 PM
aprantl updated the diff for D37771: llvm-dwarfdump: automatically dump both regular and .dwo variant of sections.

With this update:

Wed, Sep 13, 1:58 PM

Tue, Sep 12

aprantl added a comment to D37771: llvm-dwarfdump: automatically dump both regular and .dwo variant of sections.

Okay, I'll implement a "don't print anything if the section doesn't exist" approach, since that seems to align best with what everybody desires. Printing an empty "contents" only if the section was explicitly required is trickier to implement, so I'll leave this for a future improvement.

Tue, Sep 12, 4:13 PM
aprantl accepted D36596: [InstCombine] Add a flag to disable LowerDbgDeclare.

With the FIXME in the code this LGTM since it leaves no doubt that we want to migrate away from it.

Tue, Sep 12, 3:36 PM
aprantl added a comment to D37771: llvm-dwarfdump: automatically dump both regular and .dwo variant of sections.

When dumping a .o file without a .debug_types section, should --debug-types

  1. print nothing
  2. print .debug_types:\nEOF
Tue, Sep 12, 2:47 PM
aprantl added inline comments to D37771: llvm-dwarfdump: automatically dump both regular and .dwo variant of sections.
Tue, Sep 12, 2:31 PM
aprantl updated the diff for D37771: llvm-dwarfdump: automatically dump both regular and .dwo variant of sections.

Address review feedback.

Tue, Sep 12, 2:31 PM
aprantl accepted D37439: [MachO] Prevent heap overflow when load command extends past EOF.

Okay, thanks for the explanation, Kevin!

Tue, Sep 12, 2:24 PM
aprantl added inline comments to D37768: [IR] Add llvm.dbg.addr, a control-dependent version of llvm.dbg.declare.
Tue, Sep 12, 2:16 PM
aprantl added inline comments to D37768: [IR] Add llvm.dbg.addr, a control-dependent version of llvm.dbg.declare.
Tue, Sep 12, 2:16 PM
aprantl created D37771: llvm-dwarfdump: automatically dump both regular and .dwo variant of sections.
Tue, Sep 12, 1:56 PM
aprantl accepted D37617: Debug info for variables whose type is shrinked to bool fix.

thanks!

Tue, Sep 12, 1:08 PM
aprantl added inline comments to D37617: Debug info for variables whose type is shrinked to bool fix.
Tue, Sep 12, 8:55 AM
aprantl accepted D37745: [dwarfdump] Rename Brief to Verbose in DIDumpOptions.

Thanks!

Tue, Sep 12, 8:53 AM
aprantl accepted D37744: [dwarfdump][NFC] Move DebugFrameEntry classes to header..
Tue, Sep 12, 8:52 AM
aprantl added inline comments to D37740: [SelectionDAG] Pick correct frame index in LowerArguments.
Tue, Sep 12, 8:51 AM
aprantl added inline comments to D37439: [MachO] Prevent heap overflow when load command extends past EOF.
Tue, Sep 12, 8:46 AM

Mon, Sep 11

aprantl added a comment to D37714: llvm-dwarfdump: Replace -debug-dump=sect option with individual options..

I would kind of like one option to dump whichever of .debug_info or .debug_info.dwo (etc) there is, so I don't have to know up front which section to be asking for. Would also be more user-friendly to people who kind of know DWARF but not at that level of detail.

Mon, Sep 11, 4:10 PM
aprantl created D37717: llvm-dwarfdump: Make -brief the default and add -verbose instead..
Mon, Sep 11, 3:44 PM
aprantl created D37714: llvm-dwarfdump: Replace -debug-dump=sect option with individual options..
Mon, Sep 11, 2:56 PM
aprantl added a comment to D37696: [dwarfdump] Add DWARF verifiers for address ranges.

Seems generally good. Since David had a lot of comments on the original review, I'll defer to him to accept this.

Mon, Sep 11, 11:29 AM
aprantl added inline comments to D37625: [DWARF] Incorrect prologue end line record..
Mon, Sep 11, 8:30 AM

Fri, Sep 8

aprantl added a comment to D36993: [llvm-dwarfdump] Print type names in DW_AT_type DIEs.

I played around with how we could generate testcases for this.

Fri, Sep 8, 5:08 PM
aprantl added inline comments to D37511: [dwarfdump] Verify line table prologue.
Fri, Sep 8, 2:49 PM
aprantl added a comment to D37602: Properly hook debuginfo-tests up to lit and CMake.

I think this addresses most of the concerns raised so far.

Fri, Sep 8, 1:47 PM

Thu, Sep 7

aprantl added inline comments to D37602: Properly hook debuginfo-tests up to lit and CMake.
Thu, Sep 7, 5:19 PM
aprantl added inline comments to D37602: Properly hook debuginfo-tests up to lit and CMake.
Thu, Sep 7, 5:18 PM
aprantl accepted D37604: Disable debuginfo-tests for non-native configurations.

This seems reasonable to me, thanks!
When you commit this, could you please double-check that the tests are still running on the green dragon builders? I'll also keep an eye on them.

Thu, Sep 7, 4:50 PM
aprantl added inline comments to D37602: Properly hook debuginfo-tests up to lit and CMake.
Thu, Sep 7, 4:43 PM
aprantl added a comment to D37602: Properly hook debuginfo-tests up to lit and CMake.

Currently the assumption is that you clone it under the clang/test folder, but the fact that it needs to depend on lld means that clang isn't really the best place for it. Besides, this is already broken in the mono-repo unless you manually symlink it from clang/test which seems odd.

Thu, Sep 7, 4:42 PM
aprantl added a comment to D36596: [InstCombine] Add a flag to disable LowerDbgDeclare.

And we should also stake out a clear path to what is necessary to make this patch obsolete and document what is missing.

Thu, Sep 7, 3:55 PM
aprantl added a comment to D36596: [InstCombine] Add a flag to disable LowerDbgDeclare.

Ok. I think there should be a PR to revert this change that is blocked on PR34136. The description of the option should explicitly spell out a big warning that this will introduce false positives and that it is experimental/temporary only. We should aim to get this reverted as soon as possible.

Thu, Sep 7, 3:52 PM
aprantl added a comment to D36596: [InstCombine] Add a flag to disable LowerDbgDeclare.

Is this obsoleted by the dbg.addr proposal or orthogonal?

Thu, Sep 7, 2:58 PM
aprantl added a reviewer for D37439: [MachO] Prevent heap overflow when load command extends past EOF: enderby.
Thu, Sep 7, 2:52 PM
aprantl accepted D37511: [dwarfdump] Verify line table prologue.
Thu, Sep 7, 2:13 PM

Fri, Sep 1

aprantl added a comment to D37390: [diagtool] Change default tree behavior to print only flags.

Can this be tested?

Fri, Sep 1, 2:29 PM ยท Restricted Project
aprantl added a comment to D36993: [llvm-dwarfdump] Print type names in DW_AT_type DIEs.

David, apologies for missing your e-mail. I really hate that it doesn't automatically show up in Phabricator! ๐Ÿ™

If the tag doesn't have a name attribute, everything will go through this function except: DW_TAG_pointer_type, DW_TAG_ptr_to_member_type, DW_TAG_reference_type, DW_TAG_rvalue_reference_type. The first part explains why class and struct don't show up. I prefer this approach because it's guaranteed to be robust. Every DW_TAG_*_type encountered without a name will have something meaningful printed.

IIRC, the original switch had between 20 and 25 cases.

I'm curious what those 20-25 cases were - do you have a copy/roughly describe their contents? Because while 'const' does print nicely, (& volatile would be similar) I'm not sure what the other 10 or so cases might be and whether that's a reasonable way to print them.

Here's the list of cases I had originally:

case DW_TAG_array_type:
case DW_TAG_base_type:
case DW_TAG_class_type:
case DW_TAG_const_type:
case DW_TAG_enumeration_type:
case DW_TAG_file_type:
case DW_TAG_interface_type:
case DW_TAG_packed_type:
case DW_TAG_pointer_type:
case DW_TAG_ptr_to_member_type:
case DW_TAG_reference_type:
case DW_TAG_restrict_type:
case DW_TAG_set_type:
case DW_TAG_shared_type:
case DW_TAG_string_type
case DW_TAG_structure_type:
case DW_TAG_subrange_type:
case DW_TAG_subroutine_type:
case DW_TAG_thrown_type:
case DW_TAG_union_type:
case DW_TAG_unspecified_type:
case DW_TAG_volatile_type:

Ah, thanks!

I feel like maybe this should be examined more closely (an example of how each of these would be printed would be ideal, though that might be a bit much) - for example I don't think it makes sense to print out subroutine types like "int subroutine" (rather than "int(float, double)", say) which I /think/ is how they might look based on the current code)

Fri, Sep 1, 1:24 PM

Thu, Aug 31

aprantl added a comment to D37344: Fix debuginfo-tests with GDB on Linux.

Yes, looks like it is still running, it!
http://green.lab.llvm.org/green/job/clang-stage1-configure-RA_check/35057/consoleFull

Thu, Aug 31, 1:11 PM
aprantl accepted D37344: Fix debuginfo-tests with GDB on Linux.

LGTM with outstanding issues addressed.

Thu, Aug 31, 11:10 AM
aprantl added a comment to D37344: Fix debuginfo-tests with GDB on Linux.

Looks good except for the one inline comment.

Thu, Aug 31, 11:09 AM
aprantl abandoned D35923: Fix PR32332 - PCM nondeterminism with builtins caused by global module index .
Thu, Aug 31, 10:53 AM
aprantl resigned from D37338: [LoopUnroll][DebugInfo] Don't add metadata to unrolled remainder loop.

I don't understand enough about loop annotations to review this.

Thu, Aug 31, 10:49 AM
aprantl added a reviewer for D37338: [LoopUnroll][DebugInfo] Don't add metadata to unrolled remainder loop: anemet.
Thu, Aug 31, 10:49 AM
aprantl added a comment to D37311: [DebugInfo] Lower dbg.declare to DBG_VALUE with DW_OP_deref.

Okay, thanks for the explanation. In order to make this change our highest priority should be that the contract between Clang and LLVM still works.
Secondarily, we should make an effort to ensure that LLVM IR testcases that include source code actually reflect the contract and can be reproduced by recompiling the original source with clang. If the IR changes after a patch, it may make sense to clone the test so we get both coverage of the original codepath in the backend and coverage/documentation of how frontends are expected to behave.

Thu, Aug 31, 9:24 AM
aprantl accepted D37334: [llvm-dwarfdump] Brief mode only dumps debug_info by default.
Thu, Aug 31, 9:03 AM

Wed, Aug 30

aprantl accepted D37316: REQUIRES: x86_64-linux -> REQUIRES: shell.

Seems reasonable, thanks!

Wed, Aug 30, 7:28 PM
aprantl added a comment to D37315: Fix test after rL312144.

Shouldn't REQUIRES: shell be sufficient for sed?

Wed, Aug 30, 2:57 PM
aprantl added a comment to D37315: Fix test after rL312144.

Why does this test REQUIRE x86_64-linux?

Wed, Aug 30, 2:56 PM
aprantl accepted D37315: Fix test after rL312144.
Wed, Aug 30, 2:55 PM
aprantl requested changes to D37311: [DebugInfo] Lower dbg.declare to DBG_VALUE with DW_OP_deref.
Wed, Aug 30, 2:52 PM
aprantl added a comment to D37311: [DebugInfo] Lower dbg.declare to DBG_VALUE with DW_OP_deref.

As discussed on IRC, I think that this is *generally* a good idea, but I'm skeptical about a few issues with the concrete implementation that I commented on inline.
These changes have to be made very carefully. Have you tried running the debuginfo-tests with your patch? To do this you need to checkout the debuginfo-tests repository into the clang/test directory and have either gdb or lldb installed and working (yes it's magical and works with both!).

Wed, Aug 30, 2:51 PM

Tue, Aug 29

aprantl added a reviewer for D37038: Replace temp MD nodes with unique/distinct before cloning: dblaikie.
Tue, Aug 29, 11:14 AM
aprantl added a comment to D37038: Replace temp MD nodes with unique/distinct before cloning.

This may have gotten lost earlier: Would it be possible to instruct CloneFunction to not clone any temporary MDNodes via one of the flags that are passed to the ValueMapper?

Tue, Aug 29, 10:09 AM
aprantl added a comment to D37123: [dwarfdump] Pretty print location expressions and location lists.

Awesome, I can't wait for this to land!

Tue, Aug 29, 8:38 AM

Mon, Aug 28

aprantl added inline comments to D37038: Replace temp MD nodes with unique/distinct before cloning.
Mon, Aug 28, 11:48 AM
aprantl added a comment to D37123: [dwarfdump] Pretty print location expressions and location lists.
In D37123#854201, @rnk wrote:

I think the remaining concerns are:

  1. Should we split the location list base address change into another patch

Typically the answer to the question whether we should split up a patch is always yes, but I'm fine either way.

Mon, Aug 28, 11:43 AM
aprantl added a comment to D35994: Debug info for variables whose type is shrinked to bool.

In fully-compliant mode we should just not emit any DWARF expression that ends with a DW_OP_stack_value in DWARF < 4. However, LLDB and GDB (at least the version Apple used to ship in Xcode) support a couple of expressions that are outside of the specification but do work in practice, so I'm reluctant to drop support for that by being stricter.
That said, Darwin has been using DWARF 4 for a while now, so an argument could be made that it is okay to regress on older deployment targets for the sake of correctness/standards compliance.

Mon, Aug 28, 10:01 AM