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

LGTM with inline comments addressed.
Thanks!

Rebased and addressed additional review feedback.

Testcase is the same as in the original commit?

In D38042#875734, @chandlerc wrote:

In D38042#875441, @aprantl wrote:

In D38042#875418, @chandlerc wrote:

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.

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

In D38042#875418, @chandlerc wrote:

In D38042#875412, @aprantl wrote:

In D38042#875334, @chandlerc wrote:

In D38042#875328, @aprantl wrote:

In D38042#875303, @chandlerc wrote:

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.

In D38042#875412, @aprantl wrote:

In D38042#875334, @chandlerc wrote:

In D38042#875328, @aprantl wrote:

In D38042#875303, @chandlerc wrote:

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.

In D38042#875334, @chandlerc wrote:

In D38042#875328, @aprantl wrote:

In D38042#875303, @chandlerc wrote:

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.

In D38042#875303, @chandlerc wrote:

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

Did you accidentally upload the wrong patch?

Looks mostly good now. One more inline comment.

I think this works for me.

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

Added a couple more inline comments.

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?

Awesome.

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

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

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.

In D37852#871076, @probinson wrote:

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.

Sorry, I accidentally marked this as accepted.

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

Clarify comment.

use sys::path instead

Now comes with ROCK-SOLID .dwo detection!

With this update:

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.

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

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

1. print nothing
2. print .debug_types:\nEOF

Address review feedback.

Okay, thanks for the explanation, Kevin!

thanks!

Thanks!

In D37714#867252, @probinson wrote:

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.

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

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

In D37602#865266, @zturner wrote:

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

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.

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.

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

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.

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

Can this be tested?

In D36993#859034, @dblaikie wrote:

In D36993#858768, @JDevlieghere wrote:

In D36993#858121, @dblaikie wrote:

In D36993#858093, @JDevlieghere wrote:

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)

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

LGTM with outstanding issues addressed.

Looks good except for the one inline comment.

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

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.

Seems reasonable, thanks!

Shouldn't REQUIRES: shell be sufficient for sed?

Why does this test REQUIRE x86_64-linux?

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

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?

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

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.

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.