[PATCH] D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule.

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

In https://reviews.llvm.org/D38042#875734, @chandlerc wrote:

> In https://reviews.llvm.org/D38042#875441, @aprantl wrote:
>
> > In https://reviews.llvm.org/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.


I like the idea of making debug info stripping a part of the auto upgrade process, but in order to do this, we would need to run the verifier as part of the auto upgrade process (probably inside `llvm::UpgradeDebugInfo()`) in order to determine that stripping is necessary. Do you see any problems with that? I guess as long as we wire up clang's --disable-llvm-verifier option to the bitcode reader we can still get the current behavior if somebody really wants that.

I will explore this idea some more. Thanks for your input so far!


https://reviews.llvm.org/D38042