aprantl 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? > > > 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. It was just pointed out that `-disable-llvm-optzns` is now a deprecated alias for `-disable-llvm-passes`, so I see that this argument makes less sense today. Do you think that `-disable-llvm-passes` should imply `-disable-llvm-verifier`? Is it obvious to users the the Verifier is a pass? If yes, how about adding a positive version of `-disable-llvm-verifier`? > 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. 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. https://reviews.llvm.org/D38042 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
