linux-kernel - Re: [PATCH v2] tracing: silence GCC 9 array bounds warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiq72=uRuyDuRvZgxYAHxKRCOyJ-KQew+R24tPwOJuQmaO1Yw@mail.gmail.com>
Date:   Wed, 22 May 2019 15:11:10 +0200
From:   Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Ingo Molnar <mingo@...hat.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] tracing: silence GCC 9 array bounds warning

On Wed, May 22, 2019 at 1:52 PM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> On Wed, 22 May 2019 11:58:10 +0200
> Miguel Ojeda <miguel.ojeda.sandonis@...il.com> wrote:
>
> > +/* reset all but tr, trace, and overruns */
> > +static __always_inline void trace_iterator_reset(struct trace_iterator *iter)
> > +{
> > +     /*
> > +      * We do not simplify the start address to &iter->seq in order to let
> > +      * GCC 9 know that we really want to overwrite more members than
> > +      * just iter->seq (-Warray-bounds).
>
> This comment is fine for the change log, but here it is too specific.
> Why does one care about GCC 9 when we are at version GCC 21? I care
> more about why we are clearing the data and less about the way we are
> doing it.

Since the code is not written the obvious way on purpose, the idea is
to document why that is so -- otherwise the reader may wonder (and
possibly re-introduce it back). Specifying when the warning started
appearing tends to be clarifying, too.

The commit message explains the change itself, but the comment
explains why the current code is written like that.

> A comment like:
>
>         /*
>          * Reset the state of the trace_iterator so that it can read
>          * consumed data. Normally, the trace_iterator is used for
>          * reading the data when it is not consumed, and must retain
>          * state.
>          */
>
> That is more useful than why we have the offset hack.

That comment would be great in the function's description, in my
opinion, and it is a great addition to have nevertheless. I re-used
the existing comment for that to keep the change as minimal as
possible (and nevertheless I am not qualified to write it since I have
not studied the tracing code). In other words, I'm not saying there
are no further improvements :-)

Cheers,
Miguel