2023-08-01 05:59:50

by Zhangjin Wu

Subject: [PATCH v2 08/10] selftests/nolibc: avoid sign-compare warnings

Hi, Thomas

> These warnings will be enabled later so avoid triggering them.
>
> Signed-off-by: Thomas Weißschuh <[email protected]>
> ---
> tools/testing/selftests/nolibc/nolibc-test.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> index cb17cccd0bc7..82714051c72f 100644
> --- a/tools/testing/selftests/nolibc/nolibc-test.c
> +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> @@ -749,7 +749,7 @@ static int test_mmap_munmap(void)
> };
>
> page_size = getpagesize();
> - if (page_size < 0)
> + if (page_size == 0)
> return -1;
>

It was my mistake before, but do we need to align with the one used in
test_getpagesize():

static int test_getpagesize(void)
{
long x = getpagesize();
int c;

if (x < 0)
return x;

Use 'long' instead of 'size_t' to declare page_size?

Thanks,
Zhangjin

> /* find a right file to mmap, existed and accessible */
> @@ -998,7 +998,7 @@ static int run_stdlib(int min, int max)
> #define EXPECT_VFPRINTF(c, expected, fmt, ...) \
> ret += expect_vfprintf(llen, c, expected, fmt, ##__VA_ARGS__)
>
> -static int expect_vfprintf(int llen, size_t c, const char *expected, const char *fmt, ...)
> +static int expect_vfprintf(int llen, int c, const char *expected, const char *fmt, ...)
> {
> int ret, fd, w, r;
> char buf[100];
>
> --
> 2.41.0

2023-08-01 06:23:55

by Thomas Weißschuh

Subject: Re: [PATCH v2 08/10] selftests/nolibc: avoid sign-compare warnings

Hi Zhangjin!

On 2023-08-01 13:48:19+0800, Zhangjin Wu wrote:
> > These warnings will be enabled later so avoid triggering them.
> >
> > Signed-off-by: Thomas Weißschuh <[email protected]>
> > ---
> > tools/testing/selftests/nolibc/nolibc-test.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > index cb17cccd0bc7..82714051c72f 100644
> > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > @@ -749,7 +749,7 @@ static int test_mmap_munmap(void)
> > };
> >
> > page_size = getpagesize();
> > - if (page_size < 0)
> > + if (page_size == 0)
> > return -1;
> >
>
> It was my mistake before, but do we need to align with the one used in
> test_getpagesize():
>
> static int test_getpagesize(void)
> {
> long x = getpagesize();
> int c;
>
> if (x < 0)
> return x;
>
> Use 'long' instead of 'size_t' to declare page_size?

Good point.

Given that getpagesize() is documented as returning "int" I guess we
should actually change the implementation in nolibc.

> Thanks,
> Zhangjin
>
> > /* find a right file to mmap, existed and accessible */
> > @@ -998,7 +998,7 @@ static int run_stdlib(int min, int max)
> > #define EXPECT_VFPRINTF(c, expected, fmt, ...) \
> > ret += expect_vfprintf(llen, c, expected, fmt, ##__VA_ARGS__)
> >
> > -static int expect_vfprintf(int llen, size_t c, const char *expected, const char *fmt, ...)
> > +static int expect_vfprintf(int llen, int c, const char *expected, const char *fmt, ...)
> > {
> > int ret, fd, w, r;
> > char buf[100];
> >
> > --
> > 2.41.0

2023-08-01 07:33:17

by Zhangjin Wu

Subject: Re: [PATCH v2 08/10] selftests/nolibc: avoid sign-compare warnings

> On 2023-08-01 13:48:19+0800, Zhangjin Wu wrote:
> > > These warnings will be enabled later so avoid triggering them.
> > >
> > > Signed-off-by: Thomas Weißschuh <[email protected]>
> > > ---
> > > tools/testing/selftests/nolibc/nolibc-test.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > > index cb17cccd0bc7..82714051c72f 100644
> > > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > > @@ -749,7 +749,7 @@ static int test_mmap_munmap(void)
> > > };
> > >
> > > page_size = getpagesize();
> > > - if (page_size < 0)
> > > + if (page_size == 0)
> > > return -1;
> > >
> >
> > It was my mistake before, but do we need to align with the one used in
> > test_getpagesize():
> >
> > static int test_getpagesize(void)
> > {
> > long x = getpagesize();
> > int c;
> >
> > if (x < 0)
> > return x;
> >
> > Use 'long' instead of 'size_t' to declare page_size?
>
> Good point.
>
> Given that getpagesize() is documented as returning "int" I guess we
> should actually change the implementation in nolibc.
>

Yes, it is documented at [1], perhaps Willy looked at this line before:

This interface, returning an int, may have problems representing appropriate values in the future. Applications should use the sysconf() function instead.

[1]: https://pubs.opengroup.org/onlinepubs/7908799/xsh/getpagesize.html

> > Thanks,
> > Zhangjin