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

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D38042#875412, @aprantl wrote:

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

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

(sorry for the off-list duplication, but it belongs here anyways)

I disagree. `-disable-llvm-optzns` is a developer flag, and was almost an alias for `-disable-llvm-passes`. After discussion on the list we made it an actual legacy and deprecated alias for `-disable-llvm-passes` because the latter was more clear, understandable, and correct. We had cases where the passes run by `-disable-llvm-optzns` were actually problematic and we wanted to debug them but were unable to do so.

> Also, I believe the Verifier is special. The Verifier protects LLVM from crashing and as a user I think I would prefer a Verifier error over clang crashing while emitting bitcode.

I think this distinction is not a meaningful one ultimately. And the verifier should *never* be a functional requirement because it should have no effect. I'm happy for us to verify when we read input, but even then it should not mutate the IR.

> Because of auto-upgrades users already don't get the IR unperturbed, and I view stripping of broken debug info as a (in all fairness: very brutal :-) auto-upgrade.

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.


https://reviews.llvm.org/D38042