Differential D125142
[clang][auto-init] Deprecate -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang Authored by kees on May 6 2022, 4:03 PM.
Details
GCC 12 has been released and contains unconditional support for Maintain compatibility with GCC, and remove the -enable flag for "zero"
Diff Detail
Event TimelineComment Actions It would be somewhat helpful as a transition aid if -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang remained as a no-op producing a warning (a generic unused argument warning would be fine). Comment Actions Ah, good point! I got fooled into thinking unknown enable options were silently ignored, but on a double-check I see I was mistaken. :) I will update this patch. Comment Actions Report flag as "unused" Adjust flags and tests to have the option warn about being unused now. Comment Actions Also, you should definitely add some release notes for this change.
Comment Actions It looks like clang still produces meaningful diagnostics from -Wuninitialized and -Wsometimes-uninitialized even with -ftrivial-auto-var-init=zero. Same for [clang-diagnostic-sometimes-uninitialized], [clang-analyzer-core.uninitialized.UndefReturn], and [clang-diagnostic-uninitialized]. Looks like GCC now even does a better job with -Wuninitialized on void init (int *x) {}
int x (int z) {
int y;
init(&y);
return y;
}
Comment Actions update with suggestions
Comment Actions Worth mentioning the deprecation plans in clang/docs/ReleaseNotes.rst and updaing the commit message+description on phab? Comment Actions This cannot be committed as is. In particular, @rsmith's "We do not want to create or encourage the creation of language dialects and non-portable code," concern on https://discourse.llvm.org/t/making-ftrivial-auto-var-init-zero-a-first-class-option/55143/2 (shared by someone else) will be affected, I'd like to see that they lift their concerns.
-ftrivial-auto-var-init=zero: https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#index-ftrivial-auto-var-init
I think this is insufficient justification. People's objection should be addressed (either they are now completely fine without the long option, or they accept the compromise) before proceeding. The summary needs to mention this point. Comment Actions I'd also like to hear from them, but at the time those comments were made, GCC did not have support for this feature. Now that GCC does, the dialect exists in the wild and we can either choose to support the dialect or not. Given our historical interest in GCC compatibility, I think we need to reweigh the comments in the RFC against the new reality.
Why? It's not been insufficient for many other things in Clang and we go out of our way to document that we want driver-level compatibility (https://clang.llvm.org/docs/DriverInternals.html#gcc-compatibility). Given that we already have this feature, I don't see justification for why we should be driver-compatible but not expose the functionality to users (that would be user-hostile).
I agree that we should still take the feedback from the RFC seriously. Also, if this is for GCC compatibility, I think we need to be very sure that our semantics match GCC's -- it would be even worse to expose the same flag but it initializes a different set of objects between the compilers. I'm not certain if we have the test coverage demonstrating that. Comment Actions FWIW, I think this concern was addressed back when -ftrivial-auto-var-init was added: uninitialized variables are considered UB, and the compiler would warn about them with -Wuninitialized. The coverage of -Wuninitialized was expressly not changed, so code will still warn about the uninitialized variable usage (but the actual contents of that variable is now at least constant). The point being, the dialect remains unchanged and the portability remains unchanged; it is only the safety of the resulting binary that is now improved. i.e. the UB still exists (the logic of the code may not match the content of the variable), but it's predictable now, instead of being potentially controllable by external factors (i.e. the prior stack contents, which may be controllable across privilege boundaries). Comment Actions I think the most relevant post from @rsmith is: https://discourse.llvm.org/t/making-ftrivial-auto-var-init-zero-a-first-class-option/55143/40 He has a prototype: https://reviews.llvm.org/D79249 That prototype would still diverge from the GCC option, which I hear isn't desirable. The fact remains that people have deployed zero init widely (you're likely running multiple zero-init codebases to read this), and they would not ever deploy zero-or-trap init. That's simply a fact. Richard had said:
If someone pursues Richard's patch above, then this would seem like a mutually agreeable resolution. Separately, we'd discussed narrowing the performance gap between pattern and zero-init, @Florian and team had done a bunch of work 2+ years ago, but I don't know how "done" we can claim that work is since I haven't been tracking the work. Comment Actions That's a different mode and doesn't seem to be relevant to the current situation and this change to drop the -enable flag.
Right.
Performance is the smaller part of why init=zero is being used so widely. It's about stability. Quoting from my earlier thread:
This "usually handled safely" state (e.g. existing NULL pointer checks) is specifically why Chrome OS switched from init=pattern to init=zero. Comment Actions Is there a community interest to have such feature in Clang? I consider it quite useful. If so, maybe I (we) could continue work on it, if @rsmith is busy. You mentioned the need for few fixes or few bugs, do you remember which ones? Comment Actions @rsmith can we get some guidance here? Has your opinion changed in the time since GCC has been shipping this? Comment Actions I agree with @rsmith that this is creating a language dialect, and it's creating one that at least WG21 has shown divided opinions on. So I think it is worth exploring the idea he had in https://reviews.llvm.org/D79249 in so much as that comes closer to meeting our bar for language extensions. However, I still think GCC compatibility is important in how we expose the functionality. If we spell the option -ftrivial-auto-var-init=zero, it should work the same as GCC (modulo bugs); the alternative would cause too much confusion in the field, I think. That said, there's significant deployment experience with the zeroing flag, and some committees (like WG14) honor prior art when considering standardization. That there's now one very popular C implementation exposing this functionality for users makes it slightly more likely WG14 would be interested in standardizing something in this realm. Having a second very popular C implementation further strengthens that case. So while WG21 has shown divided opinions, WG14 hasn't been consulted. Putting a paper in front of them that shows some signs of life (which we could do already today without any changes here) would remove the concerns regarding meeting our bar for an extension if we wanted the same semantics as GCC. However, I'm still skeptical of WG14 adopting such a proposal, so it's by no means a slam dunk. Despite that and given the success of the feature in GCC, I'm less concerned about the creation of a language dialect in this particular case. We've done that before (many of our floating-point math flags like -ffast-math and -fhonor-nans come to mind) and we have sufficient evidence that users are making use of -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang: https://sourcegraph.com/search?q=context:global+enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang+-file:.*test.*+-file:Options.td&patternType=standard tl;dr: I think it's defensible to move forward with this patch as-is, especially given that we've been threatening to remove the -cc1 flag (we're nearing a point where we either need to support this feature or drop it and I think people would strongly prefer we support it). Comment Actions Thanks for all the careful consideration; I appreciate it! I'll land this tomorrow unless there are new specific objections. :)
Comment Actions
The subject should be updated to mention the exact option name, not a vague -enable and "zero" mode
Comment Actions Add the option to Group<clang_ignored_legacy_options_Group> instead of adding a custom diagnostic code. Comment Actions use Group<clang_ignored_legacy_options_Group> Instead of a custom warning, use the clang_ignored_legacy_options_Group, as MaskRay suggested. Comment Actions Update optional removal target release to Clang 18 2 releases was the suggested time to wait between deprecation and removal
| ||||||||||||||||||||||||||||||||||||||||||||||