Re: [LTP] [PATCH v2] syscalls/brk: add direct syscall tst_variant

[Petr Vorel <pvorel@suse.cz>]

Hi Teo,

> Hi Petr,

> On 12/12/2022 15:22, Petr Vorel wrote:
> > Hi Richie,

> > > Cyril, Petr, Would you like to add your reviewed by tags? Then we can
> > > merge this.
> > By accident I reply to my points to v1 [1].
> > To copy it here:

> > 1) There are warnings:
> > brk02.c: In function ‘brk_variants’:
> > brk02.c:26:28: warning: cast to pointer from integer of different size
> > [-Wint-to-pointer-cast]
> >     26 |                 brk_addr = (void *)brk(addr);
> >           |                            ^

> > 2) make check reports errors which are easily fixed.

> > Teo replied [2], that he's going to fix it. I thought I had set it
> > "Changes requested", but now I see "Needs Review / ACK". Setting it to
> > "Changes requested".

> I believe the points you raised are fixed in the v2, on top of this thread.
> Re-applying it on top of master on my side doesn't give me any warnings for
> the brk tests, as I do not cast the result from the libc brk anymore, and
> make check reports existing issues with the name of the function, but no
> style problems that did exist in v1. (I don't mind changing them if you
> want, but they are present on master as well).

> If you give a quick look at the patch v2 you'll see that indeed there is no
> more (void *)brk(addr) or such that generated the warnings, for example.
> (The syscalls still need it, as they return the break directly rather than
> an error, which is what the libc wrapper does.)

> I might be missing something, please do tell me if that's the case ! But I
> believe that the v2 _should_ be free of those issues.

I thought I must have put v1 code into my v2 branch. But the warnings are
actually when compiling on Alpine:

$ make V=1
gcc -I../../../../include -I../../../../include -I../../../../include/old/ -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -L../../../../lib brk01.c  -lltp -o brk01
brk01.c: In function 'verify_brk':
brk01.c:22:27: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
   22 |                 cur_brk = (void *)tst_syscall(__NR_brk, 0);
      |                           ^
brk01.c:52:35: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
   52 |                         cur_brk = (void *)tst_syscall(__NR_brk, new_brk);
      |                                   ^
gcc -I../../../../include -I../../../../include -I../../../../include/old/ -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -L../../../../lib brk02.c  -lltp -o brk02
brk02.c: In function 'brk_variants':
brk02.c:24:28: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
   24 |                 brk_addr = (void *)tst_syscall(__NR_brk, addr);
      |                            ^
brk02.c: In function 'brk_down_vmas':
brk02.c:38:28: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
   38 |                 brk_addr = (void *)tst_syscall(__NR_brk, 0);
      |                            ^

$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-alpine-linux-musl/12.1.1/lto-wrapper
Target: x86_64-alpine-linux-musl
Configured with: /home/buildozer/aports/main/gcc/src/gcc-12.1.1_git20220630/configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --build=x86_64-alpine-linux-musl --host=x86_64-alpine-linux-musl --target=x86_64-alpine-linux-musl --with-pkgversion='Alpine 12.1.1_git20220630' --enable-checking=release --disable-fixed-point --disable-libstdcxx-pch --disable-multilib --disable-nls --disable-werror --disable-symvers --enable-__cxa_atexit --enable-default-pie --enable-default-ssp --enable-cloog-backend --enable-languages=c,c++,d,objc,go,fortran,ada,jit --disable-libssp --disable-libmpx --disable-libmudflap --disable-libsanitizer --enable-shared --enable-threads --enable-tls --enable-host-shared --with-system-zlib --with-linker-hash-style=gnu
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 12.1.1 20220630 (Alpine 12.1.1_git20220630)

$ sudo ./brk01
tst_test.c:1558: TINFO: Timeout per run is 0h 00m 30s
brk01.c:24: TINFO: Testing libc variant
brk01.c:35: TCONF: brk() not implemented
brk01.c:21: TINFO: Testing syscall variant
brk01.c:59: TFAIL: brk() failed to set address have 0x4d3c4000 expected 0x4d3c5fff

$ sudo ./brk02
tst_test.c:1558: TINFO: Timeout per run is 0h 00m 30s
brk02.c:40: TINFO: Testing libc variant
brk02.c:51: TCONF: brk() not implemented
brk02.c:37: TINFO: Testing syscall variant
brk02.c:58: TFAIL: Cannot expand brk() by page size: EFAULT (14)

Looking on extra brk() support detection, you must have tested it on Alpine.
What am I missing?

Have you also compiled the code on Alpine or musl related distro?
If tested on musl, but not Alpine, what do you use? (maybe Alpine issue?)
I tested on some old random Alpine (3.17_alpha20220809), I'll try on some
updated version.

Also I wonder if we should bother with adding brk() / sbrk() logic into separate
function, added into brk.h. Probably not, although I don't like repeating code,
it's not worth for single function.

There are still missing static found by 'make check', diff below will fix that
(+ some additional space for readability).

Kind regards,
Petr

diff --git testcases/kernel/syscalls/brk/brk01.c testcases/kernel/syscalls/brk/brk01.c
index 93e430328..978e1f211 100644
--- testcases/kernel/syscalls/brk/brk01.c
+++ testcases/kernel/syscalls/brk/brk01.c
@@ -11,7 +11,7 @@
 #include "tst_test.h"
 #include "lapi/syscalls.h"
 
-void verify_brk(void)
+static void verify_brk(void)
 {
 	void *cur_brk, *new_brk;
 	size_t inc = getpagesize() * 2 - 1;
diff --git testcases/kernel/syscalls/brk/brk02.c testcases/kernel/syscalls/brk/brk02.c
index 3a97d279b..64931bc80 100644
--- testcases/kernel/syscalls/brk/brk02.c
+++ testcases/kernel/syscalls/brk/brk02.c
@@ -5,6 +5,7 @@
 
 /*\
  * [Description]
+ *
  * Expand brk() by at least 2 pages to ensure there is a newly created VMA
  * and not expanding the original due to multiple anon pages.  mprotect() that
  * new VMA then brk() back to the original address therefore causing a munmap of
@@ -16,7 +17,7 @@
 #include "tst_test.h"
 #include "lapi/syscalls.h"
 
-void *brk_variants(void *addr)
+static void *brk_variants(void *addr)
 {
 	void *brk_addr;
 
@@ -26,10 +27,11 @@ void *brk_variants(void *addr)
 		TST_EXP_PASS_SILENT(brk(addr), "brk()");
 		brk_addr = (void *)sbrk(0);
 	}
+
 	return brk_addr;
 }
 
-void brk_down_vmas(void)
+static void brk_down_vmas(void)
 {
 	void *brk_addr;
 

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp