[PATCH] D110656: [clang][Sema] Warn on uninitialized array elments

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D110656#3034971 , @beanz wrote:

> In D110656#3034083 , @xbolva00 
> wrote:
>
>> Why just no special case "= {0};" pattern and do not warn in that case?
>
> This case did show up, but was not the majority of issues for LLVM (although 
> might be the majority we care about). The related `= {nullptr};` pattern also 
> shows a few times.
>
>> If there are more patterns, take it from other side - pick patterns where 
>> you are sure that they are wrong and developer needs to fix them and emit 
>> warnings for them.
>
> There are a bunch of places (particularly in LLVM instruction matching), 
> where LLVM generates data structures with statically sized arrays that hold 
> the maximum possible values, and rely on zero-initialized values to denote 
> the end of the used space.
>
> For the tablegen-genreated code wrapping in `pragma`s to disable the warning 
> works, but there isn't really a good way that I can think of to not emit a 
> warning for an array of 10 elements where we provide 6 in a general case 
> while also warning on the actual potential bug.

This was the kind of thing I was worried about. I expect that this pattern also 
shows up (perhaps less frequently) in non-generated code as well.

> In D110656#3034149 , @xbolva00 
> wrote:
>
>> If possible, @beanz could share list of new warnings from LLVM build here?
>
> The big offenders were the tablegen-generated asm and register matchers and 
> the target files under clang/lib/Basic/Targets. Those accounted for filling 
> my scroll back buffer a few times over...
>
> I'm running a new build now disabling the warning int he clang target files 
> to see how that shapes up. Will share what I find.

Thanks, I'm curious to hear what you find!

Another alternative to surfacing this as a frontend warning is to make a 
`bugprone` check for clang-tidy as the false positive rate seems likely to be 
more suited to that tool.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110656

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

[PATCH] D110656: [clang][Sema] Warn on uninitialized array elments

[Nathan Chancellor via Phabricator via cfe-commits]

nathanchance added a comment.

In D110656#3034181 , @nathanchance 
wrote:

> I can test this on the Linux kernel tonight and report the results tomorrow 
> morning.

My apologies, I did not have time to get to this last night or today and I am 
going to be on vacation starting today for a week and a half. I can report back 
once I am back.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110656

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

[PATCH] D110656: [clang][Sema] Warn on uninitialized array elments

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

In D110656#3034083 , @xbolva00 wrote:

> Why just no special case "= {0};" pattern and do not warn in that case?

This case did show up, but was not the majority of issues for LLVM (although 
might be the majority we care about). The related `= {nullptr};` pattern also 
shows a few times.

> If there are more patterns, take it from other side - pick patterns where you 
> are sure that they are wrong and developer needs to fix them and emit 
> warnings for them.

There are a bunch of places (particularly in LLVM instruction matching), where 
LLVM generates data structures with statically sized arrays that hold the 
maximum possible values, and rely on zero-initialized values to denote the end 
of the used space.

For the tablegen-genreated code wrapping in `pragma`s to disable the warning 
works, but there isn't really a good way that I can think of to not emit a 
warning for an array of 10 elements where we provide 6 in a general case while 
also warning on the actual potential bug.

In D110656#3034149 , @xbolva00 wrote:

> If possible, @beanz could share list of new warnings from LLVM build here?

The big offenders were the tablegen-generated asm and register matchers and the 
target files under clang/lib/Basic/Targets. Those accounted for filling my 
scroll back buffer a few times over...

I'm running a new build now disabling the warning int he clang target files to 
see how that shapes up. Will share what I find.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110656

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

[PATCH] D110656: [clang][Sema] Warn on uninitialized array elments

[Nathan Chancellor via Phabricator via cfe-commits]

nathanchance added a comment.

I can test this on the Linux kernel tonight and report the results tomorrow 
morning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110656

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

[PATCH] D110656: [clang][Sema] Warn on uninitialized array elments

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a subscriber: nathanchance.
xbolva00 added a comment.

>> If there are other patterns, I'd love to know what they are.

Well, I dont know :) but something may arise so some testing would be useful.

If possible, @beanz could share list of new warnings from LLVM build here? Also 
maybe you can try this with linux kernel? Or with cooperation with 
@nathanchance..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110656

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

[PATCH] D110656: [clang][Sema] Warn on uninitialized array elments

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D110656#3034083 , @xbolva00 wrote:

> Why just no special case "= {0};" pattern and do not warn in that case?

This was what I was thinking. I was basing that on the idea that `= { 0 }` to 
zero init the entire array is far more common than ` = { 1 }` to init the first 
element to one and the rest to zero (which is a case I think this diagnostic 
shines on because some users think that will fill all `1` instead of only the 
first element).

> If there are more patterns, take it from other side - pick patterns where you 
> are sure that they are wrong and developer needs to fix them and emit 
> warnings for them.

If there are other patterns, I'd love to know what they are.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110656

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

[PATCH] D110656: [clang][Sema] Warn on uninitialized array elments

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Why just no special case "= {0};" pattern and do not warn in that case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110656

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

[PATCH] D110656: [clang][Sema] Warn on uninitialized array elments

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

Even just building Clang with this enabled revealed... a lot... Some of them 
are easy to address (having tablegen disable the warning in generated code), 
some of them are exactly the kind of thing I wanted to warn on, and some of 
them are more complicated.

Based on that alone, I don't think this would be reasonable to enable by 
default :(

Thoughts on how to proceed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110656

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

[PATCH] D110656: [clang][Sema] Warn on uninitialized array elments

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D110656#3030580 , @beanz wrote:

> I would greatly prefer to enable this by default, so let me build a toolchain 
> and see how it holds up.

Awesome, SGTM!

> The empty initializer list extension is pretty widely supported, so I single 
> zero-initialization is way less common these days, but I'll look.

It may be widely supported, but there's plenty of single-zero-inits I was 
finding on that code search:

  static guint notification_signals[LAST_SIGNAL] = { 0 };
  charport_str[MAX_PORT_STR_LEN+1] = {0};
  char aBuf[64] = {0};
  unsigned char strong_checksum[SHA256_DIGEST_LENGTH] = {0};
  unsigned long zones_size[MAX_NR_ZONES] = {0};
  unsigned long zholes_size[MAX_NR_ZONES] = {0};
  // and so on


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110656

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

[PATCH] D110656: [clang][Sema] Warn on uninitialized array elments

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

I would greatly prefer to enable this by default, so let me build a toolchain 
and see how it holds up.

The empty initializer list extension is pretty widely supported, so I single 
zero-initialization is way less common these days, but I'll look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110656

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

[PATCH] D110656: [clang][Sema] Warn on uninitialized array elments

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

We typically do not introduce new off-by-default warnings into Clang because 
experience has shown that users typically do not enable them (so they tend not 
to be worth the maintenance burden). Instead, we try to make warnings that can 
be on-by-default with a very low false positive rate. Have you run over a large 
corpus of C and C++ code to see what false positives arise? Do you have ideas 
on what it would take to make this on by default? The one big one I can think 
of is that it's not at all uncommon in C to only initialize one element of the 
array to zero and rely on zero initialization of the rest (because an empty 
initializer list is technically not valid C code; it's a GNU extension we 
support): 
https://codesearch.isocpp.org/cgi-bin/cgi_ppsearch?q=%3D+%7B0%7D%3B=Search.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110656

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

[PATCH] D110656: [clang][Sema] Warn on uninitialized array elments

[Chris Bieneman via Phabricator via cfe-commits]

beanz updated this revision to Diff 375711.
beanz added a comment.

Updating so that the warning doesn't fire on an empty initializer list.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110656

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaInit.cpp
  clang/test/Sema/missing-array-initializers.c


Index: clang/test/Sema/missing-array-initializers.c
===
--- /dev/null
+++ clang/test/Sema/missing-array-initializers.c
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wmissing-array-initializers %s
+
+// We should only warn on an explicitly sized initializer for an array
+// specifier. This means no warning on arrays without size, and no warning on
+// string literal specfiers (even if the string literal has a size).
+
+// not-expected-warning@+1 {{missing array initializer}}
+char Doggo[] = "Doggo";
+
+// not-expected-warning@+1 {{missing array initializer}}
+char Pupper[16] = "Pupper";
+
+// expected-warning@+1 {{missing array initializer: expected 16 elements, 
found 7}}
+char Floofer[16] = {'F', 'L', 'O', 'O', 'F', 'E', 'R'};
+
+// not-expected-warning@+1 {{missing array initializer}}
+int Array[] = {1, 2, 3, 4, 5};
+
+// expected-warning@+1 {{missing array initializer: expected 10 elements, 
found 5}}
+int SizedArray[10] = {1, 2, 3, 4, 5};
+
+// Don't fire on zero-initialized shorthand either
+// not-expected-warning@+1 {{missing array initializer}}
+int ZeroInitialized[10] = {};
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -1991,6 +1991,12 @@
   CheckEmptyInitializable(
   InitializedEntity::InitializeElement(SemaRef.Context, 0, Entity),
   IList->getEndLoc());
+if (!VerifyOnly && maxElementsKnown && elementIndex > 0 &&
+elementIndex < maxElements) {
+  SemaRef.Diag(IList->getEndLoc(), diag::warn_missing_array_initializers)
+  << (unsigned)maxElements.getExtValue()
+  << (unsigned)elementIndex.getExtValue();
+}
   }
 }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5797,6 +5797,9 @@
 def warn_missing_field_initializers : Warning<
   "missing field %0 initializer">,
   InGroup, DefaultIgnore;
+def warn_missing_array_initializers : Warning<
+  "missing array initializer: expected %0 elements, found %1">,
+  InGroup, DefaultIgnore;
 def warn_braces_around_init : Warning<
   "braces around %select{scalar |}0initializer">,
   InGroup>;
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -466,6 +466,7 @@
 def MismatchedReturnTypes : DiagGroup<"mismatched-return-types">;
 def MismatchedTags : DiagGroup<"mismatched-tags">;
 def MissingFieldInitializers : DiagGroup<"missing-field-initializers">;
+def MissingArrayInitializers : DiagGroup<"missing-array-initializers">;
 def ModuleLock : DiagGroup<"module-lock">;
 def ModuleBuild : DiagGroup<"module-build">;
 def ModuleImport : DiagGroup<"module-import">;


Index: clang/test/Sema/missing-array-initializers.c
===
--- /dev/null
+++ clang/test/Sema/missing-array-initializers.c
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wmissing-array-initializers %s
+
+// We should only warn on an explicitly sized initializer for an array
+// specifier. This means no warning on arrays without size, and no warning on
+// string literal specfiers (even if the string literal has a size).
+
+// not-expected-warning@+1 {{missing array initializer}}
+char Doggo[] = "Doggo";
+
+// not-expected-warning@+1 {{missing array initializer}}
+char Pupper[16] = "Pupper";
+
+// expected-warning@+1 {{missing array initializer: expected 16 elements, found 7}}
+char Floofer[16] = {'F', 'L', 'O', 'O', 'F', 'E', 'R'};
+
+// not-expected-warning@+1 {{missing array initializer}}
+int Array[] = {1, 2, 3, 4, 5};
+
+// expected-warning@+1 {{missing array initializer: expected 10 elements, found 5}}
+int SizedArray[10] = {1, 2, 3, 4, 5};
+
+// Don't fire on zero-initialized shorthand either
+// not-expected-warning@+1 {{missing array initializer}}
+int ZeroInitialized[10] = {};
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -1991,6 +1991,12 @@
   CheckEmptyInitializable(
   InitializedEntity::InitializeElement(SemaRef.Context, 0, 

[PATCH] D110656: [clang][Sema] Warn on uninitialized array elments

[Chris Bieneman via Phabricator via cfe-commits]

beanz created this revision.
beanz added reviewers: aaron.ballman, rsmith, RKSimon, dexonsmith.
beanz requested review of this revision.
Herald added a project: clang.

When defining a statically sized array with explicit array
initializers, having a warning for uninitialized members is nice.

This warning will only fire on missing elements in an explicitly sized
array initialized with an array initializer. It will not fire on char
arrays that are initialized using a string literal sequence.

This warning is added under a new warning group
-Wmissing-array-initializers.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D110656

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaInit.cpp
  clang/test/Sema/missing-array-initializers.c


Index: clang/test/Sema/missing-array-initializers.c
===
--- /dev/null
+++ clang/test/Sema/missing-array-initializers.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wmissing-array-initializers %s
+
+// We should only warn on an explicitly sized initializer for an array
+// specifier. This means no warning on arrays without size, and no warning on
+// string literal specfiers (even if the string literal has a size).
+
+// not-expected-warning@+1 {{missing array initializer}}
+char Doggo[] = "Doggo";
+
+// not-expected-warning@+1 {{missing array initializer}}
+char Pupper[16] = "Pupper";
+
+// expected-warning@+1 {{missing array initializer: expected 16 elements, 
found 7}}
+char Floofer[16] = {'F', 'L', 'O', 'O', 'F', 'E', 'R'};
+
+// not-expected-warning@+1 {{missing array initializer}}
+int Array[] = {1, 2, 3, 4, 5};
+
+// expected-warning@+1 {{missing array initializer: expected 10 elements, 
found 5}}
+int SizedArray[10] = {1, 2, 3, 4, 5};
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -1991,6 +1991,11 @@
   CheckEmptyInitializable(
   InitializedEntity::InitializeElement(SemaRef.Context, 0, Entity),
   IList->getEndLoc());
+if (!VerifyOnly && maxElementsKnown && elementIndex < maxElements) {
+  SemaRef.Diag(IList->getEndLoc(), diag::warn_missing_array_initializers)
+  << (unsigned)maxElements.getExtValue()
+  << (unsigned)elementIndex.getExtValue();
+}
   }
 }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5797,6 +5797,9 @@
 def warn_missing_field_initializers : Warning<
   "missing field %0 initializer">,
   InGroup, DefaultIgnore;
+def warn_missing_array_initializers : Warning<
+  "missing array initializer: expected %0 elements, found %1">,
+  InGroup, DefaultIgnore;
 def warn_braces_around_init : Warning<
   "braces around %select{scalar |}0initializer">,
   InGroup>;
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -466,6 +466,7 @@
 def MismatchedReturnTypes : DiagGroup<"mismatched-return-types">;
 def MismatchedTags : DiagGroup<"mismatched-tags">;
 def MissingFieldInitializers : DiagGroup<"missing-field-initializers">;
+def MissingArrayInitializers : DiagGroup<"missing-array-initializers">;
 def ModuleLock : DiagGroup<"module-lock">;
 def ModuleBuild : DiagGroup<"module-build">;
 def ModuleImport : DiagGroup<"module-import">;


Index: clang/test/Sema/missing-array-initializers.c
===
--- /dev/null
+++ clang/test/Sema/missing-array-initializers.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wmissing-array-initializers %s
+
+// We should only warn on an explicitly sized initializer for an array
+// specifier. This means no warning on arrays without size, and no warning on
+// string literal specfiers (even if the string literal has a size).
+
+// not-expected-warning@+1 {{missing array initializer}}
+char Doggo[] = "Doggo";
+
+// not-expected-warning@+1 {{missing array initializer}}
+char Pupper[16] = "Pupper";
+
+// expected-warning@+1 {{missing array initializer: expected 16 elements, found 7}}
+char Floofer[16] = {'F', 'L', 'O', 'O', 'F', 'E', 'R'};
+
+// not-expected-warning@+1 {{missing array initializer}}
+int Array[] = {1, 2, 3, 4, 5};
+
+// expected-warning@+1 {{missing array initializer: expected 10 elements, found 5}}
+int SizedArray[10] = {1, 2, 3, 4, 5};
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -1991,6 +1991,11 @@
   CheckEmptyInitializable(