On Wed, Feb 05, 2025 at 12:59:58PM +0100, Jakub Jelinek wrote:
> Hi!
>
> Kees, any progress on this?
Hi!
I need to take another run at it. I got stalled out when I discovered
that I array-of-char-arrays attributes got applied at the "wrong" depth,
and stuff wasn't working.
e.g.:
char acpi_table[TABLE_SIZE][4] __attribute((nonstring)) = {
{ "ohai" },
{ "1234" },
};
when nonstring was checked for on something like "acpi_table[2]" it
wouldn't be found, since it was applied at the top level.
>
> On Mon, Jan 06, 2025 at 04:34:27PM -0500, Marek Polacek wrote:
> > > - pedwarn_init (init_loc, 0,
> > > - ("initializer-string for array of %qT "
> > > - "is too long"), typ1);
> > > - else if (warn_unterminated_string_initialization
> > > - && compare_tree_int (TYPE_SIZE_UNIT (type), len) < 0)
> > > - warning_at (init_loc, OPT_Wunterminated_string_initialization,
> > > - ("initializer-string for array of %qT "
> > > - "is too long"), typ1);
> > > +
> > > if (compare_tree_int (TYPE_SIZE_UNIT (type), len) < 0)
> > > {
> > > - unsigned HOST_WIDE_INT size
> > > - = tree_to_uhwi (TYPE_SIZE_UNIT (type));
> > > - const char *p = TREE_STRING_POINTER (inside_init);
> > > -
> > > - inside_init = build_string (size, p);
> > > + unsigned HOST_WIDE_INT avail
> > > + = tree_to_uhwi (TYPE_SIZE_UNIT (type));
> > > + unsigned unit = TYPE_PRECISION (typ1) / BITS_PER_UNIT;
> > > + const char *p = TREE_STRING_POINTER (inside_init);
> > > +
> > > + /* Construct truncated string. */
> > > + inside_init = build_string (avail, p);
> > > +
> > > + /* Subtract the size of a single (possibly wide) character
> > > + because it may be ok to ignore the terminating NUL char
> > > + that is counted in the length of the constant. */
> > > + if (warn_cxx_compat || len - unit > avail)
> > > + {
> > > + pedwarn_init (init_loc, 0,
> > > + ("initializer-string for array of %qT "
> > > + "is too long (%lu chars into %lu "
> > > + "available)"), typ1, len, avail);
> > > + }
> >
> > No need for the { } here.
>
> The indentation is wrong in the whole block.
> The { after compare_tree_int is indented by 2 tabs (so 16 columns), but
> unsigned HOST_WIDE_INT avail already by 2 tabs and 3 spaces rather than
> 2 tabs and 2 spaces. So it is one column more right than it should be.
>
> Anyway, more importantly, the "warn_cxx_compat || " part looks wrong.
> For -Wc++-compat we should (perhaps with slightly different wording)
> preserve the GCC 14 behavior, which was
> /* Subtract the size of a single (possibly wide) character
> because it's ok to ignore the terminating null char
> that is counted in the length of the constant. */
> if (compare_tree_int (TYPE_SIZE_UNIT (type), len - unit) < 0)
> pedwarn_init (init_loc, 0,
> ("initializer-string for array of %qT "
> "is too long"), typ1);
> else if (warn_cxx_compat
> && compare_tree_int (TYPE_SIZE_UNIT (type), len) < 0)
> warning_at (init_loc, OPT_Wc___compat,
> ("initializer-string for array of %qT "
> "is too long for C++"), typ1);
> So, if (len - unit > avail) do the pedwarn_init (also note that %lu is
> wrong for unsigned HOST_WIDE_INT, one should use %wu instead; or for the
> avail case one could just print %E and pass it TYPE_SIZE_UNIT (type);
> unsigned HOST_WIDE_INT doesn't have to be unsigned long, it can be also
> unsigned long long). And if warn_cxx_compat, do warning_at, IMNSHO
> with OPT_Wc___compat, with the "is too long for C++" substring in it
> and perhaps some more details (len and avail too).
>
> Though, perhaps we should also change
> C ObjC Var(warn_unterminated_string_initialization) Warning LangEnabledBy(C
> ObjC,Wextra || Wc++-compat)
> to
> C ObjC Var(warn_unterminated_string_initialization) Warning LangEnabledBy(C
> ObjC,Wextra)
> and adjust documentation, because say
> -Wno-unterminated-string-initialization will no longer turn off that
> warning, essentially -Wc++-compat and -Wunterminated-string-initialization
> are then independent warnings with -Wc++-compat having priority over the
> latter.
> Or we could guard it with
> warn_cxx_compat && warn_unterminated_string_initialization
> but then the question is what to pass to second warning_at option,
> because one needs both.
> So, I think it is better to keep -Wc++-compat working as before (perhaps
> with the extra descriptions of the two lengths) and then have this new
> -Wunterminated-string-initialization warning implied by -Wextra which
> does nothing in the rare case when -Wc++-compat is on, and otherwise
> does the new stuff of warning except when initializing nonstring
> objects.
Okay, I will take a stab at getting this reorganized. Thanks for the
review!
--
Kees Cook
