[v2] C, ObjC: Add -Wunterminated-string-initialization
Checks
Commit Message
Warn about the following:
char s[3] = "foo";
Initializing a char array with a string literal of the same length as
the size of the array is usually a mistake. Rarely is the case where
one wants to create a non-terminated character sequence from a string
literal.
In some cases, for writing faster code, one may want to use arrays
instead of pointers, since that removes the need for storing an array of
pointers apart from the strings themselves.
char *log_levels[] = { "info", "warning", "err" };
vs.
char log_levels[][7] = { "info", "warning", "err" };
This forces the programmer to specify a size, which might change if a
new entry is later added. Having no way to enforce null termination is
very dangerous, however, so it is useful to have a warning for this, so
that the compiler can make sure that the programmer didn't make any
mistakes. This warning catches the bug above, so that the programmer
will be able to fix it and write:
char log_levels[][8] = { "info", "warning", "err" };
This warning already existed as part of -Wc++-compat, but this patch
allows enabling it separately. It is also included in -Wextra, since
it may not always be desired (when unterminated character sequences are
wanted), but it's likely to be desired in most cases.
Link: <https://lists.gnu.org/archive/html/groff/2022-11/msg00059.html>
Link: <https://lists.gnu.org/archive/html/groff/2022-11/msg00063.html>
Link: <https://inbox.sourceware.org/gcc/36da94eb-1cac-5ae8-7fea-ec66160cf413@gmail.com/T/>
Acked-by: Doug McIlroy <douglas.mcilroy@dartmouth.edu>
Cc: "G. Branden Robinson" <g.branden.robinson@gmail.com>
Cc: Ralph Corderoy <ralph@inputplus.co.uk>
Cc: Dave Kemper <saint.snit@gmail.com>
Cc: Larry McVoy <lm@mcvoy.com>
Cc: Andrew Pinski <pinskia@gmail.com>
Cc: Jonathan Wakely <jwakely.gcc@gmail.com>
Cc: Andrew Clayton <andrew@digital-domain.net>
Cc: Martin Uecker <muecker@gwdg.de>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
Hi,
I sent v1 to the wrong list. This time I've made sure to write to
gcc-patches@.
v2 adds some draft of a test, as suggested by Martin. However, I don't
know yet how to write those, so the test is just a draft. But I did
test the feature, by compiling GCC and compiling some small program with
it.
Cheers,
Alex
Range-diff against v1:
1: 61ddf1eb816 ! 1: e40d8f54942 C, ObjC: Add -Wunterminated-string-initialization
@@ Commit message
Cc: Andrew Pinski <pinskia@gmail.com>
Cc: Jonathan Wakely <jwakely.gcc@gmail.com>
Cc: Andrew Clayton <andrew@digital-domain.net>
+ Cc: Martin Uecker <muecker@gwdg.de>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
## gcc/c-family/c.opt ##
@@ gcc/c/c-typeck.cc: digest_init (location_t init_loc, tree type, tree init, tree
if (compare_tree_int (TYPE_SIZE_UNIT (type), len) < 0)
{
unsigned HOST_WIDE_INT size
+
+ ## gcc/testsuite/gcc.dg/Wunterminated-string-initialization.c (new) ##
+@@
++/* { dg-do compile } */
++/* { dg-options "-Wunterminated-string-initialization" } */
++
++char a1[] = "a";
++char a2[1] = "a"; /* { dg-warning "unterminated char sequence" } */
++char a3[2] = "a";
gcc/c-family/c.opt | 4 ++++
gcc/c/c-typeck.cc | 6 +++---
gcc/testsuite/gcc.dg/Wunterminated-string-initialization.c | 6 ++++++
3 files changed, 13 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/Wunterminated-string-initialization.c
Comments
On Fri, 2023-03-24 at 14:39 +0100, Alejandro Colomar via Gcc-patches
wrote:
> Warn about the following:
>
> char s[3] = "foo";
>
> Initializing a char array with a string literal of the same length as
> the size of the array is usually a mistake. Rarely is the case where
> one wants to create a non-terminated character sequence from a string
> literal.
>
> In some cases, for writing faster code, one may want to use arrays
> instead of pointers, since that removes the need for storing an array
> of
> pointers apart from the strings themselves.
>
> char *log_levels[] = { "info", "warning", "err" };
> vs.
> char log_levels[][7] = { "info", "warning", "err" };
>
> This forces the programmer to specify a size, which might change if a
> new entry is later added. Having no way to enforce null termination
> is
> very dangerous, however, so it is useful to have a warning for this,
> so
> that the compiler can make sure that the programmer didn't make any
> mistakes. This warning catches the bug above, so that the programmer
> will be able to fix it and write:
>
> char log_levels[][8] = { "info", "warning", "err" };
>
> This warning already existed as part of -Wc++-compat, but this patch
> allows enabling it separately. It is also included in -Wextra, since
> it may not always be desired (when unterminated character sequences
> are
> wanted), but it's likely to be desired in most cases.
>
> Link:
> <https://lists.gnu.org/archive/html/groff/2022-11/msg00059.html>
> Link:
> <https://lists.gnu.org/archive/html/groff/2022-11/msg00063.html>
> Link:
> <https://inbox.sourceware.org/gcc/36da94eb-1cac-5ae8-7fea-ec66160cf41
> 3@gmail.com/T/>
> Acked-by: Doug McIlroy <douglas.mcilroy@dartmouth.edu>
> Cc: "G. Branden Robinson" <g.branden.robinson@gmail.com>
> Cc: Ralph Corderoy <ralph@inputplus.co.uk>
> Cc: Dave Kemper <saint.snit@gmail.com>
> Cc: Larry McVoy <lm@mcvoy.com>
> Cc: Andrew Pinski <pinskia@gmail.com>
> Cc: Jonathan Wakely <jwakely.gcc@gmail.com>
> Cc: Andrew Clayton <andrew@digital-domain.net>
> Cc: Martin Uecker <muecker@gwdg.de>
> Signed-off-by: Alejandro Colomar <alx@kernel.org>
> ---
>
> Hi,
Hi Alex, thanks for the patch.
>
> I sent v1 to the wrong list. This time I've made sure to write to
> gcc-patches@.
Note that right now we're deep in bug-fixing/stabilization for GCC 13
(and trunk is still tracking that effort), so your patch might be more
suitable for GCC 14.
>
> v2 adds some draft of a test, as suggested by Martin. However, I
> don't
> know yet how to write those, so the test is just a draft. But I did
> test the feature, by compiling GCC and compiling some small program
> with
> it.
Unfortunately the answer to the question "how do I run just one
testcase in GCC's testsuite" is rather non-trivial; FWIW I've written
up some notes on working with the GCC testsuite here:
https://gcc-newbies-guide.readthedocs.io/en/latest/working-with-the-testsuite.html
Hope this is helpful
Dave
[...snip...]
Hi David,
On 3/24/23 15:53, David Malcolm wrote:
> On Fri, 2023-03-24 at 14:39 +0100, Alejandro Colomar via Gcc-patches
> wrote:
>> Warn about the following:
>>
>> char s[3] = "foo";
>>
[...]
>> ---
>>
>> Hi,
>
> Hi Alex, thanks for the patch.
:)
>
>>
>> I sent v1 to the wrong list. This time I've made sure to write to
>> gcc-patches@.
>
> Note that right now we're deep in bug-fixing/stabilization for GCC 13
> (and trunk is still tracking that effort), so your patch might be more
> suitable for GCC 14.
Sure, no problem. Do you have a "next" branch where you pick patches
for after the release, or should I resend after the release? Is
discussion of a patch reasonable now, or is it too much distracting
from your stabilization efforts?
>
>>
>> v2 adds some draft of a test, as suggested by Martin. However, I
>> don't
>> know yet how to write those, so the test is just a draft. But I did
>> test the feature, by compiling GCC and compiling some small program
>> with
>> it.
>
> Unfortunately the answer to the question "how do I run just one
> testcase in GCC's testsuite" is rather non-trivial; FWIW I've written
> up some notes on working with the GCC testsuite here:
> https://gcc-newbies-guide.readthedocs.io/en/latest/working-with-the-testsuite.html
Hmm, I'll try following that; thanks! Is there anything obvious that
I might have missed, at first glance?
>
> Hope this is helpful
Yup. Thanks!
> Dave
Cheers,
Alex
On Fri, 2023-03-24 at 18:45 +0100, Alejandro Colomar wrote:
> Hi David,
>
> On 3/24/23 15:53, David Malcolm wrote:
> > On Fri, 2023-03-24 at 14:39 +0100, Alejandro Colomar via Gcc-
> > patches
> > wrote:
> > > Warn about the following:
> > >
> > > char s[3] = "foo";
> > >
> [...]
>
> > > ---
> > >
> > > Hi,
> >
> > Hi Alex, thanks for the patch.
>
> :)
>
> >
> > >
> > > I sent v1 to the wrong list. This time I've made sure to write
> > > to
> > > gcc-patches@.
> >
> > Note that right now we're deep in bug-fixing/stabilization for GCC
> > 13
> > (and trunk is still tracking that effort), so your patch might be
> > more
> > suitable for GCC 14.
>
> Sure, no problem. Do you have a "next" branch where you pick patches
> for after the release, or should I resend after the release?
We don't; resending it after release is probably best.
> Is
> discussion of a patch reasonable now, or is it too much distracting
> from your stabilization efforts?
FWIW I'd prefer to postpone the discussion until after we branch for
the release.
>
> >
> > >
> > > v2 adds some draft of a test, as suggested by Martin. However, I
> > > don't
> > > know yet how to write those, so the test is just a draft. But I
> > > did
> > > test the feature, by compiling GCC and compiling some small
> > > program
> > > with
> > > it.
> >
> > Unfortunately the answer to the question "how do I run just one
> > testcase in GCC's testsuite" is rather non-trivial; FWIW I've
> > written
> > up some notes on working with the GCC testsuite here:
> > https://gcc-newbies-guide.readthedocs.io/en/latest/working-with-the-testsuite.html
>
> Hmm, I'll try following that; thanks! Is there anything obvious that
> I might have missed, at first glance?
The main thing is that there's a difference between compiling the test
case "by hand", versus doing it through the test harness - the latter
sets up the environment in a particular way, injects a particular set
of flags, etc etc.
Dave
Hi David,
On 3/24/23 18:58, David Malcolm wrote:
> On Fri, 2023-03-24 at 18:45 +0100, Alejandro Colomar wrote:
>> Hi David,
>>
>> On 3/24/23 15:53, David Malcolm wrote:
>>> On Fri, 2023-03-24 at 14:39 +0100, Alejandro Colomar via Gcc-
>>> patches
>>> wrote:
>>>> Warn about the following:
>>>>
>>>> char s[3] = "foo";
>>>>
>> [...]
>>
>>>> ---
>>>>
>>>> Hi,
>>>
>>> Hi Alex, thanks for the patch.
>>
>> :)
>>
>>>
>>>>
>>>> I sent v1 to the wrong list. This time I've made sure to write
>>>> to
>>>> gcc-patches@.
>>>
>>> Note that right now we're deep in bug-fixing/stabilization for GCC
>>> 13
>>> (and trunk is still tracking that effort), so your patch might be
>>> more
>>> suitable for GCC 14.
>>
>> Sure, no problem. Do you have a "next" branch where you pick patches
>> for after the release, or should I resend after the release?
>
> We don't; resending it after release is probably best.
>
>> Is
>> discussion of a patch reasonable now, or is it too much distracting
>> from your stabilization efforts?
>
> FWIW I'd prefer to postpone the discussion until after we branch for
> the release.
Sure. AFAIK it's fair game already to propose patches to GCC 14,
right? Would you please have a look into this? Thanks!
>
>>
>>>
>>>>
>>>> v2 adds some draft of a test, as suggested by Martin. However, I
>>>> don't
>>>> know yet how to write those, so the test is just a draft. But I
>>>> did
>>>> test the feature, by compiling GCC and compiling some small
>>>> program
>>>> with
>>>> it.
>>>
>>> Unfortunately the answer to the question "how do I run just one
>>> testcase in GCC's testsuite" is rather non-trivial; FWIW I've
>>> written
>>> up some notes on working with the GCC testsuite here:
>>> https://gcc-newbies-guide.readthedocs.io/en/latest/working-with-the-testsuite.html
>>
>> Hmm, I'll try following that; thanks! Is there anything obvious that
>> I might have missed, at first glance?
>
> The main thing is that there's a difference between compiling the test
> case "by hand", versus doing it through the test harness - the latter
> sets up the environment in a particular way, injects a particular set
> of flags, etc etc.
I forgot about this; I'll have a look into it when I find some time.
Cheers,
Alex
>
> Dave
>
Hi David,
Sorry for the half-year delay! I have some update. :)
On Fri, Mar 24, 2023 at 10:53:22AM -0400, David Malcolm wrote:
> On Fri, 2023-03-24 at 14:39 +0100, Alejandro Colomar via Gcc-patches
> wrote:
> > Warn about the following:
> >
> > char s[3] = "foo";
> >
> > Initializing a char array with a string literal of the same length as
> > the size of the array is usually a mistake. Rarely is the case where
> > one wants to create a non-terminated character sequence from a string
> > literal.
> >
> > In some cases, for writing faster code, one may want to use arrays
> > instead of pointers, since that removes the need for storing an array
> > of
> > pointers apart from the strings themselves.
> >
> > char *log_levels[] = { "info", "warning", "err" };
> > vs.
> > char log_levels[][7] = { "info", "warning", "err" };
> >
> > This forces the programmer to specify a size, which might change if a
> > new entry is later added. Having no way to enforce null termination
> > is
> > very dangerous, however, so it is useful to have a warning for this,
> > so
> > that the compiler can make sure that the programmer didn't make any
> > mistakes. This warning catches the bug above, so that the programmer
> > will be able to fix it and write:
> >
> > char log_levels[][8] = { "info", "warning", "err" };
> >
> > This warning already existed as part of -Wc++-compat, but this patch
> > allows enabling it separately. It is also included in -Wextra, since
> > it may not always be desired (when unterminated character sequences
> > are
> > wanted), but it's likely to be desired in most cases.
> >
> > Link:
> > <https://lists.gnu.org/archive/html/groff/2022-11/msg00059.html>
> > Link:
> > <https://lists.gnu.org/archive/html/groff/2022-11/msg00063.html>
> > Link:
> > <https://inbox.sourceware.org/gcc/36da94eb-1cac-5ae8-7fea-ec66160cf41
> > 3@gmail.com/T/>
> > Acked-by: Doug McIlroy <douglas.mcilroy@dartmouth.edu>
> > Cc: "G. Branden Robinson" <g.branden.robinson@gmail.com>
> > Cc: Ralph Corderoy <ralph@inputplus.co.uk>
> > Cc: Dave Kemper <saint.snit@gmail.com>
> > Cc: Larry McVoy <lm@mcvoy.com>
> > Cc: Andrew Pinski <pinskia@gmail.com>
> > Cc: Jonathan Wakely <jwakely.gcc@gmail.com>
> > Cc: Andrew Clayton <andrew@digital-domain.net>
> > Cc: Martin Uecker <muecker@gwdg.de>
> > Signed-off-by: Alejandro Colomar <alx@kernel.org>
> > ---
> >
> > Hi,
>
> Hi Alex, thanks for the patch.
>
> >
> > I sent v1 to the wrong list. This time I've made sure to write to
> > gcc-patches@.
>
> Note that right now we're deep in bug-fixing/stabilization for GCC 13
> (and trunk is still tracking that effort), so your patch might be more
> suitable for GCC 14.
>
> >
> > v2 adds some draft of a test, as suggested by Martin. However, I
> > don't
> > know yet how to write those, so the test is just a draft. But I did
> > test the feature, by compiling GCC and compiling some small program
> > with
> > it.
>
> Unfortunately the answer to the question "how do I run just one
> testcase in GCC's testsuite" is rather non-trivial; FWIW I've written
> up some notes on working with the GCC testsuite here:
> https://gcc-newbies-guide.readthedocs.io/en/latest/working-with-the-testsuite.html
>
> Hope this is helpful
> Dave
>
>
> [...snip...]
>
I ran the tests, and get some unexpected failure. I used dg-warning,
but maybe I used it wrong? Here's the output:
```
output is:
/home/alx/src/gnu/gcc/wustr/gcc/testsuite/gcc.dg/Wunterminated-string-initialization.c:5:14: warning: initializer-string for array of 'char' is too long [-Wunterminated-string-initi
alization]
FAIL: gcc.dg/Wunterminated-string-initialization.c (test for warnings, line 5)
```
And here's the test:
```
/* { dg-do compile } */
/* { dg-options "-Wunterminated-string-initialization" } */
char a1[] = "a";
char a2[1] = "a"; /* { dg-warning "unterminated char sequence" } */
char a3[2] = "a";
```
Why isn't it expecting the warning?
Thanks,
Alex
(I shortened the recipient list)
Am Sonntag, dem 01.10.2023 um 02:55 +0200 schrieb Alejandro Colomar:
> >
...
> I ran the tests, and get some unexpected failure. I used dg-warning,
> but maybe I used it wrong? Here's the output:
>
> ```
> output is:
> /home/alx/src/gnu/gcc/wustr/gcc/testsuite/gcc.dg/Wunterminated-string-initialization.c:5:14: warning: initializer-string for array of 'char' is too long [-Wunterminated-string-initi
> alization]
>
> FAIL: gcc.dg/Wunterminated-string-initialization.c (test for warnings, line 5)
> ```
>
> And here's the test:
>
> ```
> /* { dg-do compile } */
> /* { dg-options "-Wunterminated-string-initialization" } */
>
> char a1[] = "a";
> char a2[1] = "a"; /* { dg-warning "unterminated char sequence" } */
> char a3[2] = "a";
> ```
>
> Why isn't it expecting the warning?
Because the text does not match the actual output above?
You should see an additional FAIL for an excess warning.
Martin
On Sun, Oct 01, 2023 at 09:37:59AM +0200, Martin Uecker wrote:
>
> (I shortened the recipient list)
>
> Am Sonntag, dem 01.10.2023 um 02:55 +0200 schrieb Alejandro Colomar:
>
> > >
> ...
> > I ran the tests, and get some unexpected failure. I used dg-warning,
> > but maybe I used it wrong? Here's the output:
> >
> > ```
> > output is:
> > /home/alx/src/gnu/gcc/wustr/gcc/testsuite/gcc.dg/Wunterminated-string-initialization.c:5:14: warning: initializer-string for array of 'char' is too long [-Wunterminated-string-initi
> > alization]
> >
> > FAIL: gcc.dg/Wunterminated-string-initialization.c (test for warnings, line 5)
> > ```
> >
> > And here's the test:
> >
> > ```
> > /* { dg-do compile } */
> > /* { dg-options "-Wunterminated-string-initialization" } */
> >
> > char a1[] = "a";
> > char a2[1] = "a"; /* { dg-warning "unterminated char sequence" } */
> > char a3[2] = "a";
> > ```
> >
> > Why isn't it expecting the warning?
>
> Because the text does not match the actual output above?
Makes sense.
Here's the output after a fix (which I'll send soon as a new revision):
```
output is:
/home/alx/src/gnu/gcc/wustr/gcc/testsuite/gcc.dg/Wunterminated-string-initialization.c:5:14: warning: initializer-string for array of 'char' is too long [-Wunterminated-string-initialization]
```
and
```
=== gcc Summary ===
# of expected passes 2
```
The only thing that concerns me a little bit is that I don't see any
PASS (or XFAIL).
> You should see an additional FAIL for an excess warning.
Yep, I did.
Cheers,
Alex
>
> Martin
>
>
>
>
@@ -1382,6 +1382,10 @@ Wunsuffixed-float-constants
C ObjC Var(warn_unsuffixed_float_constants) Warning
Warn about unsuffixed float constants.
+Wunterminated-string-initialization
+C ObjC Var(warn_unterminated_string_initialization) Warning LangEnabledBy(C ObjC,Wextra || Wc++-compat)
+Warn about character arrays initialized as unterminated character sequences by a string literal.
+
Wunused
C ObjC C++ ObjC++ LangEnabledBy(C ObjC C++ ObjC++,Wall)
; documented in common.opt
@@ -8420,11 +8420,11 @@ digest_init (location_t init_loc, tree type, tree init, tree origtype,
pedwarn_init (init_loc, 0,
("initializer-string for array of %qT "
"is too long"), typ1);
- else if (warn_cxx_compat
+ else if (warn_unterminated_string_initialization
&& compare_tree_int (TYPE_SIZE_UNIT (type), len) < 0)
- warning_at (init_loc, OPT_Wc___compat,
+ warning_at (init_loc, OPT_Wunterminated_string_initialization,
("initializer-string for array of %qT "
- "is too long for C++"), typ1);
+ "is too long"), typ1);
if (compare_tree_int (TYPE_SIZE_UNIT (type), len) < 0)
{
unsigned HOST_WIDE_INT size
new file mode 100644
@@ -0,0 +1,6 @@
+/* { dg-do compile } */
+/* { dg-options "-Wunterminated-string-initialization" } */
+
+char a1[] = "a";
+char a2[1] = "a"; /* { dg-warning "unterminated char sequence" } */
+char a3[2] = "a";