[LTP] [PATCH v2] syscalls/brk: add direct syscall tst_variant
Teo Couprie Diaz
teo.coupriediaz@arm.com
Tue Dec 13 12:48:13 CET 2022
Hi Petr,
Thanks again for taking the time to look into this !
On 12/12/2022 19:28, Petr Vorel wrote:
> 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?
That is quite strange indeed. As Richard pointed out in his reply to
this message, those warnings should not happen anymore since my patch
that changed tst_syscall to use intptr_t. ( e5d2a05a90e5 : regen.sh: Use
intptr_t for tst_syscall return )
However, I believe that the relevant header is only regenerated when
running ./configure , not when building normally. I know that I forgot
to do it a few times myself while testing the change to tst_syscall !
To be sure that it is supposed to work, I did the following on an Alpine
VM I used for testing :
make clean
make autotools
./configure
And then built the brk tests, with the following results :
root# make V=1
gcc -I../../../../include -I../../../../include
-I../../../../include/old/ -g -O2 -g -O2 -fno-strict-aliasing -pipe
-Wall -W -Wold-style-definition -std=gnu99 -L../../../../lib brk01.c
-lltp -o brk01
gcc -I../../../../include -I../../../../include
-I../../../../include/old/ -g -O2 -g -O2 -fno-strict-aliasing -pipe
-Wall -W -Wold-style-definition -std=gnu99 -L../../../../lib brk02.c
-lltp -o brk02
root# ./brk01 ; ./brk02
tst_test.c:1559: 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:70: TPASS: brk() works fine
[...]
tst_test.c:1559: 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:84: TPASS: munmap at least two VMAs of brk() passed
[...]
This is with the v2 of the patch applied on top of master (
ca1b0b0cc938: tst_fill_fs: Remove O_FSYNC ).
>
> 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 usually test on a custom Debian-based musl distribution[1]. It's what
we use for porting the linux kernel on Morello[0], where I ran into the
issue. When sharing something with upstream I test on Alpine as well, to
be sure it's just not our weird environment !
Just to be sure, I ran the steps above on Alpine and on my side I don't
seem to have the same issues you observe.
> I tested on some old random Alpine (3.17_alpha20220809), I'll try on some
> updated version.
My Alpine VM seems to be an even older version, 3.16, I can try an
updated version as well if you think it's valuable. My GCC is older than
yours as well :
root# gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-alpine-linux-musl/11.2.1/lto-wrapper
Target: x86_64-alpine-linux-musl
Configured with:
/home/buildozer/aports/main/gcc/src/gcc-11.2.1_git20220219/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
11.2.1_git20220219' --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 11.2.1 20220219 (Alpine 11.2.1_git20220219)
> 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.
I did wonder about that too, and reached the same conclusion as you : it
seems to be a bit much for a single function. Happy to do it if you
change your mind or if another maintainer disagrees !
>
> There are still missing static found by 'make check', diff below will fix that
> (+ some additional space for readability).
Indeed, but as they were missing in the original code I did not know if
they should be changed. Thanks for clarifying and for the diff !
> Kind regards,
> Petr
Best regards,
Téo
[0]: https://git.morello-project.org/morello/kernel/linux
[1]:
https://git.morello-project.org/morello/docs/-/blob/morello/mainline/morello-linux-purecap-environment.rst
> 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;
>
More information about the ltp
mailing list