aprantl added a comment. In https://reviews.llvm.org/D38042#875418, @chandlerc wrote:
> 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. 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? >> >> >>> 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. 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? https://reviews.llvm.org/D38042 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
