This is an archive of the discontinued LLVM Phabricator instance.

[clang][auto-init] Deprecate -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang
ClosedPublic

Authored by kees on May 6 2022, 4:03 PM.

Details

Summary

GCC 12 has been released and contains unconditional support for
-ftrivial-auto-var-init=zero:
https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#index-ftrivial-auto-var-init

Maintain compatibility with GCC, and remove the -enable flag for "zero"
mode. The flag is left to generate an "unused" warning, though, to not
break all the existing users. The flag will be fully removed in Clang 17.

Link: https://github.com/llvm/llvm-project/issues/44842

Diff Detail

Event Timeline

kees created this revision.May 6 2022, 4:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2022, 4:03 PM
kees requested review of this revision.May 6 2022, 4:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2022, 4:03 PM

Strong +1.

It would be great to backport it to llvm 14.0.x as well.

brooks added a subscriber: brooks.May 7 2022, 11:39 AM

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

kees planned changes to this revision.May 7 2022, 11:03 PM

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

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.

kees updated this revision to Diff 427916.May 8 2022, 2:12 AM

Report flag as "unused"

Adjust flags and tests to have the option warn about being unused now.

Also, you should definitely add some release notes for this change.

clang/include/clang/Driver/Options.td
2799–2800

We might as well comment on when we expect to remove it; I took a stab in the dark and figured two releases of deprecation should be enough for most folks (so it's deprecated in Clang 15 and 16, then removed in Clang 17).

I don't think we want this option to be ignored though; I think we still want to accept the option, but then be loud about it being deprecated when translating it to -ftrivial-auto-var-init=zero. You can emit diag::warn_drv_deprecated_arg for the diagnostic.

nickdesaulniers accepted this revision.May 9 2022, 10:02 AM

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;
}
clang/test/Driver/clang_f_opts.c
573

consider breaking long run lines into 2 with \. Example:

// RUN: %clang -### ... -S \
// RUN:   -ftrivial-auto-var-init=zero ...
This revision is now accepted and ready to land.May 9 2022, 10:02 AM
kees planned changes to this revision.May 9 2022, 12:40 PM
kees marked 2 inline comments as done.
kees updated this revision to Diff 428173.May 9 2022, 12:47 PM

update with suggestions

  • Add FIXME comment with deprecation schedule
  • Report deprecation warning when -enable flag is used
This revision is now accepted and ready to land.May 9 2022, 12:47 PM
nickdesaulniers accepted this revision.May 9 2022, 12:52 PM

Worth mentioning the deprecation plans in clang/docs/ReleaseNotes.rst and updaing the commit message+description on phab?

kees updated this revision to Diff 428224.May 9 2022, 3:42 PM
kees edited the summary of this revision. (Show Details)

add release notes

MaskRay added reviewers: Restricted Project, rsmith.May 10 2022, 12:21 AM
MaskRay added a comment.EditedMay 10 2022, 12:24 AM

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.

GCC 12 has been released and contains unconditional support for

-ftrivial-auto-var-init=zero: https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#index-ftrivial-auto-var-init

Maintain compatibility with GCC, and remove the -enable flag for "zero" mode. The flag is left to generate an "unused" warning, though, to not break all the existing users. The flag will be fully removed in Clang 17.

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.

MaskRay requested changes to this revision.May 10 2022, 12:26 AM
This revision now requires changes to proceed.May 10 2022, 12:26 AM

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.

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.

GCC 12 has been released and contains unconditional support for

-ftrivial-auto-var-init=zero: https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#index-ftrivial-auto-var-init

Maintain compatibility with GCC, and remove the -enable flag for "zero" mode. The flag is left to generate an "unused" warning, though, to not break all the existing users. The flag will be fully removed in Clang 17.

I think this is insufficient justification.

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

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.

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.

kees added a comment.May 10 2022, 10:52 AM

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.

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

kees added a comment.May 10 2022, 3:14 PM

This is marked "needs revision", but I think it just needs wider review?

jfb added a subscriber: Florian.May 11 2022, 4:18 AM

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
I assume he would like someone to pursue it further, it was a good faith attempt at not just demanding. I'd played with it and it needed a few fixes, but overall it was pretty complete. Does someone want to give it a go?

That prototype would still diverge from the GCC option, which I hear isn't desirable.
The discussions on standardizing this are perpetually stalled whenever they come up, so it's not an avenue that I ever expect to turn positive.

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 we want a separate flag to control whether / how much we use such a trapping mode, I think that could be reasonable, subject to having a good answer to the language dialect concern I expressed previously (eg, maybe there’s never a guarantee that we won’t insert a trap, even though with the flag on its lowest setting that is actually what happens).

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.

kees added a comment.May 11 2022, 9:59 AM

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
I assume he would like someone to pursue it further, it was a good faith attempt at not just demanding. I'd played with it and it needed a few fixes, but overall it was pretty complete. Does someone want to give it a go?

That's a different mode and doesn't seem to be relevant to the current situation and this change to drop the -enable flag.

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.

Right.

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.

Performance is the smaller part of why init=zero is being used so widely. It's about stability. Quoting from my earlier thread:

Another driving factor (see below from various vendors/projects), is the
security stance. Putting non-zero values into most variables types ends
up making them arguably more dangerous than if they were zero-filled.
Most notably, sizes and indexes and less likely to be used out of bounds
if they are zero-initialized. The same holds for bool values that tend
to indicate success instead of failing safe with a false value. While
pointers in the non-canonical range are nice, zero tends to be just
as good. There are certainly exceptions here, but the bulk of the
historical record on how "uninitialized" variables have been used in
real world exploitation involve their being non-zero, and analysis of
those bugs support that conclusion.

This "usually handled safely" state (e.g. existing NULL pointer checks) is specifically why Chrome OS switched from init=pattern to init=zero.

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
I assume he would like someone to pursue it further, it was a good faith attempt at not just demanding. I'd played with it and it needed a few fixes, but overall it was pretty complete. Does someone want to give it a go?

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?

@rsmith can we get some guidance here? Has your opinion changed in the time since GCC has been shipping this?

@rsmith can we get some guidance here? Has your opinion changed in the time since GCC has been shipping this?

Maybe we should now ask @aaron.ballman as he is now main code owner.

@rsmith can we get some guidance here? Has your opinion changed in the time since GCC has been shipping this?

Maybe we should now ask @aaron.ballman as he is now main code owner.

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

MaskRay accepted this revision.Sep 29 2022, 9:47 AM
This revision is now accepted and ready to land.Sep 29 2022, 9:47 AM
kees added a comment.Sep 29 2022, 12:30 PM

Thanks for all the careful consideration; I appreciate it! I'll land this tomorrow unless there are new specific objections. :)

aaron.ballman added inline comments.Sep 29 2022, 12:34 PM
clang/docs/ReleaseNotes.rst
280–283

[clang][auto-init] Remove -enable flag for "zero" mode

The subject should be updated to mention the exact option name, not a vague -enable and "zero" mode

kees marked an inline comment as done.Sep 29 2022, 9:30 PM

[clang][auto-init] Remove -enable flag for "zero" mode

The subject should be updated to mention the exact option name, not a vague -enable and "zero" mode

Yes, good point. Coming right up! :)

kees updated this revision to Diff 464146.Sep 29 2022, 11:14 PM
kees retitled this revision from [clang][auto-init] Remove -enable flag for "zero" mode to [clang][auto-init] Remove -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang.

rebase and tweak

Rebase to main, update subject, add markup to release notes.

kees updated this revision to Diff 464337.Sep 30 2022, 11:26 AM
kees retitled this revision from [clang][auto-init] Remove -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang to [clang][auto-init] Deprecate -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang.

Adjust subject to be more accurate ("deprecate" vs "remove")

nickdesaulniers accepted this revision.Sep 30 2022, 1:29 PM
kees updated this revision to Diff 464381.Sep 30 2022, 1:29 PM

Keep git-clang-format happy

clang/lib/Driver/ToolChains/Clang.cpp
3463–3464

Rather than open code the string, I wonder if we can use getLastArg + A->getAsString(Args)?

srhines accepted this revision.Sep 30 2022, 2:05 PM

Thank you, Kees and reviewers, for helping to make progress here.

xbolva00 accepted this revision.Sep 30 2022, 2:09 PM

Add the option to Group<clang_ignored_legacy_options_Group> instead of adding a custom diagnostic code.

kees updated this revision to Diff 464474.Sep 30 2022, 11:25 PM

use Group<clang_ignored_legacy_options_Group>

Instead of a custom warning, use the clang_ignored_legacy_options_Group, as MaskRay suggested.

kees updated this revision to Diff 464476.Sep 30 2022, 11:29 PM

Update optional removal target release to Clang 18

2 releases was the suggested time to wait between deprecation and removal
for this option. As this change was originally written during the Clang
15 development window, and we're now on 16, update the target to Clang 18.

kees marked an inline comment as done.Sep 30 2022, 11:37 PM
kees added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
3463–3464

I went with MaskRay's suggestion, which ultimately does the same thing, just without doing it open-coded.

MaskRay accepted this revision.Oct 1 2022, 12:59 AM
This revision was automatically updated to reflect the committed changes.
kees marked an inline comment as done.