[LTP] [PATCH v1] syscalls/signal06: add volatile to loop variable

[LTP] [PATCH v1] syscalls/signal06: add volatile to loop variable

[Edward Liaw via ltp]

On Android compiled with clang, the loop variable will be optimized out
unless it is tagged with volatile.

Signed-off-by: Edward Liaw <edliaw@google.com>
---
 testcases/kernel/syscalls/signal/signal06.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/signal/signal06.c b/testcases/kernel/syscalls/signal/signal06.c
index 64f886ee3..b40ff3e40 100644
--- a/testcases/kernel/syscalls/signal/signal06.c
+++ b/testcases/kernel/syscalls/signal/signal06.c
@@ -65,7 +65,7 @@ char altstack[4096 * 10] __attribute__((aligned(4096)));
 
 void test(void)
 {
-	int loop = 0;
+	volatile int loop = 0;
 	int pid = getpid();
 
 	D = VALUE;
-- 
2.37.0.144.g8ac04bfd2-goog


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

Re: [LTP] [PATCH v1] syscalls/signal06: add volatile to loop variable

[Petr Vorel]

Hi Edward,

> On Android compiled with clang, the loop variable will be optimized out
> unless it is tagged with volatile.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Thanks!

Kind regards,
Petr

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

Re: [LTP] [PATCH v1] syscalls/signal06: add volatile to loop variable

[Cyril Hrubis]

Hi!
> On Android compiled with clang, the loop variable will be optimized out
> unless it is tagged with volatile.

Looking at the code it looks strange that it's optimized out since we
use the value for the loop as:

	loop = 0;

	D = VALUE;
	while (D == VALUE && loop < LOOPS) {
		asm(...);
		loop++;
	}

And the D variable is properly marked as volatile so it's not like the
loop can be expected to iterate preciselly LOOPS iterations.

It looks to me like the compiler actually forgets about the volatility
of D for some reason and then assumes that the loop does LOOPS
iterations.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v1] syscalls/signal06: add volatile to loop variable

[Edward Liaw via ltp]

Hey Cyril, sorry for the late reply, I was on vacation.

On Tue, Jul 19, 2022 at 3:19 AM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > On Android compiled with clang, the loop variable will be optimized out
> > unless it is tagged with volatile.
>
> Looking at the code it looks strange that it's optimized out since we
> use the value for the loop as:
>
>         loop = 0;
>
>         D = VALUE;
>         while (D == VALUE && loop < LOOPS) {
>                 asm(...);
>                 loop++;
>         }
>
> And the D variable is properly marked as volatile so it's not like the
> loop can be expected to iterate preciselly LOOPS iterations.
>
> It looks to me like the compiler actually forgets about the volatility
> of D for some reason and then assumes that the loop does LOOPS
> iterations.

I'm not totally sure how the compiler is working in this case.

What I saw with Android is that it fails with:
signal06    0  TINFO  :  loop = 2076174312
signal06    1  TFAIL  :
external/ltp/testcases/kernel/syscalls/signal/signal06.c:87: Bug
Reproduced!

It makes one iteration, then loop is set to a random large int and the
loop terminates.  Printing the value of loop inside the for loop
actually caused it to iterate 30000 times and succeed.

Compared to a successful run, which looks like:
signal06    0  TINFO  :  loop = 30000
signal06    1  TPASS  :  signal06 call succeeded

Thanks,
Edward

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

Re: [LTP] [PATCH v1] syscalls/signal06: add volatile to loop variable

[Edward Liaw via ltp]

> > It looks to me like the compiler actually forgets about the volatility
> > of D for some reason and then assumes that the loop does LOOPS
> > iterations.

Hey Cyril,
I think I finally understand what you mean by this now; it is rather
strange that the volatility of D does not protect loop from being
optimized away by the compiler.  I don't have a good explanation as to
why it's happening but I'm not sure how to evaluate what's going on
either.  Should I do anything to move this patch forward?

Thanks,
Edward

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

Re: [LTP] [PATCH v1] syscalls/signal06: add volatile to loop variable

[Cyril Hrubis]

Hi!
> I think I finally understand what you mean by this now; it is rather
> strange that the volatility of D does not protect loop from being
> optimized away by the compiler.  I don't have a good explanation as to
> why it's happening but I'm not sure how to evaluate what's going on
> either.  Should I do anything to move this patch forward?

It all boils down if we want to work around something that looks like a
compiler bug in tests or not. I would be inclined not to do so since LTP
was littered with quite a lot of workarounds for glibc/compiler bugs and
we spend quite some time cleaning that mess up. But in this case I can
agree that this is a borderland issue so I'm not strongly against that
either.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v1] syscalls/signal06: add volatile to loop variable

[Petr Vorel]

Hi Edward,

> Hi!
> > I think I finally understand what you mean by this now; it is rather
> > strange that the volatility of D does not protect loop from being
> > optimized away by the compiler.  I don't have a good explanation as to
> > why it's happening but I'm not sure how to evaluate what's going on
> > either.  Should I do anything to move this patch forward?

> It all boils down if we want to work around something that looks like a
> compiler bug in tests or not. I would be inclined not to do so since LTP
> was littered with quite a lot of workarounds for glibc/compiler bugs and
> we spend quite some time cleaning that mess up. But in this case I can
> agree that this is a borderland issue so I'm not strongly against that
> either.

Edward, which which clang version requires it? It'd be nice to document it, so
that it can be removed in the future.
Is there any bug report for it?

Kind regards,
Petr

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

Re: [LTP] [PATCH v1] syscalls/signal06: add volatile to loop variable

[Edward Liaw via ltp]

[-- Attachment #1.1: Type: text/plain, Size: 1145 bytes --]

We are currently building with clang 14.0.6.  I haven't filed a bug report
with llvm, will work on doing that.

On Tue, Aug 16, 2022 at 5:43 AM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Edward,
>
> > Hi!
> > > I think I finally understand what you mean by this now; it is rather
> > > strange that the volatility of D does not protect loop from being
> > > optimized away by the compiler.  I don't have a good explanation as to
> > > why it's happening but I'm not sure how to evaluate what's going on
> > > either.  Should I do anything to move this patch forward?
>
> > It all boils down if we want to work around something that looks like a
> > compiler bug in tests or not. I would be inclined not to do so since LTP
> > was littered with quite a lot of workarounds for glibc/compiler bugs and
> > we spend quite some time cleaning that mess up. But in this case I can
> > agree that this is a borderland issue so I'm not strongly against that
> > either.
>
> Edward, which which clang version requires it? It'd be nice to document
> it, so
> that it can be removed in the future.
> Is there any bug report for it?
>
> Kind regards,
> Petr
>

[-- Attachment #1.2: Type: text/html, Size: 1584 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

Re: [LTP] [PATCH v1] syscalls/signal06: add volatile to loop variable

[Petr Vorel]

Hi Edward,

> We are currently building with clang 14.0.6.  I haven't filed a bug report
> with llvm, will work on doing that.
Thanks for info. I expected it'd be for aarch64 arch, but I can reproduce it on
the same clang version on x86_64 on openSUSE Tumbleweed.

Would you mind we delay merging after you fill the bug in llvm, so that we can
add it to git commit message?

Tested-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

> On Tue, Aug 16, 2022 at 5:43 AM Petr Vorel <pvorel@suse.cz> wrote:

> > Hi Edward,

> > > Hi!
> > > > I think I finally understand what you mean by this now; it is rather
> > > > strange that the volatility of D does not protect loop from being
> > > > optimized away by the compiler.  I don't have a good explanation as to
> > > > why it's happening but I'm not sure how to evaluate what's going on
> > > > either.  Should I do anything to move this patch forward?

> > > It all boils down if we want to work around something that looks like a
> > > compiler bug in tests or not. I would be inclined not to do so since LTP
> > > was littered with quite a lot of workarounds for glibc/compiler bugs and
> > > we spend quite some time cleaning that mess up. But in this case I can
> > > agree that this is a borderland issue so I'm not strongly against that
> > > either.

> > Edward, which which clang version requires it? It'd be nice to document
> > it, so
> > that it can be removed in the future.
> > Is there any bug report for it?

> > Kind regards,
> > Petr


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

Re: [LTP] [PATCH v1] syscalls/signal06: add volatile to loop variable

[Edward Liaw via ltp]

[-- Attachment #1.1: Type: text/plain, Size: 495 bytes --]

On Wed, Aug 17, 2022, 2:12 AM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Edward,
>
> > We are currently building with clang 14.0.6.  I haven't filed a bug
> report
> > with llvm, will work on doing that.
> Thanks for info. I expected it'd be for aarch64 arch, but I can reproduce
> it on
> the same clang version on x86_64 on openSUSE Tumbleweed.
>
> Would you mind we delay merging after you fill the bug in llvm, so that we
> can
> add it to git commit message?
>

Sure thing, not a problem.

>

[-- Attachment #1.2: Type: text/html, Size: 1009 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

Re: [LTP] [PATCH v1] syscalls/signal06: add volatile to loop variable

[Edward Liaw via ltp]

[-- Attachment #1.1: Type: text/plain, Size: 296 bytes --]

>
> On Wed, Aug 17, 2022, 2:12 AM Petr Vorel <pvorel@suse.cz> wrote:
>
>> Hi Edward,
>>
>> Would you mind we delay merging after you fill the bug in llvm, so that
>> we can
>> add it to git commit message?
>
>
Here's the link to the bug on llvm:
https://github.com/llvm/llvm-project/issues/57234

[-- Attachment #1.2: Type: text/html, Size: 897 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

Re: [LTP] [PATCH v1] syscalls/signal06: add volatile to loop variable

[Petr Vorel]

> > On Wed, Aug 17, 2022, 2:12 AM Petr Vorel <pvorel@suse.cz> wrote:

> >> Hi Edward,

> >> Would you mind we delay merging after you fill the bug in llvm, so that
> >> we can
> >> add it to git commit message?


> Here's the link to the bug on llvm:
> https://github.com/llvm/llvm-project/issues/57234

Thanks! The bug was closed as 'adding "cx" worked'. Reading topperc's comment it
looks like there no other way to fix the issue on clang than workaround with
volatile. Does it mean that it's a syscall problem and clang can do nothing
about it?

Kind regards,
Petr

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

Re: [LTP] [PATCH v1] syscalls/signal06: add volatile to loop variable

[Cyril Hrubis]

Hi!
> Thanks! The bug was closed as 'adding "cx" worked'. Reading topperc's comment it
> looks like there no other way to fix the issue on clang than workaround with
> volatile. Does it mean that it's a syscall problem and clang can do nothing
> about it?

It's problem with the inline assembly in the body of the while loop, the
call to the syscall changes the register value that is used for the D
variable in the case of clang, so the loop exits prematurely. We have to
add cx register to the clobber list for that asm statement so that
compiler knows that it's changed by the assembly.

Interfacing assembly with C is a bit tricky since you have to explain
to compiler which registers are changed from the assembly otherwise the
results are undefined.

The patch should look like:

diff --git a/testcases/kernel/syscalls/signal/signal06.c b/testcases/kernel/syscalls/signal/signal06.c
index 64f886ee3..78efd0fb9 100644
--- a/testcases/kernel/syscalls/signal/signal06.c
+++ b/testcases/kernel/syscalls/signal/signal06.c
@@ -73,7 +73,7 @@ void test(void)
                /* sys_tkill(pid, SIGHUP); asm to avoid save/reload
                 * fp regs around c call */
                asm ("" : : "a"(__NR_tkill), "D"(pid), "S"(SIGHUP));
-               asm ("syscall" : : : "ax");
+               asm ("syscall" : : : "ax", "cx");

                loop++;
        }

Although it may not be a complete as the llwm issue suggests we should
have a look at calling conventions for the syscall and check if we need
to add any other registers to the clobber list.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v1] syscalls/signal06: add volatile to loop variable

[Cyril Hrubis]

On Fri, Aug 19, 2022 at 10:41:23AM +0200, Cyril Hrubis wrote:
> Hi!
> > Thanks! The bug was closed as 'adding "cx" worked'. Reading topperc's comment it
> > looks like there no other way to fix the issue on clang than workaround with
> > volatile. Does it mean that it's a syscall problem and clang can do nothing
> > about it?
> 
> It's problem with the inline assembly in the body of the while loop, the
> call to the syscall changes the register value that is used for the D
> variable in the case of clang, so the loop exits prematurely. We have to
> add cx register to the clobber list for that asm statement so that
> compiler knows that it's changed by the assembly.
> 
> Interfacing assembly with C is a bit tricky since you have to explain
> to compiler which registers are changed from the assembly otherwise the
> results are undefined.
> 
> The patch should look like:
> 
> diff --git a/testcases/kernel/syscalls/signal/signal06.c b/testcases/kernel/syscalls/signal/signal06.c
> index 64f886ee3..78efd0fb9 100644
> --- a/testcases/kernel/syscalls/signal/signal06.c
> +++ b/testcases/kernel/syscalls/signal/signal06.c
> @@ -73,7 +73,7 @@ void test(void)
>                 /* sys_tkill(pid, SIGHUP); asm to avoid save/reload
>                  * fp regs around c call */
>                 asm ("" : : "a"(__NR_tkill), "D"(pid), "S"(SIGHUP));
> -               asm ("syscall" : : : "ax");
> +               asm ("syscall" : : : "ax", "cx");
> 
>                 loop++;
>         }
> 
> Although it may not be a complete as the llwm issue suggests we should
> have a look at calling conventions for the syscall and check if we need
> to add any other registers to the clobber list.

Looking at: https://en.wikibooks.org/wiki/X86_Assembly/Interfacing_with_Linux

We may as well add r11 as suggested by the clang issue comment and that
should be complete.

Edward will you send a patch?

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v1] syscalls/signal06: add volatile to loop variable

[Joerg Vehlow]

Hi,

Am 8/19/2022 um 10:41 AM schrieb Cyril Hrubis:
> Hi!
>> Thanks! The bug was closed as 'adding "cx" worked'. Reading topperc's comment it
>> looks like there no other way to fix the issue on clang than workaround with
>> volatile. Does it mean that it's a syscall problem and clang can do nothing
>> about it?
> 
> It's problem with the inline assembly in the body of the while loop, the
> call to the syscall changes the register value that is used for the D
> variable in the case of clang, so the loop exits prematurely. We have to
> add cx register to the clobber list for that asm statement so that
> compiler knows that it's changed by the assembly.
> 
> Interfacing assembly with C is a bit tricky since you have to explain
> to compiler which registers are changed from the assembly otherwise the
> results are undefined.
> 
> The patch should look like:
> 
> diff --git a/testcases/kernel/syscalls/signal/signal06.c b/testcases/kernel/syscalls/signal/signal06.c
> index 64f886ee3..78efd0fb9 100644
> --- a/testcases/kernel/syscalls/signal/signal06.c
> +++ b/testcases/kernel/syscalls/signal/signal06.c
> @@ -73,7 +73,7 @@ void test(void)
>                 /* sys_tkill(pid, SIGHUP); asm to avoid save/reload
>                  * fp regs around c call */
>                 asm ("" : : "a"(__NR_tkill), "D"(pid), "S"(SIGHUP));
> -               asm ("syscall" : : : "ax");
> +               asm ("syscall" : : : "ax", "cx");
Why is this even split up into two asm instructions?
I guess there is nothing, that prevents the compiler from reordering the
asm instructions, because it does not know, that they have side effects
(they are not marked volatile).

asm volatile ("syscall" : : "a"(__NR_tkill), "D"(pid), "S"(SIGHUP):
"rax", "rcx", "r11");


I am not sure if there is any good reason, to split this up into two asm
instructions and if there is a good reason, to use the short names of
the registers.

Joerg

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

Re: [LTP] [PATCH v1] syscalls/signal06: add volatile to loop variable

[Cyril Hrubis]

Hi!
> > It's problem with the inline assembly in the body of the while loop, the
> > call to the syscall changes the register value that is used for the D
> > variable in the case of clang, so the loop exits prematurely. We have to
> > add cx register to the clobber list for that asm statement so that
> > compiler knows that it's changed by the assembly.
> > 
> > Interfacing assembly with C is a bit tricky since you have to explain
> > to compiler which registers are changed from the assembly otherwise the
> > results are undefined.
> > 
> > The patch should look like:
> > 
> > diff --git a/testcases/kernel/syscalls/signal/signal06.c b/testcases/kernel/syscalls/signal/signal06.c
> > index 64f886ee3..78efd0fb9 100644
> > --- a/testcases/kernel/syscalls/signal/signal06.c
> > +++ b/testcases/kernel/syscalls/signal/signal06.c
> > @@ -73,7 +73,7 @@ void test(void)
> >                 /* sys_tkill(pid, SIGHUP); asm to avoid save/reload
> >                  * fp regs around c call */
> >                 asm ("" : : "a"(__NR_tkill), "D"(pid), "S"(SIGHUP));
> > -               asm ("syscall" : : : "ax");
> > +               asm ("syscall" : : : "ax", "cx");
> Why is this even split up into two asm instructions?
> I guess there is nothing, that prevents the compiler from reordering the
> asm instructions, because it does not know, that they have side effects
> (they are not marked volatile).
> 
> asm volatile ("syscall" : : "a"(__NR_tkill), "D"(pid), "S"(SIGHUP):
> "rax", "rcx", "r11");
> 
> 
> I am not sure if there is any good reason, to split this up into two asm
> instructions and if there is a good reason, to use the short names of
> the registers.

I was wondering too, I guess it's a direct copy of a reproducer from a
kernel commit see:

https://lore.kernel.org/lkml/1414436240-13879-8-git-send-email-kamal@canonical.com/

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v1] syscalls/signal06: add volatile to loop variable

[Petr Vorel]

> Hi!
> > Thanks! The bug was closed as 'adding "cx" worked'. Reading topperc's comment it
> > looks like there no other way to fix the issue on clang than workaround with
> > volatile. Does it mean that it's a syscall problem and clang can do nothing
> > about it?

> It's problem with the inline assembly in the body of the while loop, the
> call to the syscall changes the register value that is used for the D
> variable in the case of clang, so the loop exits prematurely. We have to
> add cx register to the clobber list for that asm statement so that
> compiler knows that it's changed by the assembly.

> Interfacing assembly with C is a bit tricky since you have to explain
> to compiler which registers are changed from the assembly otherwise the
> results are undefined.

> The patch should look like:

> diff --git a/testcases/kernel/syscalls/signal/signal06.c b/testcases/kernel/syscalls/signal/signal06.c
> index 64f886ee3..78efd0fb9 100644
> --- a/testcases/kernel/syscalls/signal/signal06.c
> +++ b/testcases/kernel/syscalls/signal/signal06.c
> @@ -73,7 +73,7 @@ void test(void)
>                 /* sys_tkill(pid, SIGHUP); asm to avoid save/reload
>                  * fp regs around c call */
>                 asm ("" : : "a"(__NR_tkill), "D"(pid), "S"(SIGHUP));
> -               asm ("syscall" : : : "ax");
> +               asm ("syscall" : : : "ax", "cx");

OK, I completely overlooked asm() in the test. Thanks!

Petr

>                 loop++;
>         }

> Although it may not be a complete as the llwm issue suggests we should
> have a look at calling conventions for the syscall and check if we need
> to add any other registers to the clobber list.

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

Re: [LTP] [PATCH v1] syscalls/signal06: add volatile to loop variable

[Edward Liaw via ltp]

[-- Attachment #1.1: Type: text/plain, Size: 739 bytes --]

On Fri, Aug 19, 2022 at 2:14 AM Joerg Vehlow <lkml@jv-coder.de> wrote:

> Why is this even split up into two asm instructions?
> I guess there is nothing, that prevents the compiler from reordering the
> asm instructions, because it does not know, that they have side effects
> (they are not marked volatile).
>
> asm volatile ("syscall" : : "a"(__NR_tkill), "D"(pid), "S"(SIGHUP):
> "rax", "rcx", "r11");
>

I tried this and clang complains that the "Asm-specifier for input or
output variable conflicts with asm clobber list" for rax.  Should it be

  asm volatile ("syscall" : : "a"(__NR_tkill), "D"(pid), "S"(SIGHUP) :
"rcx", "r11");

instead?  Is it implicit that __NR_tkill is going into rax so it will be
clobbered?

Thanks,
Edward

[-- Attachment #1.2: Type: text/html, Size: 1253 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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