| Message ID | 20220712173921.2623135-1-edliaw@google.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1] syscalls/signal06: add volatile to loop variable | expand |
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
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.
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
> > 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
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.
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
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 >
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
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. >
> > 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
> > 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
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.
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?
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
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/
> 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.
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
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;
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(-)