Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   SeongJae Park <sj@kernel.org>
To:     Rong Tao <rtoax@foxmail.com>
Cc:     sj@kernel.org, Rong Tao <rongtao@cestc.cn>,
        Shuah Khan <shuah@kernel.org>, damon@lists.linux.dev,
        linux-mm@kvack.org, linux-kselftest@vger.kernel.org,
        linux-kernel@vger.kernel.org, Yuanchu Xie <yuanchu@google.com>
Subject: Re: [PATCH] selftests/damon: Fix unnecessary compilation warnings
Date:   Thu, 10 Nov 2022 20:09:39 +0000
Message-Id: <20221110200939.101886-1-sj@kernel.org>
In-Reply-To: <tencent_CF837EA1B8AC81CB7715E85797510D441E07@qq.com>
References: 
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Cc-ing Yuanchu Xie.

Hi Rong,

On Thu, 10 Nov 2022 21:34:18 +0800 Rong Tao <rtoax@foxmail.com> wrote:

> From: Rong Tao <rongtao@cestc.cn>
> 
> When testing overflow and overread, there is no need to keep unnecessary
> compilation warnings, we should simply ignore them.
> 
> How to reproduce the problem:
> 
> $ make -C tools/testing/selftests/
> gcc     huge_count_read_write.c  -o /home/sd/Git/linux/tools/testing/selftests/damon/huge_count_read_write

'checkpatch.pl' complains:

    WARNING: use relative pathname instead of absolute in changelog text
    #20:
    gcc     huge_count_read_write.c  -o /home/sd/Git/linux/tools/testing/selftests/damon/huge_count_read_write

Also, could we add four spaces indent for code snippet/command outputs like
above?

> huge_count_read_write.c: In function ‘write_read_with_huge_count’:
> huge_count_read_write.c:23:9: warning: ‘write’ reading 4294967295 bytes from a region of size 1 [-Wstringop-overread]
>    23 |         write(filedesc, "", 0xfffffffful);
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from huge_count_read_write.c:8:
> /usr/include/unistd.h:378:16: note: in a call to function ‘write’ declared with attribute ‘access (read_only, 2, 3)’
>   378 | extern ssize_t write (int __fd, const void *__buf, size_t __n) __wur
>       |                ^~~~~
> huge_count_read_write.c:25:15: warning: ‘read’ writing 4294967295 bytes into a region of size 25 overflows the destination [-Wstringop-overflow=]
>    25 |         ret = read(filedesc, buf, 0xfffffffful);
>       |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> huge_count_read_write.c:14:14: note: destination object ‘buf’ of size 25
>    14 |         char buf[25];
>       |              ^~~
> In file included from huge_count_read_write.c:8:
> /usr/include/unistd.h:371:16: note: in a call to function ‘read’ declared with attribute ‘access (write_only, 2, 3)’
>   371 | extern ssize_t read (int __fd, void *__buf, size_t __nbytes) __wur
>       |                ^~~~
> 
> Signed-off-by: Rong Tao <rongtao@cestc.cn>
> ---
>  tools/testing/selftests/damon/huge_count_read_write.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tools/testing/selftests/damon/huge_count_read_write.c b/tools/testing/selftests/damon/huge_count_read_write.c
> index ad7a6b4cf338..8fbe276870e7 100644
> --- a/tools/testing/selftests/damon/huge_count_read_write.c
> +++ b/tools/testing/selftests/damon/huge_count_read_write.c
> @@ -8,6 +8,11 @@
>  #include <unistd.h>
>  #include <stdio.h>
>  
> +#pragma GCC diagnostic push
> +/* Ignore read(2) overflow and write(2) overread compile warnings */
> +#pragma GCC diagnostic ignored "-Wstringop-overread"
> +#pragma GCC diagnostic ignored "-Wstringop-overflow"
> +

Thank you for sending this patch!

However, there was a similar patch from Yuanchu[1], and this causes another
warning for old gcc[2] that I use (9.4.0), like below.

    gcc -Wno-stringop-overread -Wno-stringop-overflow    huge_count_read_write.c  -o /home/sjpark/linux/tools/testing/selftests/damon/huge_count_read_write
    huge_count_read_write.c:13:32: warning: unknown option after ‘#pragma GCC diagnostic’ kind [-Wpragmas]
       13 | #pragma GCC diagnostic ignored "-Wstringop-overread"
          |                                ^~~~~~~~~~~~~~~~~~~~~
    cc1: warning: unrecognized command line option ‘-Wno-stringop-overread’

As mentioned as a reply to Yuanchu's patch, I'd slightly prefer making it
silent for both new and old compilers than this approach, but no strong opinion
from my side.  Yuanchu and Shuah, do you have some opinion?

[1] https://lore.kernel.org/lkml/CAJj2-QE4ee=N9wYXVQc6gyZYC3zgAsWVwWJ7DMaS2B9q2WqBHw@mail.gmail.com/
[2] https://lore.kernel.org/lkml/20220504184537.130085-1-sj@kernel.org/
[3] https://lore.kernel.org/lkml/20220517160417.1096-1-sj@kernel.org/


Thanks,
SJ

>  void write_read_with_huge_count(char *file)
>  {
>  	int filedesc = open(file, O_RDWR);
> @@ -27,6 +32,8 @@ void write_read_with_huge_count(char *file)
>  	close(filedesc);
>  }
>  
> +#pragma GCC diagnostic pop
> +
>  int main(int argc, char *argv[])
>  {
>  	if (argc != 2) {
> -- 
> 2.31.1
> 
>