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

2022-09-29 Thread Kees Cook via Phabricator via cfe-commits 
kees marked an inline comment as done.
kees added a comment.

In D125142#3825972 , @MaskRay wrote:

>> [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! :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-29 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay added a comment.

> [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`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-29 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:240-243
+- -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang
+  has been deprecated. The flag will be removed in Clang 17.
+  -ftrivial-auto-var-init=zero is now available unconditionally, to be
+  compatible with GCC.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-29 Thread Kees Cook via Phabricator via cfe-commits 
kees added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-15 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

In D125142#3782296 , @xbolva00 wrote:

> In D125142#3782244 , 
> @nickdesaulniers wrote:
>
>> @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=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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-10 Thread Dávid Bolvanský via Phabricator via cfe-commits 
xbolva00 added a comment.

In D125142#3782244 , @nickdesaulniers 
wrote:

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-10 Thread Nick Desaulniers via Phabricator via cfe-commits 
nickdesaulniers added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-08-03 Thread Dávid Bolvanský via Phabricator via cfe-commits 
xbolva00 added a comment.

In D125142#3505767 , @jfb wrote:

> 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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-05-11 Thread Kees Cook via Phabricator via cfe-commits 
kees added a comment.

In D125142#3505767 , @jfb wrote:

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-05-11 Thread JF Bastien via Phabricator via cfe-commits 
jfb added a subscriber: Florian.
jfb added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-05-10 Thread Kees Cook via Phabricator via cfe-commits 
kees added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-05-10 Thread Kees Cook via Phabricator via cfe-commits 
kees added a comment.

In D125142#3502732 , @MaskRay wrote:

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-05-10 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

In D125142#3502732 , @MaskRay wrote:

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-05-10 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-05-09 Thread Kees Cook via Phabricator via cfe-commits 
kees updated this revision to Diff 428224.
kees edited the summary of this revision.
kees added a comment.

add release notes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/cl-options.c
  clang/test/Driver/clang_f_opts.c

Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -569,18 +569,19 @@
 
 // RUN: %clang -### -S -ftrivial-auto-var-init=uninitialized %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-UNINIT %s
 // RUN: %clang -### -S -ftrivial-auto-var-init=pattern %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-PATTERN %s
-// RUN: %clang -### -S -ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-GOOD %s
-// RUN: %clang -### -S -ftrivial-auto-var-init=zero %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-BAD %s
+// RUN: %clang -### -S -ftrivial-auto-var-init=zero %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO %s
+// RUN: %clang -### -S -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang \
+// RUN:   -ftrivial-auto-var-init=zero %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-ENABLE-DEPRECATED %s
 // CHECK-TRIVIAL-UNINIT-NOT: hasn't been enabled
 // CHECK-TRIVIAL-PATTERN-NOT: hasn't been enabled
-// CHECK-TRIVIAL-ZERO-GOOD-NOT: hasn't been enabled
-// CHECK-TRIVIAL-ZERO-BAD: hasn't been enabled
+// CHECK-TRIVIAL-ZERO-NOT: hasn't been enabled
+// CHECK-TRIVIAL-ZERO-ENABLE-DEPRECATED: has been deprecated
 
 // RUN: %clang -### -S -ftrivial-auto-var-init=pattern -ftrivial-auto-var-init-stop-after=1 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-PATTERN-STOP-AFTER %s
-// RUN: %clang -### -S -ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang -ftrivial-auto-var-init-stop-after=1 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-STOP-AFTER %s
+// RUN: %clang -### -S -ftrivial-auto-var-init=zero -ftrivial-auto-var-init-stop-after=1 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-STOP-AFTER %s
 // RUN: %clang -### -S -ftrivial-auto-var-init-stop-after=0 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-STOP-AFTER-MISSING-DEPENDENCY %s
 // RUN: %clang -### -S -ftrivial-auto-var-init=pattern -ftrivial-auto-var-init-stop-after=0 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-PATTERN-STOP-AFTER-INVALID-VALUE %s
-// RUN: %clang -### -S -ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang -ftrivial-auto-var-init-stop-after=0 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-STOP-AFTER-INVALID-VALUE %s
+// RUN: %clang -### -S -ftrivial-auto-var-init=zero -ftrivial-auto-var-init-stop-after=0 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-STOP-AFTER-INVALID-VALUE %s
 // CHECK-TRIVIAL-PATTERN-STOP-AFTER-NOT: is used without '-ftrivial-auto-var-init'
 // CHECK-TRIVIAL-PATTERN-STOP-AFTER-NOT: only accepts positive integers
 // CHECK-TRIVIAL-ZERO-STOP-AFTER-NOT: is used without '-ftrivial-auto-var-init'
Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -734,7 +734,6 @@
 // RUN: -fimplicit-modules \
 // RUN: -fno-implicit-modules \
 // RUN: -ftrivial-auto-var-init=zero \
-// RUN: -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang \
 // RUN: --version \
 // RUN: -Werror /Zs -- %s 2>&1
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3411,8 +3411,9 @@
 }
 
   if (!TrivialAutoVarInit.empty()) {
-if (TrivialAutoVarInit == "zero" && !Args.hasArg(options::OPT_enable_trivial_var_init_zero))
-  D.Diag(diag::err_drv_trivial_auto_var_init_zero_disabled);
+if (TrivialAutoVarInit == "zero" && Args.hasArg(options::OPT_enable_trivial_var_init_zero))
+  D.Diag(diag::warn_ignored_clang_option)
+  << "-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang";
 CmdArgs.push_back(
 Args.MakeArgString("-ftrivial-auto-var-init=" + TrivialAutoVarInit));
   }
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2697,9 +2697,10 @@
   NormalizedValuesScope<"LangOptions::TrivialAutoVarInitKind">,

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

2022-05-09 Thread Nick Desaulniers via Phabricator via cfe-commits 
nickdesaulniers accepted this revision.
nickdesaulniers added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-05-09 Thread Kees Cook via Phabricator via cfe-commits 
kees updated this revision to Diff 428173.
kees added a comment.
This revision is now accepted and ready to land.

update with suggestions

- Add FIXME comment with deprecation schedule
- Report deprecation warning when -enable flag is used


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/cl-options.c
  clang/test/Driver/clang_f_opts.c

Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -569,18 +569,19 @@
 
 // RUN: %clang -### -S -ftrivial-auto-var-init=uninitialized %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-UNINIT %s
 // RUN: %clang -### -S -ftrivial-auto-var-init=pattern %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-PATTERN %s
-// RUN: %clang -### -S -ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-GOOD %s
-// RUN: %clang -### -S -ftrivial-auto-var-init=zero %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-BAD %s
+// RUN: %clang -### -S -ftrivial-auto-var-init=zero %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO %s
+// RUN: %clang -### -S -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang \
+// RUN:   -ftrivial-auto-var-init=zero %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-ENABLE-DEPRECATED %s
 // CHECK-TRIVIAL-UNINIT-NOT: hasn't been enabled
 // CHECK-TRIVIAL-PATTERN-NOT: hasn't been enabled
-// CHECK-TRIVIAL-ZERO-GOOD-NOT: hasn't been enabled
-// CHECK-TRIVIAL-ZERO-BAD: hasn't been enabled
+// CHECK-TRIVIAL-ZERO-NOT: hasn't been enabled
+// CHECK-TRIVIAL-ZERO-ENABLE-DEPRECATED: has been deprecated
 
 // RUN: %clang -### -S -ftrivial-auto-var-init=pattern -ftrivial-auto-var-init-stop-after=1 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-PATTERN-STOP-AFTER %s
-// RUN: %clang -### -S -ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang -ftrivial-auto-var-init-stop-after=1 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-STOP-AFTER %s
+// RUN: %clang -### -S -ftrivial-auto-var-init=zero -ftrivial-auto-var-init-stop-after=1 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-STOP-AFTER %s
 // RUN: %clang -### -S -ftrivial-auto-var-init-stop-after=0 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-STOP-AFTER-MISSING-DEPENDENCY %s
 // RUN: %clang -### -S -ftrivial-auto-var-init=pattern -ftrivial-auto-var-init-stop-after=0 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-PATTERN-STOP-AFTER-INVALID-VALUE %s
-// RUN: %clang -### -S -ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang -ftrivial-auto-var-init-stop-after=0 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-STOP-AFTER-INVALID-VALUE %s
+// RUN: %clang -### -S -ftrivial-auto-var-init=zero -ftrivial-auto-var-init-stop-after=0 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-STOP-AFTER-INVALID-VALUE %s
 // CHECK-TRIVIAL-PATTERN-STOP-AFTER-NOT: is used without '-ftrivial-auto-var-init'
 // CHECK-TRIVIAL-PATTERN-STOP-AFTER-NOT: only accepts positive integers
 // CHECK-TRIVIAL-ZERO-STOP-AFTER-NOT: is used without '-ftrivial-auto-var-init'
Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -734,7 +734,6 @@
 // RUN: -fimplicit-modules \
 // RUN: -fno-implicit-modules \
 // RUN: -ftrivial-auto-var-init=zero \
-// RUN: -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang \
 // RUN: --version \
 // RUN: -Werror /Zs -- %s 2>&1
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3411,8 +3411,9 @@
 }
 
   if (!TrivialAutoVarInit.empty()) {
-if (TrivialAutoVarInit == "zero" && !Args.hasArg(options::OPT_enable_trivial_var_init_zero))
-  D.Diag(diag::err_drv_trivial_auto_var_init_zero_disabled);
+if (TrivialAutoVarInit == "zero" && Args.hasArg(options::OPT_enable_trivial_var_init_zero))
+  D.Diag(diag::warn_ignored_clang_option)
+  << "-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang";
 CmdArgs.push_back(
 Args.MakeArgString("-ftrivial-auto-var-init=" + TrivialAutoVarInit));
   }
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2697,9 +2697,10 @@

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

2022-05-09 Thread Nick Desaulniers via Phabricator via cfe-commits 
nickdesaulniers accepted this revision.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.

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();
  return y;
  }




Comment at: clang/test/Driver/clang_f_opts.c:573
+// RUN: %clang -### -S -ftrivial-auto-var-init=zero %s 2>&1 | FileCheck 
-check-prefix=CHECK-TRIVIAL-ZERO %s
+// RUN: %clang -### -S -ftrivial-auto-var-init=zero 
-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang %s 
2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-UNUSED %s
 // CHECK-TRIVIAL-UNINIT-NOT: hasn't been enabled

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

```
// RUN: %clang -### ... -S \
// RUN:   -ftrivial-auto-var-init=zero ...
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-05-09 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

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




Comment at: clang/include/clang/Driver/Options.td:2700-2701
   MarshallingInfoEnum, "Uninitialized">;
 def enable_trivial_var_init_zero : Flag<["-"], 
"enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang">,
-  Flags<[CC1Option, CoreOption, NoArgumentUnused]>,
-  HelpText<"Trivial automatic variable initialization to zero is only here for 
benchmarks, it'll eventually be removed, and I'm OK with that because I'm only 
using it to benchmark">;
+  Flags<[CC1Option, CoreOption, Ignored]>;
 def ftrivial_auto_var_init_stop_after : Joined<["-"], 
"ftrivial-auto-var-init-stop-after=">, Group,

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-05-08 Thread Kees Cook via Phabricator via cfe-commits 
kees updated this revision to Diff 427916.
kees added a comment.

Report flag as "unused"

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/cl-options.c
  clang/test/Driver/clang_f_opts.c

Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -569,18 +569,18 @@
 
 // RUN: %clang -### -S -ftrivial-auto-var-init=uninitialized %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-UNINIT %s
 // RUN: %clang -### -S -ftrivial-auto-var-init=pattern %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-PATTERN %s
-// RUN: %clang -### -S -ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-GOOD %s
-// RUN: %clang -### -S -ftrivial-auto-var-init=zero %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-BAD %s
+// RUN: %clang -### -S -ftrivial-auto-var-init=zero %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO %s
+// RUN: %clang -### -S -ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-UNUSED %s
 // CHECK-TRIVIAL-UNINIT-NOT: hasn't been enabled
 // CHECK-TRIVIAL-PATTERN-NOT: hasn't been enabled
-// CHECK-TRIVIAL-ZERO-GOOD-NOT: hasn't been enabled
-// CHECK-TRIVIAL-ZERO-BAD: hasn't been enabled
+// CHECK-TRIVIAL-ZERO-NOT: hasn't been enabled
+// CHECK-TRIVIAL-ZERO-UNUSED: argument unused during compilation
 
 // RUN: %clang -### -S -ftrivial-auto-var-init=pattern -ftrivial-auto-var-init-stop-after=1 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-PATTERN-STOP-AFTER %s
-// RUN: %clang -### -S -ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang -ftrivial-auto-var-init-stop-after=1 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-STOP-AFTER %s
+// RUN: %clang -### -S -ftrivial-auto-var-init=zero -ftrivial-auto-var-init-stop-after=1 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-STOP-AFTER %s
 // RUN: %clang -### -S -ftrivial-auto-var-init-stop-after=0 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-STOP-AFTER-MISSING-DEPENDENCY %s
 // RUN: %clang -### -S -ftrivial-auto-var-init=pattern -ftrivial-auto-var-init-stop-after=0 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-PATTERN-STOP-AFTER-INVALID-VALUE %s
-// RUN: %clang -### -S -ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang -ftrivial-auto-var-init-stop-after=0 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-STOP-AFTER-INVALID-VALUE %s
+// RUN: %clang -### -S -ftrivial-auto-var-init=zero -ftrivial-auto-var-init-stop-after=0 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-STOP-AFTER-INVALID-VALUE %s
 // CHECK-TRIVIAL-PATTERN-STOP-AFTER-NOT: is used without '-ftrivial-auto-var-init'
 // CHECK-TRIVIAL-PATTERN-STOP-AFTER-NOT: only accepts positive integers
 // CHECK-TRIVIAL-ZERO-STOP-AFTER-NOT: is used without '-ftrivial-auto-var-init'
Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -734,7 +734,6 @@
 // RUN: -fimplicit-modules \
 // RUN: -fno-implicit-modules \
 // RUN: -ftrivial-auto-var-init=zero \
-// RUN: -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang \
 // RUN: --version \
 // RUN: -Werror /Zs -- %s 2>&1
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3411,8 +3411,6 @@
 }
 
   if (!TrivialAutoVarInit.empty()) {
-if (TrivialAutoVarInit == "zero" && !Args.hasArg(options::OPT_enable_trivial_var_init_zero))
-  D.Diag(diag::err_drv_trivial_auto_var_init_zero_disabled);
 CmdArgs.push_back(
 Args.MakeArgString("-ftrivial-auto-var-init=" + TrivialAutoVarInit));
   }
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2698,8 +2698,7 @@
   NormalizedValues<["Uninitialized", "Zero", "Pattern"]>,
   MarshallingInfoEnum, "Uninitialized">;
 def enable_trivial_var_init_zero : Flag<["-"], "enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang">,
-  Flags<[CC1Option, CoreOption, NoArgumentUnused]>,
-  HelpText<"Trivial automatic variable initialization to zero is

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

2022-05-08 Thread Kees Cook via Phabricator via cfe-commits 
kees planned changes to this revision.
kees added a comment.

In D125142#3499010 , @brooks wrote:

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-05-07 Thread Brooks Davis via Phabricator via cfe-commits 
brooks added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-05-06 Thread Dávid Bolvanský via Phabricator via cfe-commits 
xbolva00 added a comment.

Strong +1.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits