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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
