[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

Not sure if this is the fault of the LLVM half or the Clang half, but I'm 
seeing mis-compilations in the current patches (llvm 
ca1e713fdd4fab5273b36ba6f292a844fca4cb2d with D53765 
.185490 and clang 
01879634f01bdbfac4636ebe03b68e85b20cd664 with D56571 
.185489). My earlier builds were okay (llvm 
b1650507d25d28a03f30626843b7b133796597b4 with D53765 
.183738 and clang 
61738985ebe78eeff6cfae7f97543d3456bac25a with D56571 
.181973).

I narrowed the failure down to the kernel's move_addr_to_user() function:

  static int move_addr_to_user(struct sockaddr_storage *kaddr, int klen,
   void __user *uaddr, int __user *ulen)
  {
  int err;
  int len;
  
  BUG_ON(klen > sizeof(struct sockaddr_storage));
  err = get_user(len, ulen);
  if (err)
  return err;
  if (len > klen)
  len = klen;
  if (len < 0)
  return -EINVAL;
  if (len) {
  if (audit_sockaddr(klen, kaddr))
  return -ENOMEM;
  if (copy_to_user(uaddr, kaddr, len))
  return -EFAULT;
  }
  return __put_user(klen, ulen);
  }

Working build produces:

  81c217a0 :
  81c217a0:   e8 5b fc 3d 00  callq  82001400 
<__fentry__>
  81c217a5:   81 fe 81 00 00 00   cmp$0x81,%esi
  81c217ab:   0f 83 c8 00 00 00   jae81c21879 

  81c217b1:   55  push   %rbp
  81c217b2:   48 89 e5mov%rsp,%rbp
  81c217b5:   41 57   push   %r15
  81c217b7:   41 56   push   %r14
  81c217b9:   41 55   push   %r13
  81c217bb:   41 54   push   %r12
  81c217bd:   53  push   %rbx
  81c217be:   49 89 cemov%rcx,%r14
  81c217c1:   49 89 d7mov%rdx,%r15
  81c217c4:   41 89 f5mov%esi,%r13d
  81c217c7:   49 89 fcmov%rdi,%r12
  81c217ca:   48 c7 c7 36 35 88 82mov
$0x82883536,%rdi
  81c217d1:   be da 00 00 00  mov$0xda,%esi
  81c217d6:   e8 25 8b 5c ff  callq  811ea300 
<__might_fault>
  81c217db:   4c 89 f0mov%r14,%rax
  81c217de:   e8 2d f6 2f 00  callq  81f20e10 
<__get_user_4>
  81c217e3:   85 c0   test   %eax,%eax
  81c217e5:   75 68   jne81c2184f 

  81c217e7:   48 89 d3mov%rdx,%rbx
  81c217ea:   44 39 ebcmp%r13d,%ebx
  81c217ed:   41 0f 4f dd cmovg  %r13d,%ebx
  81c217f1:   85 db   test   %ebx,%ebx
  81c217f3:   78 65   js 81c2185a 

  81c217f5:   74 48   je 81c2183f 

  81c217f7:   65 48 8b 04 25 00 4emov%gs:0x14e00,%rax
  81c217fe:   01 00 
  81c21800:   48 8b 80 38 07 00 00mov0x738(%rax),%rax
  81c21807:   48 85 c0test   %rax,%rax
  81c2180a:   74 05   je 81c21811 

  81c2180c:   83 38 00cmpl   $0x0,(%rax)
  81c2180f:   74 50   je 81c21861 

  81c21811:   48 63 dbmovslq %ebx,%rbx
  81c21814:   4c 89 e7mov%r12,%rdi
  81c21817:   48 89 demov%rbx,%rsi
  81c2181a:   ba 01 00 00 00  mov$0x1,%edx
  81c2181f:   e8 3c 75 5f ff  callq  81218d60 
<__check_object_size>
  81c21824:   4c 89 ffmov%r15,%rdi
  81c21827:   4c 89 e6mov%r12,%rsi
  81c2182a:   48 89 damov%rbx,%rdx
  81c2182d:   e8 ae 84 99 ff  callq  815b9ce0 
<_copy_to_user>
  81c21832:   48 89 c1mov%rax,%rcx
  81c21835:   b8 f2 ff ff ff  mov$0xfff2,%eax
  81c2183a:   48 85 c9test   %rcx,%rcx
  81c2183d:   75 10   jne81c2184f 

  81c2183f:   90  nop
  81c21840:   90  nop
  81c21841:   90  nop
  81c21842:   b8 f2 ff ff ff  mov$0xfff2,%eax
  81c21847:  

[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

I reduced the C code to this:

  volatile int mystery = 0;
  
  static int noinline demo(int klen)
  {
  int err;
  int len;
  
  err = mystery * klen;
  if (err)
  return err;
  if (len > klen)
  len = klen;
  if (len < 0)
  return -EINVAL;
  if (len)
  return -ENOMEM;
  return __put_user(klen, (int __user *)NULL);
  }

Something in the __put_user() use of asm-goto is broken.


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

https://reviews.llvm.org/D56571



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

[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D56571#1386973 , @kees wrote:

> Not sure if this is the fault of the LLVM half or the Clang half, but I'm 
> seeing mis-compilations in the current patches (llvm 
> ca1e713fdd4fab5273b36ba6f292a844fca4cb2d with D53765 
> .185490 and clang 
> 01879634f01bdbfac4636ebe03b68e85b20cd664 with D56571 
> .185489). My earlier builds were okay (llvm 
> b1650507d25d28a03f30626843b7b133796597b4 with D53765 
> .183738 and clang 
> 61738985ebe78eeff6cfae7f97543d3456bac25a with D56571 
> .181973).


This appears to be fixed with latest llvm, clang, and D53765 
.185703.


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

https://reviews.llvm.org/D56571



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

[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

[Kees Cook via Phabricator via cfe-commits]

kees added inline comments.



Comment at: test/Analysis/asm-goto.cpp:1
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG %s 2>&1 | FileCheck 
%s
+

Based on Nathan's comments in another thread, it seems like this needs to be:

```
// REQUIRES: x86-registered-target
// RUN: %clang_analyze_cc1 -triple x86_64 -analyzer-checker=debug.DumpCFG %s 
2>&1 | FileCheck %s
```



Comment at: test/CodeGen/asm-goto.c:1
+// RUN: %clang_cc1 -O0 -emit-llvm  %s -o - | FileCheck %s
+

Based on Nathan's comments in another thread, it seems like this needs to be:

```
// REQUIRES: x86-registered-target
// RUN: %clang_cc1 -O0 -triple x86_64 -emit-llvm  %s -o - | FileCheck %s
```



Comment at: test/Sema/asm-goto.cpp:1
+// RUN: %clang_cc1 %s -triple i386-pc-linux-gnu -verify -fsyntax-only
+

Based on Nathan's comments in another thread, it seems like this needs to be:

```
// REQUIRES: x86-registered-target
// RUN: %clang_cc1 -triple x86_64 -fsyntax-only -verify -fblocks -std=gnu99 %s 
-Wno-unreachable-code
```


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

https://reviews.llvm.org/D56571



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

[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

Nick points out that "REQUIRES: x86-registered-target" is likely not needed.


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

https://reviews.llvm.org/D56571



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

[PATCH] D65629: cfi-icall: Allow the jump table to be optionally made non-canonical.

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

Just FYI, I can confirm a happily running arm64 kernel with CFI enabled built 
with this patch series. The C wrappers aren't needed and CFI is still 
triggering on mismatches:

  [  106.656280] lkdtm: Performing direct entry CFI_FORWARD_PROTO
  [  106.657307] lkdtm: Calling matched prototype ...
  [  106.657432] lkdtm: Calling mismatched prototype ...
  [  106.658216] [ cut here ]
  [  106.659084] CFI failure (target: 
lkdtm_increment_int$53641d38e2dc4a151b75cbe816cbb86b.cfi_jt+0x0/0x4):
  [  106.671576] WARNING: CPU: 1 PID: 2716 at kernel/cfi.c:29 
__cfi_check_fail+0x44/0x4c


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65629



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

[PATCH] D63260: [Attr] Support _attribute__ ((fallthrough))

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

For latest version see https://reviews.llvm.org/D64838


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

https://reviews.llvm.org/D63260



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

[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

Should the per-function analysis warning actually be removed? That seems like a 
helpful check to catch a different form of bad behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80791



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

[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

Specifically, this appears to be a legitimate bug, found by the warnings: 
https://bugs.llvm.org/show_bug.cgi?id=46258


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80791



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

[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D80791#2120503 , @nickdesaulniers 
wrote:

> Might someone wish to disable PAC/BTI on an individual function, while having 
> it on for the rest?  I guess that would mean you can't call that function 
> indirectly?


It would mean you can't call it _at all_, not just indirectly. :) Which is why 
I still think the warning is useful. Perhaps don't warn for the functions with 
the attribute?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80791



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

[PATCH] D123958: [randstruct] Randomize all elements of a record

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D123958#3459205 , @aaron.ballman 
wrote:

> I had assumed that any structure not marked for randomization would not be 
> randomized. Based on that, I don't think inner structure objects (anonymous 
> or otherwise) should automatically randomize their fields. WDYT?

Correct, inner structs should be evaluated without looking at the state of the 
outer randomization.

  struct mixed {
  int a;
  float b;
  } __attribute__((randomize_layout));
  
  struct ordered {
  int foo;
  char bar[8];
  };
  
  struct composite {
  int one;
  struct mixed two;
  struct ordered three;
  struct {
  unsigned long am;
  double ordered;
  };
  } __attribute__((randomize_layout));



- Each of the member's relative positions of `struct composite` should be 
randomized: one, two, three, anon struct.
- The members of `two` should be randomized.
- The members of `three` should //not// be randomized.
- The members of the anon struct should //not// be randomized.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123958

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

[PATCH] D123544: [randstruct] Automatically randomize a structure of function pointers

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

I tested this (with D123958 ), and it appears 
to be working as intended. These two fptrs are the first and second listed, and 
are happily randomized:

  [0.00] LSM: offsetof(struct security_hook_heads, 
ptrace_access_check): 816
  [0.00] LSM: offsetof(struct security_hook_heads, ptrace_traceme): 512


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123544

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

[PATCH] D110869: [X86] Implement -fzero-call-used-regs option

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

I can build and boot with this. Nice! :) One issue I see is in instruction 
sequence ordering.

Looking at the end of `__startup_64` without the feature enabled, everything 
looks "normal":

  31 c0  xor%eax,%eax
  5b pop%rbx
  41 5e  pop%r14
  41 5f  pop%r15
  5d pop%rbp
  c3 ret

with `-fzero-call-used-regs=used-gpr`:

  31 c0  xor%eax,%eax
  31 c9  xor%ecx,%ecx
  31 ff  xor%edi,%edi
  31 d2  xor%edx,%edx
  31 f6  xor%esi,%esi
  45 31 c0   xor%r8d,%r8d
  45 31 c9   xor%r9d,%r9d
  45 31 d2   xor%r10d,%r10d
  45 31 db   xor%r11d,%r11d
  5b pop%rbx
  41 5e  pop%r14
  41 5f  pop%r15
  5d pop%rbp
  c3 ret

The registers are being wiped, and that's what's needed for the "reducing data 
lifetime" part of this feature, but because of the ordering, it is not really 
making ROP attacks harder since all the `pop` instructions are available before 
the `ret`. I would have expected:

  31 c0  xor%eax,%eax
  5b pop%rbx
  41 5e  pop%r14
  41 5f  pop%r15
  5d pop%rbp
  31 c9  xor%ecx,%ecx
  31 ff  xor%edi,%edi
  31 d2  xor%edx,%edx
  31 f6  xor%esi,%esi
  45 31 c0   xor%r8d,%r8d
  45 31 c9   xor%r9d,%r9d
  45 31 d2   xor%r10d,%r10d
  45 31 db   xor%r11d,%r11d
  c3 ret

And looking at the results with GCC, that's effectively the case. (They're 
still using `xor`+`mov`, but I'm expecting them 
 to switch to all-`xor`.)

  31 c0  xor%eax,%eax
  5b pop%rbx
  5d pop%rbp
  41 5c  pop%r12
  31 d2  xor%edx,%edx
  89 d1  mov%edx,%ecx
  89 d6  mov%edx,%esi
  89 d7  mov%edx,%edi
  41 89 d0   mov%edx,%r8d
  41 89 d1   mov%edx,%r9d
  41 89 d2   mov%edx,%r10d
  41 89 d3   mov%edx,%r11d
  c3 ret

With that swapped around, this will be looking great! :)

Some more details about the anti-ROP-ness are here:
https://git.kernel.org/linus/a82adfd5c7cb4b8bb37ef439aed954f9972bb618


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110869

___
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

[Kees Cook via Phabricator via cfe-commits]

kees created this revision.
kees added reviewers: jfb, nickdesaulniers, aaron.ballman.
Herald added a project: All.
kees requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

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.

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


Repository:
  rG LLVM Github Monorepo

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,16 @@
 
 // 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-GOOD %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
 
 // 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
@@ -2697,9 +2697,6 @@
   NormalizedValuesScope<"LangOptions::TrivialAutoVarInitKind">,
   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, CoreOptio

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

[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

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

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

[Kees Cook via Phabricator via cfe-commits]

kees added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:906
   // member, only a T[0] or T[] member gets that treatment.
+  // Under StrictFlexArraysLevel, obey c99+ that disallows FAM in union, 
see
+  // C11 6.7.2.1 §18

jyknight wrote:
> serge-sans-paille wrote:
> > jyknight wrote:
> > > I believe this bit is incorrect -- it should just go back to 'return 
> > > true;'. The StrictFlexArraysLevel check above already eliminates the 
> > > cases we want to eliminate (size==1 in strictness-level 2.)
> > Well, if we are in strictness-level 2, with an undefined size or size = 0, 
> > we can still reach that path, and don't want to return 'true' because FAM 
> > in union are in invalid per the standard.
> Yes, we can reach this path, which is why the change is incorrect. We should 
> not be changing the FAMness of undefined size, or size == 0, in any of the 
> modes. To be more specific -- 
> 
> `union X { int x[0]; };` should still be a FAM in all strictness modes. (if 
> you don't want zero-length-arrays, use `-Werror=zero-length-array`).
> 
> For `union X { int x[]; };`: this ought to be a compiler error. It's likely 
> just an oversight that we currently accept it;  I'd be OK with a (separate) 
> patch to fix that. (GCC emits an error, so there's unlikely to be 
> compatibility issues with such a change.)
> `union X { int x[0]; };` should still be a FAM in all strictness modes. (if 
> you don't want zero-length-arrays, use `-Werror=zero-length-array`).

The Linux kernel cannot use `-Wzero-length-array` because we have cases of 
userspace APIs being stuck with them. (i.e. they are part of the struct 
declaration, even though the kernel code doesn't use them.) For example:

```
In file included from ../kernel/bounds.c:13:
In file included from ../include/linux/log2.h:12:
In file included from ../include/linux/bitops.h:9:
In file included from ../include/uapi/linux/kernel.h:5:
../include/uapi/linux/sysinfo.h:22:10: error: zero size arrays are an extension 
[-Werror,-Wzero-length-array]
char _f[20-2*sizeof(__kernel_ulong_t)-sizeof(__u32)];   /* Padding: 
libc5 uses this.. */
^~~
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126864

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

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D126864#3641939 , 
@serge-sans-paille wrote:

> @kees are you ok with current state?

Build tests are still looking correct on my end. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126864

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

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

I should clarify: I still need the =3 mode. Since sizeof([0]) == 0 and 
sizeof([]) == error, they are being treated differently already by the compiler 
causing bugs in Linux. The kernel must still have a way to reject the _use_ of 
a [0] array. We cannot reject _declaration_ of them due to userspace API.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126864

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

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D126864#3646854 , @jyknight wrote:

> In D126864#3645994 , @kees wrote:
>
>> I should clarify: I still need the =3 mode. Since sizeof([0]) == 0 and 
>> sizeof([]) == error, they are being treated differently already by the 
>> compiler causing bugs in Linux. The kernel must still have a way to reject 
>> the _use_ of a [0] array. We cannot reject _declaration_ of them due to 
>> userspace API.
>
> Looks like the linux kernel is currently chock-full of zero-length-arrays 
> which are actually intended/required to work as flexible array members. Do 
> you have a pending patch to convert all of them to [] which should be?

Well, yes and no. It's been a multi-year on-going effort. Though it's actually 
complete for the kernel now as of v5.18:
https://github.com/KSPP/linux/issues/78

What remains is the userspace headers. The _real_ pain has been the 1-element 
arrays. Ugh. The master bug is here:
https://github.com/KSPP/linux/issues/21

> If you're saying you absolutely need this mode -- which I still believe would 
> be nonsensical to provide -- I'd like to see the set of concrete examples you 
> cannot otherwise deal with.

I have several goals. The most important is making all the fixed-sized trailing 
arrays are NOT treated as flexible arrays so that `__builtin_object_size` will 
work as expected for Linux's FORTIFY implementation and 
`-fstrict-flex-arrays=2` certainly solves that. However, then we're left with 
all the 0-element arrays, which need to be cleaned up because they don't behave 
the same as true flexible arrays. For example, with `sizeof`; the kernel has 
dealt with allocation size bugs relating to `sizeof` being used against 
0-element arrays (which _should_ have caused a build failure if it were a true 
flexible array and the bug would have been immediately found), and I would like 
to make sure those can never happen again.

If this were any other code base, I could just use `-Wzero-length-array`, but 
the scattered ancient userspace APIs need to live on until $forever.

And if this were any other code base, I could use `#pragma`, but it's frown on 
in general, and is going to be met with even greater resistance in userspace 
API headers, especially given that it will need to be `#ifdef`ed for clang vs 
gcc, and then tweaked again once GCC gains the option, which would then need to 
be version checked. (This is why `#pragma` is strongly discouraged: Linux has a 
whole system for doing compiler behavior detection for its internal headers and 
build system, but such things don't exist for the userspace headers.)

So given the behavioral differences that already existed between true flexible 
arrays and 0-element arrays, I'd wanted to have a way to turn off _all_ the 
flexible array "extensions". I never wanted these different levels for 
`-fstrict-flex-arrays`. :) I want the complete disabling of all the extensions 
that were creating inconsistent behaviors. But to lose the ability to disable 
the 0-is-flexible would be disappointing, but if I absolutely have no choice, I 
can live with it. Given Linux will be the first user of this option, and the 
reason for this option being created, I'd _much_ rather have it doing what's 
needed, though. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126864

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

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

Example of the bug I want to block:

  struct foo {
  int stuff;
  u32 data[0];
  };
  
  struct foo *deserialize(u8 *str, int len)
  {
  struct foo *instance;
  size_t bytes;
  
  bytes = sizeof(*instance) + sizeof(instance->data) * (len / sizeof(u32));
  instance = kmalloc(bytes, GFP_KERNEL);
  if (!instance)
  return NULL;
  memcpy(instance->data, str, len)
  }

This contains a catastrophic 1 character bug (should be 
`sizeof(*instance->data)`) that will only be encountered at runtime when the 
memcpy runs past the end of the the allocation. It could have been caught at 
build-time if the flex-array extensions were disabled; without 
`-fstrict-flex-arrays=3` I have no way to block these (or similar) sneaking 
back into the kernel by way of old (or new) userspace APIs. :( So actually, 
even with `#pragma`, we could still trip over this. Please leave the `=3` mode.

https://godbolt.org/z/dexd3a4Y8


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126864

___
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

[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 @@
   Normaliz

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

[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">,
   NormalizedValues<["Uninitial

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

[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

[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

[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] D68720: Support -fstack-clash-protection for x86

[Kees Cook via Phabricator via cfe-commits]

kees reopened this revision.
kees added a comment.
This revision is now accepted and ready to land.

Sorry if I missed something here, but why is this marked as "Closed"? It seems 
like the feature has still not landed (i.e. it got reverted).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68720

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

[PATCH] D68720: Support -fstack-clash-protection for x86

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

Ah! Yes, I see it now. Thanks and sorry for the noise!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68720

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

[PATCH] D91895: [Clang] improve -Wimplicit-fallthrough GCC compat

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

The kernel's [[ 
https://www.kernel.org/doc/html/latest/process/deprecated.html#implicit-switch-case-fall-through

| stance ]] on switch statements reads: |
|



  All switch/case blocks must end in one of:
  
  break;
  fallthrough;
  continue;
  goto ;
  return [expression];

And we've had multiple real bugs now of the "missing break before default 
return" style:

https://git.kernel.org/linus/291ddeb621e4a9f1ced8302a777fbd7fbda058c6

So, from the kernel's perspective, please don't make `-Wimplicit-fallthrough` 
more permissive -- we're finding real bugs. :)

And to speak to another example in the thread:

  // drivers/hid/usbhid/hid-core.c
   490   case -ESHUTDOWN:  /* unplug */ 

 
   491 unplug = 1; 
   492   case -EILSEQ:   /* protocol error or unplug */ // 
-Wimplicit-fallthrough...how?!  

   
   493   case -EPROTO:   /* protocol error or unplug */ 

 
   494   case -ECONNRESET: /* unlink */ 

 
   495   case -ENOENT:  

 
   496   case -EPIPE:/* report not available */ 

 
   497 break;

I think this should warn too. While this won't turn into a "missing break" 
error, there's no way to know (from looking at code) what the _intent_ is here. 
Should it have done "return?" Why is "break" assumed to be safe here, etc?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91895

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

Does this address https://bugs.llvm.org/show_bug.cgi?id=50322 ? (I assume this 
is a new version of https://reviews.llvm.org/D92657 ?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

I'm setting up to test this patch (thank you!) using my current kernel FORTIFY 
improvements. Right now I have a bunch of compile-time behavior selftests 
written:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/overflow&id=3c5221f3f4fd865a780f544b72c68f4209bd2e76

It should be possible to do an A/B test against those from the kernel's view of 
its FORTIFY functions. However, due to other bugs with __builtin_object_size(), 
the kernel still can't use name-matched inlines:
https://github.com/ClangBuiltLinux/linux/issues/1401
i.e. D109967  will fix half of what is 
needed, but the plan in the kernel right now is to work around the problem 
entirely by using macros instead.

I'll report back on the results of my testing, though, since it should give a 
good sense of how much coverage the kernel can gain back with this bug fixed.


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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

Yeah, I can confirm that many cases are improved with this patch (others are 
more sensitive and depend on the __bos behavior I mentioned). With the 17 
kernel FORTIFY self-tests, all 17 fail without this patch. With this patch, 9 
start passing. Nice!


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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

Is https://bugs.llvm.org/show_bug.cgi?id=50322 and dup of 
https://bugs.llvm.org/show_bug.cgi?id=23280 ? (Can both be closed?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967

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

[PATCH] D135727: [clang] Correct sanitizer behavior in union FAMs

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

This looks great to me. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135727

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

[PATCH] D134902: [clang] Implement -fstrict-flex-arrays=3

[Kees Cook via Phabricator via cfe-commits]

kees accepted this revision.
kees added a comment.

The self-tests all look correct to me, so I expect it is working how I'd 
expect. I haven't had a chance to do kernel builds with this yet, but I'm 
hoping to make time soon. I'd say if others are happy, let's land it, and if 
anything unexpected happens we can fix those issues if they appear. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134902

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

[PATCH] D134831: [Clang][Sema] Add -Wcast-function-type-strict

[Kees Cook via Phabricator via cfe-commits]

kees added a subscriber: nathanchance.
kees added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8686
+def warn_cast_function_type_strict : Warning,
+  InGroup, DefaultIgnore;
 def err_cast_pointer_to_non_pointer_int : Error<

aaron.ballman wrote:
> samitolvanen wrote:
> > nickdesaulniers wrote:
> > > I don't think we need a new group for a warning that only contains one 
> > > diagnostic kind?
> > > I don't think we need a new group for a warning that only contains one 
> > > diagnostic kind?
> > 
> > I might have misunderstood something, but we seem to have plenty of groups 
> > with only one diagnostic kind. Is there a way to add a new warning flag 
> > without adding a diagnostic group? Most users of `-Wcast-function-type` 
> > wouldn't want to enable the stricter version, so I would prefer to keep 
> > this separate.
> > 
> > I did notice that some warnings don't define the group in 
> > DiagnosticGroups.td, but specify it directly here. For example, 
> > `InGroup>`. I'm not sure if there are 
> > any benefits in doing so.
> Typically we only define a group in DiagnosticGroups.td when the group is 
> going to be used by more than one diagnostic, otherwise we prefer using 
> `InGroup>` to form one-off diagnostic groups.
> 
> However, in this case, I am wondering if we want to add 
> `CastFunctionTypeStrict` to be a subgroup of `CastFunctionType` so that users 
> who enable `-Wcast-function-type` get the stricter checking by default, but 
> still have a way to disable the stricter checking if it's too noisy for them?
> However, in this case, I am wondering if we want to add 
> `CastFunctionTypeStrict` to be a subgroup of `CastFunctionType` so that users 
> who enable `-Wcast-function-type` get the stricter checking by default, but 
> still have a way to disable the stricter checking if it's too noisy for them?

I'd be for that. It'll be very noisy for the Linux kernel, but they are all 
instances we need to fix.

cc @nathanchance


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134831

___
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

[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] D134902: [clang] Implement -fstrict-flex-arrays=3

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

Excellent! Thank you for getting this prepared. Having this properly managed in 
the kernel means we don't have to do horrible ugly hacks to work around old 
0-length arrays in UAPI (which have all been unioned with a proper flexible 
array now).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134902

___
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

[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-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang

[Kees Cook via Phabricator via cfe-commits]

kees updated this revision to Diff 464146.
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".
kees added a comment.

rebase and tweak

Rebase to main, update subject, add markup to 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/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
@@ -719,7 +719,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
@@ -3458,8 +3458,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

[PATCH] D134902: [clang] Implement -fstrict-flex-arrays=3

[Kees Cook via Phabricator via cfe-commits]

kees added inline comments.



Comment at: clang/test/CodeGen/bounds-checking-fam.c:23
 };
 
 // CHECK-LABEL: define {{.*}} @{{.*}}test_one{{.*}}(

I would expect to see here:

```
struct Zero {
  int a[0];
};

// CHECK-LABEL: define {{.*}} @{{.*}}test_zero{{.*}}(
int test_one(struct One *p, int i) {
  // CHECK-STRICT-0-NOT: @__ubsan
  // CHECK-STRICT-1-NOT: @__ubsan
  // CHECK-STRICT-2-NOT: @__ubsan
  // CHECK-STRICT-3: call void @__ubsan_handle_out_of_bounds_abort(
  return p->a[i] + (p->a)[i];
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134902

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

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

[Kees Cook via Phabricator via cfe-commits]

kees updated this revision to Diff 464337.
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".
kees added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125142

Files:
  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
@@ -719,7 +719,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
@@ -3458,8 +3458,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
===

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

[Kees Cook via Phabricator via cfe-commits]

kees updated this revision to Diff 464381.
kees added a comment.

Keep git-clang-format happy


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125142

Files:
  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
@@ -719,7 +719,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
@@ -3458,8 +3458,11 @@
 }
 
   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
@@ -2791,9 +2791,10 @@
   NormalizedValuesScope<"LangOptions::TrivialAutoVarInitKind">,
   NormalizedValues<["Uninitialized", "Zero", "Pattern"]>,
   MarshallingInfoEn

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

[Kees Cook via Phabricator via cfe-commits]

kees updated this revision to Diff 464474.
kees added a comment.

use Group

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125142

Files:
  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
@@ -719,7 +719,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
@@ -3458,8 +3458,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
@@ -247,6 +247,12 @@
 def mmpx : Flag<["-"], "mmpx">, Group;
 def mno_mpx : Flag<["-"], "mno-mpx">, Group;
 
+// Retired with clang-16.0, to provide a deprecation period; it should
+// be removed in Clang 18 or later.
+def enable_trivial_var_init_zero : Flag<["-"], "enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang">,
+  Flag

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

[Kees Cook via Phabricator via cfe-commits]

kees updated this revision to Diff 464476.
kees added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125142

Files:
  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
@@ -719,7 +719,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
@@ -3458,8 +3458,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
@@ -247,6 +247,12 @@
 def mmpx : Flag<["-"], "mmpx">, Group;
 def mno_mpx : Flag<["-"], "mno-mpx">, Group;
 
+// Retired with clang-16.0, to provide a deprecation period; it should
+/

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

[Kees Cook via Phabricator via cfe-commits]

kees marked an inline comment as done.
kees added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3464-3465
+  D.Diag(diag::warn_ignored_clang_option)
+  << "-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-"
+ "from-clang";
 CmdArgs.push_back(

nickdesaulniers wrote:
> Rather than open code the string, I wonder if we can use `getLastArg` + 
> `A->getAsString(Args)`?
I went with MaskRay's suggestion, which ultimately does the same thing, just 
without doing it open-coded.


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] Deprecate -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang

[Kees Cook via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
kees marked an inline comment as done.
Closed by commit rGaef03c9b3bed: [clang][auto-init] Deprecate 
-enable-trivial-auto-var-init-zero-knowing-it-will… (authored by kees).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125142

Files:
  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
@@ -719,7 +719,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
@@ -3458,8 +3458,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
@@ -247,6 +247,12 @@
 def mmpx : Flag<["-"], "mmpx">, Group;
 def mno_mpx : Flag<["-"], "mno-mpx">, Group;
 
+// Retired with clang-16.0, to provide a deprecation period; it should
+// be removed in Clang 18 or later.
+def enable_trivial_var_init_zero : Flag<["-"], "enable-t

[PATCH] D135107: [clang][NFC] Use enum for -fstrict-flex-arrays

[Kees Cook via Phabricator via cfe-commits]

kees accepted this revision.
kees added a comment.
This revision is now accepted and ready to land.

L


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135107

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

[PATCH] D134902: [clang] Implement -fstrict-flex-arrays=3

[Kees Cook via Phabricator via cfe-commits]

kees added inline comments.



Comment at: clang/test/CodeGen/object-size-flex-array.c:45
+  // CHECK-STRICT-2: ret i32 -1
+  // CHECK-STRICT-3: ret i32 0
   return OBJECT_SIZE_BUILTIN(f->c, 1);

serge-sans-paille wrote:
> This one worries me a bit, as an array of size 0 is invalid C, unless you 
> consider the extension of it being a FAM. Shouldn't we emit a warning or 
> something here? @kees what meaning would you give to that construct under 
> `-fstrict-flex-arrays=3` ?
```
type identifier[0]
```
when not a fake FAM is the same as:

```
struct { } identifier
```

It's addressable with no size.

FWIW, this is how GCC is treating it, and opted for no warning. The warning 
gains nothing and is likely an irritant: if someone is requesting =3, they want 
this behavior. If they didn't, they'd just use `-Wzero-length-array`. In 
particular, the Linux kernel is in the position of needing to have zero-length 
arrays (legacy userspace API) alongside real FAMs, even if we don't reference 
the zero-length members. So we cannot use '-Wzero-length-array', but we want to 
make sure no zero-length arrays will ever be used as fake FAMs, as a code 
quality/style enforcement. This is especially true of FORTIFY_SOURCE, where 
`__bos()` needs to report 0 instead of -1 for such a destination buffer size, 
so that we immediately trip compile-time (or at worst, run-time) warnings, to 
keep any kernel internals from using the deprecated members as a fake FAM.

Take this case:

```
struct broken {
int foo;
int fake_fam[0];
struct something oops;
};
```

There have been bugs where the above struct was created because "oops" got 
added after "fake_fam" by someone not realizing. Under FORTIFY_SOURCE, doing:

```
memcpy(p->fake_fam, src, len);
```

raises no warning when `__bos(p->fake_fam, 1)` returns -1 and will happily 
stomp on "oops". If `__bos()` returns 0, we can compile-time (or run-time) 
block the memcpy. (And this holds for -fsanitize=bounds as well: if it is 
considered to be unknown size, it won't trip on access, but if it's 0-sized, 
it'll trip.) 

So, we can't keep zero-length arrays out of the kernel, but we want to be able 
to enforce that if they DO show up, they will trip warnings quickly.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134902

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

[PATCH] D135411: Add generic KCFI operand bundle lowering

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

What's the best way to test this in the kernel? I assume add 
`ARCH_SUPPORTS_CFI_CLANG` to an arch, and see what blows up? :) Have you tried 
this on any other arch yet?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135411

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

[PATCH] D135411: Add generic KCFI operand bundle lowering

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D135411#3841841 , @samitolvanen 
wrote:

> That being said, I did compile a 64-bit MIPS kernel using this pass, but I 
> didn't try booting it. I would expect to run into quite a few issues 
> initially.

I've done a test build with riscv64, and I can see the expected loads and 
literal compares. Nice!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135411

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

[PATCH] D134902: [clang] Implement -fstrict-flex-arrays=3

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D134902#3848246 , @void wrote:

> @rsmith, @serge-sans-paille, and @kees, I need some advice. There's a test in 
> `clang/test/CodeGen/bounds-checking.c` that's checking bounds stuff on 
> unions. The behavior is...weird to me. It says that an array of 0 or 1 is a 
> FAM, but one larger is not (see below).

Note that `union` vs `struct` shouldt't matter. A union is just a struct where 
all members are "trailing". ;)

`f5` looks like a broken test that didn't realize that N-sized trailing arrays 
are considered fake FAMs. This would explain some of the unexpected behavior 
I've seen with -fsanitize=bounds under Clang vs GCC:
https://godbolt.org/z/5v3evhMqq
Here GCC (correctly) accepts all as fake FAMs.

Note that GCC has an option `-fsanitize=strict-bounds` that changes the 
behavior to treating `[N]` and `[1]` as fixed size, but _not_ `[0]`. The plan 
is for GCC to make this an alias of `-fstrict-flex-arrays=2`, and then have 
UBSAN Bounds correctly tied to the `-fstrict-flex-arrays` level.

> That seems counter to how structs are handled. If this is true, then the 
> check in `clang/lib/AST/Expr.cpp` also needs to be updated...

I would expect diagnostics, `__builtin_object_size()`, 
`__builtin_dynamic_object_size()`, and `-fsanitize=bounds` to all agree on the 
definition of fake FAMs, which is all controlled by `-fstrict-flex-arrays` 
level.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134902

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

[PATCH] D134902: [clang] Implement -fstrict-flex-arrays=3

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D134902#3848595 , 
@serge-sans-paille wrote:

> I second the opinion here. C99 says nothing about flexible array member for 
> unions, that's already a "language extension". (and so not be considered as 
> FAM by `-fstrict-flex-arrays=3`)

To be super pedantic, C99 implies a FAM in a union is illegal. 6.7.2.1.16 says 
"As a special case, the last element of a structure with more than one named 
member may have an incomplete array type; this is called a flexible array 
member." The implication is that such a state ("more than one named member") 
isn't possible in a union.

But in real-world usage, this definition isn't useful and flies in the face of 
actual (fake) FAM usage. Having fake FAMs in unions is _very_ common in the 
Linux kernel, and they even appear alone in structs. There is no pragmatic 
reason for the C99 limitation, and it's needlessly enforced only for "real" 
FAMs. But this is a separate issue we can solve separately.

> Both GCC and Clang implement that extension for array of size 0 and 1, see 
> https://godbolt.org/z/1xYMYq75s. That's the *legacy* behavior of Clang.
>
> We may want to harmonize with struct behavior (for consistency etc) but I'd 
> advocate to so in a separate patch.

I just want to repeat for clarity: this isn't about union vs struct. This is 
about UBSAN vs not. Here is the same behavior, shown with a struct:
https://godbolt.org/z/4TbWYP4f9
Clang's `-fsanitize=array-bounds` is misbehaving.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134902

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

[PATCH] D135727: [clang] Correct sanitizer behavior in union FAMs

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

Change log typo? "but for all" should be "but not for all" ?




Comment at: clang/lib/AST/Expr.cpp:223
 // arrays to be treated as flexible-array-members, we still emit 
diagnostics
 // as if they are not. Pending further discussion...
+if (StrictFlexArraysLevel == FAMKind::ZeroOrIncomplete && Size.uge(1))

Isn't this commit addressing this commit, and it can be removed?



Comment at: clang/test/CodeGen/bounds-checking-fam.c:13
+
+union Zero {
+  int a[0];

I think it might be better to make this a struct, and adjust it and the other 
to include a second member to avoid the C99 issues.

```
struct Zero {
int ignored;
int a[0];
};
```

Also, I think a "real" FAM should be included as well, since its behavior 
should always be the same, regardless of `-fstrict-flex-arrays`:

```
struct FAM {
int ignored;
int a[];
}
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135727

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

[PATCH] D135727: [clang] Correct sanitizer behavior in union FAMs

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D135727#3853896 , @void wrote:

> @kees @serge-sans-paille: It appears to me that a terminating array of size > 
> 2 *isn't* treated as a FAM in Clang, at least Clang warns about it. The first 
> failure above (`clang/test/Sema/array-bounds-ptr-arith.c`) shows that. It 
> turns out that the same failure occurs in that testcase when the array isn't 
> the last in a structure, so I'll change it.

Okay, what the heck is even going on in this test? The diagnostic appears to 
think the array changes in size based on the cast??

  :16:24: warning: the pointer incremented by 80 refers past the end of 
the array (that contains 64 elements) [-Warray-bounds-pointer-arithmetic]
  return (void *)es->s_uuid + 80;
 ^~~

s_uuid is 8, not 64. 64 would be 8 "void *"s.  This seems like a very very 
broken diagnostic?

These should all trip the diagnostic, but don't:

  es->s_uuid + 8; /* this is past the end */
  (void *)es->s_uuid + 9; /* this is past the end by 1, but doesn't trip 
because it thinks it's suddenly 8 times larger */

For -Warray-bounds, GCC isn't fooled by the "void *" casts (but doesn't have 
-Warray-bounds-arithmetic):
https://godbolt.org/z/5eqEEv4of

Note that while the diagnostics of both GCC and Clang complain only about >1 
terminal arrays, they both return -1 for `__builtin_object_size` regardless of 
length.

So, we're facing, again, a disconnect between diagnostics, bos, and sanitizer. 
GCC's sanitizer follows bos rules, where-as Clang's sanitizer followed 
diagnostics rules. Given that bos is used for run-time analysis, it follows 
that sanitizer and bos should match.

> There's another failure that I'm not too sure about. The 
> `Clang.SemaCXX::array-bounds.cpp` failure is due to a union that's acting 
> like an FAM. I have a question for you. Should `a` and `c` in the union in 
> this code be considered an FAM?

This test looks correct to me:

>   struct {
> union {
>   short a[2]; // expected-note 4 {{declared here}}
>   char c[4];
> };
> int ignored;
>   };

I would **not** expect this to warn or trap because it's not the trailing 
member of the //structure//.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135727

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

[PATCH] D135727: [clang] Correct sanitizer behavior in union FAMs

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D135727#3856978 , @void wrote:

> I think we opened the can or worms. :-)

At this point I think the can we opened has worms with cans of worms with cans 
of worms. I'd say it's turtles all the way down, but it seems to just be worms 
instead. ;)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135727

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

[PATCH] D135920: [clang][Sema] Use correct array size for diagnostic

[Kees Cook via Phabricator via cfe-commits]

kees added inline comments.



Comment at: clang/test/Sema/array-bounds-ptr-arith.c:15
 {
-return (void *)es->s_uuid + 80; // expected-warning {{refers past the 
end of the array}}
+return (void *)es->s_uuid + 80; // expected-warning {{refers past the 
end of the array (that contains 8 elements)}}
 }

Can this be changed to `8` from `80` to make sure it is also doing the math 
right on the check and not just the warning text?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135920

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

[PATCH] D135920: [clang][Sema] Use correct array size for diagnostic

[Kees Cook via Phabricator via cfe-commits]

kees added inline comments.



Comment at: clang/test/SemaCXX/array-bounds.cpp:240
 
-((char*)foo)[sizeof(foo)] = '\0';  // expected-warning {{array index 32768 
is past the end of the array (which contains 32768 elements)}}
+((char*)foo)[sizeof(foo)] = '\0';  // expected-warning {{array index 32768 
is past the end of the array (which contains 4096 elements)}}
 

serge-sans-paille wrote:
> I find this new message quite confusing, because the array index is given in 
> terms of char elements, while the array size is given in terms of double 
> elements. I actually find the original message more consistent to that 
> respect.
Perhaps show both element count and byte offset?

`array index $count is $(bytes - end_of_array_in_bytes) past the end of the 
array (which contains $end_of_array_in_bytes bytes of $array_max_index 
elements)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135920

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

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays for stricter handling of flexible arrays

[Kees Cook via Phabricator via cfe-commits]

kees accepted this revision.
kees added a comment.
This revision is now accepted and ready to land.

Are the presubmit build failures unrelated?


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

https://reviews.llvm.org/D126864

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

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays for stricter handling of flexible arrays

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D126864#3582360 , @nickdesaulniers 
wrote:

> If the GCC developers split this into two distinct flags, should we land 
> something we're just going to have to turn around and modify to match the new 
> flags/semantics they've created?

I'm cautious about waiting on theoretical design choices. They appear to agree 
that a "fully strict" mode should exist, and that's what -fstrict-flex-arrays 
here gets us. I'd rather get this into Clang now so I can start working through 
all the new cases it finds instead of waiting for bikeshedding to end. If Clang 
needs to rename the option in a few months, I think that's okay. But I'll leave 
it up to @serge-sans-paille


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

https://reviews.llvm.org/D126864

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

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays for stricter handling of flexible arrays

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D126864#3584536 , 
@serge-sans-paille wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 does toward 
> `-fstrict-flex-arrays=` with
>
> - `n=0` ⇒ `-fno-strict-flex-arrays`, current state (the default)
> - `n=1` ⇒ only consider `[ 0]`, `[1}` and `[ ]` as flex array member
> - `n=2` ⇒ only consider `[ 0]` and `[ ]` as flex array member
> - `n=3` ⇒ only consider `[ ]` as flex array member
>
> I personnally like that approach, and I'd rather land that implementation.

Sounds good to me!


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

https://reviews.llvm.org/D126864

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

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D126864#3598257 , 
@serge-sans-paille wrote:

> @kees does the new version looks good to you?

Hi, yes, this is working as expected in my kernel builds. Yay! :)


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

https://reviews.llvm.org/D126864

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

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays for stricter handling of flexible arrays

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

Thanks for working on this!

Doing test builds with the Linux kernel correctly detects a number of trailing 
arrays that were being treated as flexible arrays (and need to be fixed in the 
kernel). This is exactly what was expected and wanted. :)




Comment at: clang/include/clang/Basic/LangOptions.def:425
 LANGOPT(MatrixTypes, 1, 0, "Enable or disable the builtin matrix type")
+LANGOPT(StrictFlexArrays, 1, 0, "Rely on strict definition of flexible arrays")
 

I think this option should likely also affect the logic in `-fsanitize=bounds` 
too, though I think that could be a separate change. Fixing `__bos` is more 
important. :)



Comment at: clang/test/CodeGen/object-size-flex-array.c:27
+  float f;
+  double c[2];
+} foo2_t;

Perhaps add some additional tests for "not a trailing array" here too, just for 
completeness?


```
typedef struct {
  double c[0];
  float f;
} fooM0_t;

typedef struct {
  double c[1];
  float f;
} fooM1_t;

typedef struct {
  double c[2];
  float f;
} fooM2_t;

```



Comment at: clang/test/CodeGen/object-size-flex-array.c:34
+  // CHECK: ret i32 %
+  // CHECK-STRICT: ret i32 %
+  return OBJECT_SIZE_BUILTIN(f->c, 1);

Shouldn't this explicitly return -1?


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

https://reviews.llvm.org/D126864

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

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays for stricter handling of flexible arrays

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D126864#3556262 , @efriedma wrote:

> I'm a little concerned about the premise of this, though.  See 
> https://github.com/llvm/llvm-project/issues/29694 for why we relaxed this 
> check in the first place.  I mean, the Linux kernel itself can maybe ensure 
> it isn't doing anything silly, but most code has to deal with system headers, 
> which are apparently broken.  So this option is a trap for most code.

Fixing system headers will likely come after this lands. Code bases that can 
use it (e.g. Linux kernel) will pave the way. But yes, totally agreed: it 
cannot be default-enabled.

As for SOCK_MAXADDRLEN, that's a horrid hack, and the definition of `struct 
sockaddr` needs to change. :) The Linux kernel has played games like that 
before, but we've been removing them all for saner implementations (which is 
why `-fstrict-flex-arrays` is desired: flushing out any remaining weird spots).


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

https://reviews.llvm.org/D126864

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

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays for stricter handling of flexible arrays

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D126864#3564519 , @efriedma wrote:

> Do we want some builtin define so headers can check if we're in 
> -fstrict-flex-arrays mode?  It might be hard to mess with the definitions 
> otherwise.

Hm, maybe? It wonder if that's more confusing, as code would need to do really 
careful management of allocation sizes.

  struct foo {
  u32 len;
  #ifndef STRICT_FLEX_ARRAYS
  u32 array[14];
  #else
  u32 array[];
  #endif
  };

`sizeof(struct foo)` will change. Or, even more ugly:

  struct foo {
union {
  struct {
u32 len_old;
u32 array_old[14];
  };
  struct {
u32 len;
u32 array[];
  };
};
  };

But then `sizeof(struct foo)` stays the same.

(FWIW, the kernel is effectively doing the latter without any define.)


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

https://reviews.llvm.org/D126864

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

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays for stricter handling of flexible arrays

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D126864#3567647 , @nickdesaulniers 
wrote:

> @kees maybe we should think about what would be needed for toolchains that 
> don't yet support `-fstrict-flex-arrays` in kernel sources? Does this become 
> a toolchain portability issue for older released toolchains we still intend 
> to support?

Gaining this for the kernel is just to make sure all the trailing arrays are 
actually getting found and handled. The kernel will handle this fine either 
way. (It already has to treat UAPI 1-element array conversions very 
carefully...)


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

https://reviews.llvm.org/D126864

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

[PATCH] D144136: Add a "remark" to report on array accesses

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D144136#4651030 , @void wrote:

> or can it be simplified into something like this:
>
>   array-bounds.c:341:2: remark: accessing flexible array, counted by 'count', 
> with 'index - 1' [-Rarray-bounds]
>  341 | TEST_ACCESS(p, array, index, SHOULD_TRAP);
>  | ^

Yeah, this is totally enough. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144136

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

[PATCH] D148381: [Clang] Implement the 'counted_by' attribute

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D148381#4645600 , @void wrote:

> Added more error messages. Changed some code around to align with coding 
> practices. Added some more test cases.

Doing a full "allmodconfig" Linux kernel build with my 200ish __counted_by 
annotations is building cleanly. Yay! :)




Comment at: clang/test/Sema/attr-counted-by.c:21
+  struct bar *fam[] __counted_by(non_integer); // expected-note {{counted_by 
field 'non_integer' declared here}}
+};

void wrote:
> aaron.ballman wrote:
> > Please add test with the other strange FAM-like members, as well as one 
> > that is just a trailing constant array of size 2.
> > 
> > Some more tests or situations to consider:
> > ```
> > struct S {
> >   struct {
> > int NotInThisStruct;
> >   };
> >   int FAM[] __counted_by(NotInThisStruct); // Should this work?
> > };
> > 
> > int Global;
> > struct T {
> >   int FAM[] __counted_by(Global); // Should fail per your design but is 
> > that the behavior you want?
> > };
> > 
> > struct U {
> >   struct {
> > int Count;
> >   } data;
> >   int FAM[] __counted_by(data.Count); // Thoughts?
> > };
> > 
> > struct V {
> >   int Sizes[2];
> >   int FAM[] __counted_by(Sizes[0]); // Thoughts?
> > };
> > ```
> > (I'm not suggesting any of this needs to be accepted in order to land the 
> > patch.)
> > 
> > You should also have tests showing the attribute is diagnosed when applied 
> > to something other than a field, given something other than an identifier, 
> > is given zero arguments, and is given two arguments.
> I added a "this isn't a flexible array member" error message.
> 
> ```
> struct S {
>   struct {
> int NotInThisStruct;
>   };
>   int FAM[] __counted_by(NotInThisStruct); // Should this work?
> };
> ```
> 
> Yes, I believe it should. Kees had a similar example:
> 
> ```
> struct S {
>   int x;
>   struct {
> int count;
> int FAM[] __counted_by(count);
>   };
> };
> ```
> 
> I made changes to support this. It may be controversial, so please let me 
> know if you or anyone has an issue with it.
> 
> ```
> int Global;
> struct T {
>   int FAM[] __counted_by(Global); // Should fail per your design but is that 
> the behavior you want?
> };
> ```
> 
> Yes, it should fail. I'll add a test case.
> 
> ```
> struct U {
>   struct {
> int Count;
>   } data;
>   int FAM[] __counted_by(data.Count); // Thoughts?
> };
> ```
> 
> I don't think we should support that at the moment. Referencing arbitrary 
> fields in a nested structure is very complicated and the syntax for it is not 
> at all settled on.
> 
> ```
> struct V {
>   int Sizes[2];
>   int FAM[] __counted_by(Sizes[0]); // Thoughts?
> };
> ```
> 
> Hmm...Not sure. I'll ask the GCC person who's implementing this on her side 
> about this.
> Yes, I believe it should. Kees had a similar example:
> 
> ```
> struct S {
>   int x;
>   struct {
> int count;
> int FAM[] __counted_by(count);
>   };
> };
> ```
> 
> I made changes to support this. It may be controversial, so please let me 
> know if you or anyone has an issue with it.

FWIW, the Linux kernel has a LOT of this style (in an anonymous struct), which 
is why I wanted to make sure it was supported in this first version of the 
feature.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148381

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

[PATCH] D159373: [clang][auto-init] Remove -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang

[Kees Cook via Phabricator via cfe-commits]

kees created this revision.
kees added reviewers: nickdesaulniers, aaron.ballman.
Herald added a project: All.
kees requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This was deprecated in Clang 16 and scheduled for removal in Clang 18.
Time to remove it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159373

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Driver/Options.td
  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
@@ -563,12 +563,9 @@
 // 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 %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-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 
-ftrivial-auto-var-init-stop-after=1 %s 2>&1 | FileCheck 
-check-prefix=CHECK-TRIVIAL-ZERO-STOP-AFTER %s
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -272,12 +272,6 @@
 def mmpx : Flag<["-"], "mmpx">, Group;
 def mno_mpx : Flag<["-"], "mno-mpx">, 
Group;
 
-// Retired with clang-16.0, to provide a deprecation period; it should
-// be removed in Clang 18 or later.
-def enable_trivial_var_init_zero : Flag<["-"], 
"enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang">,
-  Flags<[NoArgumentUnused]>, Visibility<[ClangOption, CC1Option, CLOption]>,
-  Group;
-
 // Group that ignores all gcc optimizations that won't be implemented
 def clang_ignored_gcc_optimization_f_Group : OptionGroup<
   "">, Group, 
Flags<[Ignored]>;
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -127,6 +127,9 @@
 Removed Compiler Flags
 -
 
+* ``-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang`` 
has been removed.
+  It has not been needed to enable ``-ftrivial-auto-var-init=zero`` since 
Clang 16.
+
 Attribute Changes in Clang
 --
 - On X86, a warning is now emitted if a function with 
``__attribute__((no_caller_saved_registers))``


Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -563,12 +563,9 @@
 // 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 %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-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 -ftrivial-auto-var-init-stop-after=1 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-STOP-AFTER %s
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -272,12 +272,6 @@
 def mmpx : Flag<["-"], "mmpx">, Group;
 def mno_mpx : Flag<["-"], "mno-mpx">, Group;
 
-// Retired with clang-16.0, to provide a deprecation period; it should
-// be removed in Clang 18 or later.
-def enable_trivial_var_init_zero : Flag<["-"], "enable-trivial-auto-var-init-zero-knowing-it-will-be-re

[PATCH] D159373: [clang][auto-init] Remove -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang

[Kees Cook via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG00e54d04ae28: [clang][auto-init] Remove 
-enable-trivial-auto-var-init-zero-knowing-it-will-be… (authored by kees).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159373

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Driver/Options.td
  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
@@ -563,12 +563,9 @@
 // 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 %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-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 
-ftrivial-auto-var-init-stop-after=1 %s 2>&1 | FileCheck 
-check-prefix=CHECK-TRIVIAL-ZERO-STOP-AFTER %s
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -272,12 +272,6 @@
 def mmpx : Flag<["-"], "mmpx">, Group;
 def mno_mpx : Flag<["-"], "mno-mpx">, 
Group;
 
-// Retired with clang-16.0, to provide a deprecation period; it should
-// be removed in Clang 18 or later.
-def enable_trivial_var_init_zero : Flag<["-"], 
"enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang">,
-  Flags<[NoArgumentUnused]>, Visibility<[ClangOption, CC1Option, CLOption]>,
-  Group;
-
 // Group that ignores all gcc optimizations that won't be implemented
 def clang_ignored_gcc_optimization_f_Group : OptionGroup<
   "">, Group, 
Flags<[Ignored]>;
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -127,6 +127,9 @@
 Removed Compiler Flags
 -
 
+* ``-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang`` 
has been removed.
+  It has not been needed to enable ``-ftrivial-auto-var-init=zero`` since 
Clang 16.
+
 Attribute Changes in Clang
 --
 - On X86, a warning is now emitted if a function with 
``__attribute__((no_caller_saved_registers))``


Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -563,12 +563,9 @@
 // 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 %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-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 -ftrivial-auto-var-init-stop-after=1 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-STOP-AFTER %s
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -272,12 +272,6 @@
 def mmpx : Flag<["-"], "mmpx">, Group;
 def mno_mpx : Flag<["-"], "mno-mpx">, Group;
 
-// Retired with clang-16.0, to provide a deprecation period; it should
-// be removed in Clang 18 or later.
-def enable_trivial_var_init_zero : Flag<["-"], "enable-trivial-auto-var-init-zero-kno

[PATCH] D148381: [Clang] Add counted_by attribute

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D148381#4639436 , @xbolva00 wrote:

> Will gcc use counted_by or element_count ?

GCC is using `__counted_by`: 
https://gcc.gnu.org/pipermail/gcc-patches/2023-August/628459.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148381

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

[PATCH] D144136: Add a "remark" to report on array accesses

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

Can you refresh this patch to work with https://reviews.llvm.org/D148381 ? My 
testing seems to imply that it doesn't know the size of the array. I assume the 
`if (!IsUnboundedArray)` check is incomplete now. i.e. for a `__counted_by` 
array, I see the "unknown" remark:

  array-bounds.c:341:2: remark: accessing unknown sized array by 'index - 1' 
[-Rarray-bounds]
341 | TEST_ACCESS(p, array, index, SHOULD_TRAP);   
| ^ 

which is from the `array-bounds.c` test cases:

  TEST_SIGNAL(counted_by_enforced_by_sanitizer, SIGILL)
  {
  struct annotated *p;
  int index = MAX_INDEX + unconst;
  
  p = alloc_annotated(index);
  
  REPORT_SIZE(p->array);
  TEST_ACCESS(p, array, index, SHOULD_TRAP);
  }




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144136

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

[PATCH] D148381: [Clang] Add counted_by attribute

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

I can generate warnings for anonymous structs were the `__counted_by` member is 
reported as "not found". For example:

  little.c:7:28: warning: counted_by field 'count' not found
  7 | int array[] __counted_by(count);
|  ^

For this:

  #define __counted_by(member)   __attribute__((__counted_by__(member)))
  
  struct anon {
  unsigned long flags;
  struct {
  unsigned char count;
  int array[] __counted_by(count);
  };
  };
  
  extern void bar(int input);
  
  void foo(struct anon *p, int index)
  {
  bar(p->array[index]);
  }

Otherwise, things are looking good on test builds so far...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148381

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

[PATCH] D144136: Add a "remark" to report on array accesses

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

This information will be useful for evaluating the coverage of the bounds 
checker for a given program, which in turn can help guide both improvements to 
the bounds checker (e.g. adding knowledge from 
`__builtin_dynamic_object_size()`) and improvements to the built code base 
(e.g. refactoring data structures to take advantage of known compile-time or 
run-time bounds that will be covered by the bounds checker). It answers the 
question, "What percentage of array accesses are being bounds checked?" (With 
the implied goal of reaching 100% going forward.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144136

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

[PATCH] D144136: Add a "remark" to report on array accesses

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

This appears to be working for me. For before/after changes, the other half is 
still needed, i.e. a "accessing array of unknown size" and eventually splitting 
the dynamic sizing check off of that one (once -fsanitize=bounds checks 
__builtin_dynamic_object_size).

For example, comparing various development builds over time, if some source had 
49 array accesses:

initial code: fixed:5 unknown:44
code refactored: fixed:10 unknown:39
bdos added to bounds checker: fixed:10 dynamic:4 unknown:35
code refactoring: fixed:10 dynamic:28 unknown:11


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144136

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

[PATCH] D144136: Add a "remark" to report on array accesses

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

Here's a test-case. I'd expect 6 remarks from building this:

  /* Build with -Wall -O2 -fstrict-flex-arrays=3 -fsanitize=bounds 
-Rarray-bounds */
  #include 
  #include 
  #include 
  #include 
  
  #define report_size(p, index)  do {\
  const size_t bdos = __builtin_dynamic_object_size(p, 1); \
  \
  if (__builtin_constant_p(bdos)) { \
  if (bdos == SIZE_MAX) { \
  printf(#p " has unknowable size\n"); \
  } else { \
  printf(#p " has a fixed size: %zu\n", bdos); \
  } \
  } else { \
  printf(#p " has a dynamic size: %zu\n", bdos); \
  } \
  printf(#p "[" #index "] assignment: %d\n", (p)[index] = 15); \
  } while (0)
  
  struct fixed {
  unsigned long flags;
  size_t foo;
  int array[16];
  };
  
  /* should emit "fixed" */
  void do_fixed(struct fixed *p, int index)
  {
  report_size(p->array, 0);
  report_size(p->array, index);
  }
  
  struct flex {
  unsigned long flags;
  size_t foo;
  int array[];
  };
  
  /* should emit "dynamic" */
  void do_dynamic(unsigned char count, int index)
  {
  /* malloc() is marked with __attribute__((alloc_size(1))) */
  struct flex *p = malloc(sizeof(*p) + count * sizeof(*p->array));
  report_size(p->array, 0);
  report_size(p->array, index);
  free(p);
  }
  
  /* should emit "unknowable" */
  void do_unknown(struct flex *p, int index)
  {
  report_size(p->array, 0);
  report_size(p->array, index);
  }

Currently, it only emits once for the compile-time known index with a 
compile-time known array size:

  array.c:31:17: remark: accessing fixed sized array 'int[16]' by 0 
[-Rarray-bounds]  
  report_size(p->array, 0); 
  ^ 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144136

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

[PATCH] D144136: Add a "remark" to report on array accesses

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

This gets me all 6 reports. The details about the array and the index don't 
really matter for the basic metrics:

  diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/Diagnostic
  SemaKinds.td
  index ba831c026342..29d2167b504b 100644
  --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
  +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
  @@ -9451,7 +9451,7 @@ def note_array_declared_here : Note<
   
 "array %0 declared here">;

   def remark_array_access : Remark<
  -  "accessing %select{fixed|dynamic}0 sized array %1 by %2">,
  +  "accessing %select{fixed|unknown|dynamic}0 sized array %1 by %2">,
 InGroup;   
  

   def warn_inconsistent_array_form : Warning<
  diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
  index 9ced29a5f5d0..1c6aa7f05c7f 100644   
  --- a/clang/lib/Sema/SemaChecking.cpp 
  
  +++ b/clang/lib/Sema/SemaChecking.cpp
  @@ -16207,8 +16207,26 @@ void Sema::CheckArrayAccess(const Expr *BaseExpr, 
const Expr *IndexExpr,   
   return;  

 Expr::EvalResult Result;
  -  if (!IndexExpr->EvaluateAsInt(Result, Context, Expr::SE_AllowSideEffects))
  +  if (!IndexExpr->EvaluateAsInt(Result, Context, Expr::SE_AllowSideEffects)) 
{
  +SmallString<128> sizeString;
  +llvm::raw_svector_ostream OS(sizeString);
  +
  +OS << "'";
  +IndexExpr->printPretty(OS, nullptr, getPrintingPolicy());
  +OS << "'";
  +
  +if (!IsUnboundedArray) {
  +  Context.getDiagnostics().Report(
  +  BaseExpr->getBeginLoc(), diag::remark_array_access)
  +  << 0 << ArrayTy->desugar() << OS.str();
  +} else {
  +  Context.getDiagnostics().Report(
  +  BaseExpr->getBeginLoc(), diag::remark_array_access)
  +  << 1 << "something" << OS.str();
  +}
  +
   return;
  +  }

 llvm::APSInt index = Result.Val.getInt();
 if (IndexNegated) {
  @@ -16219,6 +16237,11 @@ void Sema::CheckArrayAccess(const Expr *BaseExpr, 
const Expr *IndexExpr,
 if (IsUnboundedArray) {
   if (EffectiveType->isFunctionType())
 return;
  +
  +Context.getDiagnostics().Report(
  +BaseExpr->getBeginLoc(), diag::remark_array_access)
  +<< 1 << "something" << "whatever";
  +
   if (index.isUnsigned() || !index.isNegative()) {
 const auto &ASTC = getASTContext();
 unsigned AddrBits = ASTC.getTargetInfo().getPointerWidth(

Using "desugar" on a flexible array appears to blow up. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144136

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

[PATCH] D148381: [WIP][Clang] Add element_count attribute

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

This is great! It passes my own testing for the sanitizer too. I'm looking 
forward to __bdos support. :)

FWIW, similar progress is being made on the GCC side:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108896


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148381

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

[PATCH] D119816: [SanitizerBounds] Add support for NoSanitizeBounds function

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

FWIW, related problems with `pskb_expand_head` were seen again here:
https://github.com/ClangBuiltLinux/linux/issues/1599

I have trouble reproducing it, but I think the kernel patch there solves the 
problem created by `__alloc_size` vs `ksize()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119816

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