Re: [PATCH v5 RESEND] C, ObjC: Add -Wunterminated-string-initialization

[Sandra Loosemore]

On 2/26/24 09:19, Joseph Myers wrote:

On Mon, 26 Feb 2024, Alejandro Colomar wrote:


The idea seems reasonable, but the patch needs documentation for the new
option in invoke.texi.


Thanks!  Will do.

I don't see an obvious order in that file.  Where would you put the
option?  Do you want me to sort(1) it first, and then insert the new
option in alphabetic order?


Sandra might have a better idea of how the warning options ought to be
ordered in the manual.  (Obviously, don't mix reordering with any content
changes.)


Please don't blindly sort the options sections of the manual.  They've 
typically been organized to present general options first (things like 
-Wpedantic in the warning options section, or -g or -o in the debugging 
and optimization optimization sections), then more specialized flags for 
controlling specific individual behaviors.  We could do a better job of 
keeping the latter sublists alphabetized, but in some cases we have also 
grouped documentation for related options together even if they are not 
alphabetical, but that needs some manual review and decision-making and 
should not be mixed with adding new content.  We do try to keep the 
lists in the "Option Summary" section alphabetized except for the 
general options listed first, though.


As far as where to put documentation for new options, I'd say that you 
should put it in alphabetical order in the appropriate section, unless 
there is good reason to put it somewhere else, or the alphabetization of 
the whole section is so messed-up you can't figure out where it should 
go (putting it at the end is OK in that case, as opposed to some other 
random location in the list).  Remember to add the matching entry in the 
appropriate "Option Summary" list, and for this option IIUC you also 
need to add it to the lists of other options enabled by -Wextra and 
-Wc++-compat.


Are we still accepting patches to add new options in stage 4, or is this 
a stage 1 item?


-Sandra

Re: [PATCH v5 RESEND] C, ObjC: Add -Wunterminated-string-initialization

[Mike Stump]

On Feb 26, 2024, at 7:56 AM, Alejandro Colomar  wrote:
> 
> I don't see an obvious order in that file.  Where would you put the
> option?

The best place, would be to put it just after:

-Warray-bounds
-Warray-bounds=n

This is a functional style grouping that best mirrors the existing ordering.  A 
runner up would be next to the string options or near a "terminating NUL", but 
after review, these don't seem more suitable.

> Do you want me to sort(1) it first, and then insert the new
> option in alphabetic order?

No.  Let others play at that.

Re: [PATCH v5 RESEND] C, ObjC: Add -Wunterminated-string-initialization

[Joseph Myers]

On Mon, 26 Feb 2024, Alejandro Colomar wrote:

> > The idea seems reasonable, but the patch needs documentation for the new 
> > option in invoke.texi.
> 
> Thanks!  Will do.
> 
> I don't see an obvious order in that file.  Where would you put the
> option?  Do you want me to sort(1) it first, and then insert the new
> option in alphabetic order?

Sandra might have a better idea of how the warning options ought to be 
ordered in the manual.  (Obviously, don't mix reordering with any content 
changes.)

-- 
Joseph S. Myers
josmy...@redhat.com

Re: [PATCH v5 RESEND] C, ObjC: Add -Wunterminated-string-initialization

[Alejandro Colomar]

Hi Joseph,

On Mon, Feb 26, 2024 at 03:24:32PM +, Joseph Myers wrote:
> On Sun, 25 Feb 2024, Mike Stump wrote:
> 
> > On Feb 6, 2024, at 2:45 AM, Alejandro Colomar  wrote:
> > > 
> > > Warn about the following:
> > > 
> > >char  s[3] = "foo";
> > 
> > No ObjC specific impact here, so no need for ObjC review.
> > 
> > As a member of the peanut gallery, I like the patch.
> > 
> > Joseph, this is been submitted 5 times over the past year.  Any thoughts?
> 
> The idea seems reasonable, but the patch needs documentation for the new 
> option in invoke.texi.

Thanks!  Will do.

I don't see an obvious order in that file.  Where would you put the
option?  Do you want me to sort(1) it first, and then insert the new
option in alphabetic order?

$ man gcc 2>/dev/null | grep -e '^ \{0,3\}[^ ]' -e '^ \{4,7\}-' | sed -n '/^ 
\{1,3\}Options to Request or Suppress Warnings/,/^ \{1,3\}[^ ]/p'
   Options to Request or Suppress Warnings
 -fsyntax-only
 -fmax-errors=n
 -w  Inhibit all warning messages.
 -Werror
 -Werror=
 -Wfatal-errors
 -Wno- options with old compilers, but if something goes wrong, the 
compiler warns that an unrecognized option is present.
 -Wpedantic
 -pedantic
 -pedantic-errors
 -Wall
 -Wextra
 -Wabi (C, Objective‐C, C++ and Objective-C++ only)
 -Wno-changes-meaning (C++ and Objective-C++ only)
 -Wchar-subscripts
 -Wno-coverage-mismatch
 -Wno-coverage-invalid-line-number
 -Wno-cpp (C, Objective‐C, C++, Objective-C++ and Fortran only)
 -Wdouble-promotion (C, C++, Objective‐C and Objective-C++ only)
 -Wduplicate-decl-specifier (C and Objective‐C only)
 -Wformat
 -Wformat=n
 -Wno-format-contains-nul
 -Wno-format-extra-args
 -Wformat-overflow
 -Wformat-overflow=level
 -Wno-format-zero-length
 -Wformat-nonliteral
 -Wformat-security
 -Wformat-signedness
 -Wformat-truncation
 -Wformat-truncation=level
 -Wformat-y2k
 -Wnonnull
 -Wnonnull-compare
 -Wnull-dereference
 -Winfinite-recursion
 -Winit-self (C, C++, Objective‐C and Objective-C++ only)
 -Wno-implicit-int (C and Objective‐C only)
 -Wno-implicit-function-declaration (C and Objective‐C only)
 -Wimplicit (C and Objective‐C only)
 -Wimplicit-fallthrough
 -Wimplicit-fallthrough=n
 -Wno-if-not-aligned (C, C++, Objective‐C and Objective-C++ only)
 -Wignored-qualifiers (C and C++ only)
 -Wno-ignored-attributes (C and C++ only)
 -Wmain
 -Wmisleading-indentation (C and C++ only)
 -Wmissing-attributes
 -Wmissing-braces
 -Wmissing-include-dirs (C, C++, Objective‐C, Objective-C++ and Fortran 
only)
 -Wno-missing-profile
 -Wmismatched-dealloc
 -Wmultistatement-macros
 -Wparentheses
 -Wno-self-move (C++ and Objective-C++ only)
 -Wsequence-point
 -Wno-return-local-addr
 -Wreturn-type
 -Wno-shift-count-negative
 -Wno-shift-count-overflow
 -Wshift-negative-value
 -Wno-shift-overflow
 -Wshift-overflow=n
 -Wswitch
 -Wswitch-default
 -Wswitch-enum
 -Wno-switch-bool
 -Wno-switch-outside-range
 -Wno-switch-unreachable
 -Wsync-nand (C and C++ only)
 -Wtrivial-auto-var-init
 -Wunused-but-set-parameter
 -Wunused-but-set-variable
 -Wunused-function
 -Wunused-label
 -Wunused-local-typedefs (C, Objective‐C, C++ and Objective-C++ only)
 -Wunused-parameter
 -Wno-unused-result
 -Wunused-variable
 -Wunused-const-variable
 -Wunused-const-variable=n
 -Wunused-value
 -Wunused
 -Wuninitialized
 -Wno-invalid-memory-model
 -Wmaybe-uninitialized
 -Wunknown-pragmas
 -Wno-pragmas
 -Wno-prio-ctor-dtor
 -Wstrict-aliasing
 -Wstrict-aliasing=n
 -Wstrict-overflow
 -Wstrict-overflow=n
 -Wstring-compare
 -Wno-stringop-overflow
 -Wstringop-overflow
 -Wstringop-overflow=type
 -Wno-stringop-overread
 -Wno-stringop-truncation
 -Wstrict-flex-arrays
 -Wsuggest-attribute=[pure|const|noreturn|format|cold|malloc]
 -Walloc-zero
 -Walloc-size-larger-than=byte‐size
 -Wno-alloc-size-larger-than
 -Walloca
 -Walloca-larger-than=byte‐size
 -Wno-alloca-larger-than
 -Warith-conversion
 -Warray-bounds
 -Warray-bounds=n
 -Warray-compare
 -Warray-parameter
 -Warray-parameter=n
 -Wattribute-alias=n
 -Wno-attribute-alias
 -Wbidi-chars=[none|unpaired|any|ucn]
 -Wbool-compare
 -Wbool-operation
 -Wduplicated-branches
 -Wduplicated-cond
 -Wframe-address
 -Wno-discarded-qualifiers (C and Objective‐C only)
 -Wno-discarded-array-qualifiers (C and Objective‐C only)
 -Wno-incompatible-pointer-types (C and Objective‐C only)
 -Wno-int-conversion (C and Objective‐C only)
 -Wzero-length-bounds
 -Wno-div-by-zero
 -Wsystem-headers
 -Wtautological-compare
 -Wtrampolines
 -Wfloat-equal
 -Wtraditional (C and Object

Re: [PATCH v5 RESEND] C, ObjC: Add -Wunterminated-string-initialization

[Joseph Myers]

On Sun, 25 Feb 2024, Alejandro Colomar wrote:

> or if it's just that everyone was busy doing
> other stuff.

Yes, that's right.  The patch was already listed on my patch review 
backlog, but that backlog is long.

-- 
Joseph S. Myers
josmy...@redhat.com

Re: [PATCH v5 RESEND] C, ObjC: Add -Wunterminated-string-initialization

[Joseph Myers]

On Sun, 25 Feb 2024, Mike Stump wrote:

> On Feb 6, 2024, at 2:45 AM, Alejandro Colomar  wrote:
> > 
> > Warn about the following:
> > 
> >char  s[3] = "foo";
> 
> No ObjC specific impact here, so no need for ObjC review.
> 
> As a member of the peanut gallery, I like the patch.
> 
> Joseph, this is been submitted 5 times over the past year.  Any thoughts?

The idea seems reasonable, but the patch needs documentation for the new 
option in invoke.texi.

-- 
Joseph S. Myers
josmy...@redhat.com

Re: [PATCH v5 RESEND] C, ObjC: Add -Wunterminated-string-initialization

[Alejandro Colomar]

Hi Mike, Joseph,

On Sun, Feb 25, 2024 at 10:10:09AM -0800, Mike Stump wrote:
> On Feb 6, 2024, at 2:45 AM, Alejandro Colomar  wrote:
> > 
> > Warn about the following:
> > 
> >char  s[3] = "foo";
> 
> No ObjC specific impact here, so no need for ObjC review.
> 
> As a member of the peanut gallery, I like the patch.
> 
> Joseph, this is been submitted 5 times over the past year.  Any thoughts?

Thanks!  BTW, I'd like to know if I did anything wrong so that it wasn't
reviewed in all this time, or if it's just that everyone was busy doing
other stuff.  Do you prefer if I ping more often?  Or something else?

Have a lovely day!
Alex

-- 

Looking for a remote C programming job at the moment.


signature.asc
Description: PGP signature

Re: [PATCH v5 RESEND] C, ObjC: Add -Wunterminated-string-initialization

[Mike Stump]

On Feb 6, 2024, at 2:45 AM, Alejandro Colomar  wrote:
> 
> Warn about the following:
> 
>char  s[3] = "foo";

No ObjC specific impact here, so no need for ObjC review.

As a member of the peanut gallery, I like the patch.

Joseph, this is been submitted 5 times over the past year.  Any thoughts?

> 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.
> 
> Since Wc++-compat now includes this warning, the test has to be modified
> to expect the text of the new warning too, in .
> 
> Link: 
> Link: 
> Link: 
> 
> Acked-by: Doug McIlroy 
> Cc: "G. Branden Robinson" 
> Cc: Ralph Corderoy 
> Cc: Dave Kemper 
> Cc: Larry McVoy 
> Cc: Andrew Pinski 
> Cc: Jonathan Wakely 
> Cc: Andrew Clayton 
> Cc: Martin Uecker 
> Cc: David Malcolm 
> Signed-off-by: Alejandro Colomar 
> ---
> 
> v5:
> 
> -  Fix existing C++-compat tests.  [reported by ]
> 
> 
> gcc/c-family/c.opt | 4 
> gcc/c/c-typeck.cc  | 6 +++---
> gcc/testsuite/gcc.dg/Wcxx-compat-14.c  | 2 +-
> gcc/testsuite/gcc.dg/Wunterminated-string-initialization.c | 6 ++
> 4 files changed, 14 insertions(+), 4 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/Wunterminated-string-initialization.c
> 
> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
> index 44b9c862c14..e8f6b836836 100644
> --- a/gcc/c-family/c.opt
> +++ b/gcc/c-family/c.opt
> @@ -1407,6 +1407,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
> diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
> index e55e887da14..7df9de819ed 100644
> --- a/gcc/c/c-typeck.cc
> +++ b/gcc/c/c-typeck.cc
> @@ -8399,11 +8399,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
> diff --git a/gcc/testsuite/gcc.dg/Wcxx-compat-14.c 
> b/gcc/testsuite/gcc.dg/Wcxx-compat-14.c
> index 23783711be6..6df0ee197cc 100644
> --- a/gcc/testsuite/gcc.dg/Wcxx-compat-14.c
> +++ b/gcc/testsuite/gcc.dg/Wcxx-compat-14.c
> @@ -2,5 +2,5 @@
> /* { dg-options "-Wc++-compat" } */
> 
> char a1[] = "a";
> -char a2[1] = "a";/* { dg-warning "C\[+\]\[+\]" } */
> +char a2[1] = "a";/* { dg-warning "initializer-string for array of 'char' 
> is too long" } */
> char a3[2] = "a";
> diff --git a/gcc/testsuite/gcc.dg/Wunterminated-string-initialization.c 
> b/gcc/testsuite/gcc.dg/Wuntermin