diff mbox series

[uclibc-ng-devel] arc patch from glibc repo

Message ID 20171105100054.GU1829@waldemar-brodkorb.de
State Rejected
Headers show
Series [uclibc-ng-devel] arc patch from glibc repo | expand

Commit Message

Waldemar Brodkorb Nov. 5, 2017, 10 a.m. UTC 
Hi Alexey,

the attached patch fixes at least 3 test suite errors for me.
tst-cancel20/21/4.
Tested with latest 2017.09 ARC binutils/gcc.

Okay to apply?

best regards
 Waldemar

Comments

Cupertino Miranda Nov. 6, 2017, 10:38 a.m. UTC | #1 
Hi Waldemar,

Can you share in which context does this happens, for example by sharing
a .i file for the compilation and point out for the actual location of
the macro expansion?
The reason to request this is to understand if the compiler is not doing
the right thing with blink, as semantically the patched and unpatched
code should be the same, as long as the surrounding code is correct.

It could also be that the problem is not necessarily the blink not being
clobbered but by some other reason it not expecting it to be needed.

Best regards,
Cupertino

On 11/05/2017 11:03 AM, Waldemar Brodkorb wrote:
> Hi Alexey,
>
> the attached patch fixes at least 3 test suite errors for me.
> tst-cancel20/21/4.
> Tested with latest 2017.09 ARC binutils/gcc.
>
> Okay to apply?
>
> best regards
>  Waldemar
>
Waldemar Brodkorb Nov. 9, 2017, 7:17 a.m. UTC | #2 
Hi Cupertino,
Cupertino Miranda wrote,

> Hi Waldemar,
> 
> Can you share in which context does this happens, for example by sharing
> a .i file for the compilation and point out for the actual location of
> the macro expansion?
> The reason to request this is to understand if the compiler is not doing
> the right thing with blink, as semantically the patched and unpatched
> code should be the same, as long as the surrounding code is correct.
> 
> It could also be that the problem is not necessarily the blink not being
> clobbered but by some other reason it not expecting it to be needed.

I tried to generate the files for you, but had updated the Linux
kernel before your mail. It seems now that the same test suite tests
are executing fine without the blink removal patch.

Is there any related bugfix made between Linux 4.9.50 and 4.9.60
someone remembers? Strange, I need to verify with 4.9.50 again.

best regards
 Waldemar
Cupertino Miranda Nov. 9, 2017, 1:18 p.m. UTC | #3 
Hi Waldemar,

I personally made this tst-cancel test start to pass in the past,
related to unwinding issues we had in our tools.
However, I have noticed that some of the tst-cancel tests would have
some non-deterministic behavior.

This was related to some pipes not blocking, when writing some data, but
not blocking.
If I increased the written elements to the pipe, those would block
always, and the test would pass.

It is possible that there was some change in the kernel that might have
fixed this.
Vineet: Do you know anything about this ?

Also be sure you are using our latest tools, as like I said I have
addressed these problems.
Indeed, clobbering blink was an issue when millicode option was enabled.

Regarding your patch I would advise not to apply it until we find the
real reason for blink not to be restored.

Best regards,
Cupertino

On 11/09/2017 08:18 AM, Waldemar Brodkorb wrote:
> Hi Cupertino,
> Cupertino Miranda wrote,
>
>> Hi Waldemar,
>>
>> Can you share in which context does this happens, for example by sharing
>> a .i file for the compilation and point out for the actual location of
>> the macro expansion?
>> The reason to request this is to understand if the compiler is not doing
>> the right thing with blink, as semantically the patched and unpatched
>> code should be the same, as long as the surrounding code is correct.
>>
>> It could also be that the problem is not necessarily the blink not being
>> clobbered but by some other reason it not expecting it to be needed.
> I tried to generate the files for you, but had updated the Linux
> kernel before your mail. It seems now that the same test suite tests
> are executing fine without the blink removal patch.
>
> Is there any related bugfix made between Linux 4.9.50 and 4.9.60
> someone remembers? Strange, I need to verify with 4.9.50 again.
>
> best regards
>  Waldemar
>
Vineet Gupta Nov. 9, 2017, 7:09 p.m. UTC | #4 
On 11/09/2017 05:18 AM, Cupertino Miranda wrote:
> Hi Waldemar,
>
> I personally made this tst-cancel test start to pass in the past,
> related to unwinding issues we had in our tools.
> However, I have noticed that some of the tst-cancel tests would have
> some non-deterministic behavior.
>
> This was related to some pipes not blocking, when writing some data, but
> not blocking.
> If I increased the written elements to the pipe, those would block
> always, and the test would pass.
>
> It is possible that there was some change in the kernel that might have
> fixed this.
> Vineet: Do you know anything about this ?

Not in ARC port per se.

> Regarding your patch I would advise not to apply it until we find the
> real reason for blink not to be restored.

Right. Although in SNPS github uclibc repo we carry an equivalent patch (from 
Cuper) for fixing some of the cancelation tests. That is the only out of tree 
patch we have in uClibc just because we (I) didn't have time to analyze things 
fully. However I'm planning to clean this up and prove that we need that patch 
(and submit it for merge) or prove it is useless - which is quite unlikely - since 
Cuper is generally NOT smoking pot when coding ;-)

OTOH, while we do need this analysis, the change itself seemed relatively benign, 
but was kept on hold due to reasons needing a historic detour (read below if 
interested)

In 2010 ? I rewrote the uClibc syscall generators. To reduce generated code bloat 
and stack spills for prologue/epilogue etc ernro handling was moved into an 
out-of-line function. For fast path case of a simpler successful syscalls (no 
errno), no stack operations were needed.

    0000c244 <__libc_nanosleep>:
         c244:    mov       r8,162
         c248:    swi
         c24c:    brge       r0,0,c25c
         c250:    st.a        blink, [sp,-4]     # this blink save is from inline
    asm - not gcc
         c254:    bl           ad5c                 # this is errno setter
         c258:    ld.ab      blink, [sp,4]
         c25c:    j_s        [blink]
         c25e:    nop_s


The proposed change adds blink to clobber list, making BLINK saved twice now (once 
by gcc and other due to above) and also undoes all the hand crafted beauty above.

-Vineet
Vineet Gupta Dec. 13, 2017, 9:31 p.m. UTC | #5 
On 11/05/2017 02:03 AM, Waldemar Brodkorb wrote:
> Hi Alexey,
>
> the attached patch fixes at least 3 test suite errors for me.
> tst-cancel20/21/4.
> Tested with latest 2017.09 ARC binutils/gcc.
>
> Okay to apply?
>
> best regards
>   Waldemar

Are you sure it is the list above.

I presume, 
https://tests.embedded-test.org/uClibc-ng/1.0.26/REPORT.arcv2.libc.uClibc-ng-1.0.26
didn't have this fix and yet the tests mentioned above are PASS there.

The reason I ask is because in my experiments here with ARC GNU 2017.09 + uClibc 
with and w/o this patch (actually not exactly same, but similar - attached), I 
don't see any consistent PASS(es)
There are some changes to say tst-kill6, tst-oncex3 etc but if you run them 5 
times they seem to PASS/FAIL randomly - in either setups

So this patch is not fixing anything IMO. We need to really fix the inherent race 
in these tests.

BTW I did post a test case fix for tst-cancel2 and that is now consistently PASS.

-Vineet
From 055c35bed580c8946feaba4ba8a1f61b550f48f6 Mon Sep 17 00:00:00 2001
From: Cupertino Miranda <cmiranda@synopsys.com>
Date: Tue, 21 Feb 2017 17:05:22 +0100
Subject: [PATCH] Patch to clobber blink that works.

---
 libc/sysdeps/linux/arc/bits/syscalls.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libc/sysdeps/linux/arc/bits/syscalls.h b/libc/sysdeps/linux/arc/bits/syscalls.h
index 248ef78..92c1035 100644
--- a/libc/sysdeps/linux/arc/bits/syscalls.h
+++ b/libc/sysdeps/linux/arc/bits/syscalls.h
@@ -68,6 +68,7 @@ extern int __syscall_error (int);
 #define INLINE_SYSCALL(name, nr_args, args...)				\
 ({									\
 	register int __res __asm__("r0");				\
+	__asm__ volatile ("" : : : "blink");  \
 	__res = INTERNAL_SYSCALL_NCS(__NR_##name, , nr_args, args);	\
 	if (__builtin_expect (INTERNAL_SYSCALL_ERROR_P ((__res), ), 0))	\
 	{								\
@@ -81,6 +82,7 @@ extern int __syscall_error (int);
 #define INLINE_SYSCALL_NCS(num, nr_args, args...)			\
 ({									\
 	register int __res __asm__("r0");				\
+	__asm__ volatile ("" : : : "blink");  \
 	__res = INTERNAL_SYSCALL_NCS(num, , nr_args, args);		\
 	if (__builtin_expect (INTERNAL_SYSCALL_ERROR_P ((__res), ), 0))	\
 	{								\
Waldemar Brodkorb Dec. 16, 2017, 7:56 p.m. UTC | #6 
Hi Vineet,
Vineet Gupta wrote,

> On 11/05/2017 02:03 AM, Waldemar Brodkorb wrote:
> >Hi Alexey,
> >
> >the attached patch fixes at least 3 test suite errors for me.
> >tst-cancel20/21/4.
> >Tested with latest 2017.09 ARC binutils/gcc.
> >
> >Okay to apply?
> >
> >best regards
> >  Waldemar
> 
> Are you sure it is the list above.
> 
> I presume, https://tests.embedded-test.org/uClibc-ng/1.0.26/REPORT.arcv2.libc.uClibc-ng-1.0.26
> didn't have this fix and yet the tests mentioned above are PASS there.
> 
> The reason I ask is because in my experiments here with ARC GNU 2017.09 +
> uClibc with and w/o this patch (actually not exactly same, but similar -
> attached), I don't see any consistent PASS(es)
> There are some changes to say tst-kill6, tst-oncex3 etc but if you run them
> 5 times they seem to PASS/FAIL randomly - in either setups
> 
> So this patch is not fixing anything IMO. We need to really fix the inherent
> race in these tests.
> 
> BTW I did post a test case fix for tst-cancel2 and that is now consistently PASS.

Indeed it seems really to depend on which system I have run the
tests with and without the patch.

On my IBM X200 notebook the tests passed after adding the patch.
But on the gcc farm server the tests were still failing.

So it is like you said, the patch does not fix the issue.

best regards
 Waldemar
diff mbox series

Patch

From 761b5a4fd0aa8327738223b00c72ff53e7ab171f Mon Sep 17 00:00:00 2001
From: Waldemar Brodkorb <wbx@openadk.org>
Date: Sun, 5 Nov 2017 10:50:31 +0100
Subject: [PATCH] arc: fix some thread cancelation problems

Following a recent ARC glibc commit
db1d4d56c71181703917e8c22516ada9fbff5879

Signed-off-by: Waldemar Brodkorb <wbx@openadk.org>
---
 libc/sysdeps/linux/arc/bits/syscalls.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/libc/sysdeps/linux/arc/bits/syscalls.h b/libc/sysdeps/linux/arc/bits/syscalls.h
index 248ef7844..ab69cf6b3 100644
--- a/libc/sysdeps/linux/arc/bits/syscalls.h
+++ b/libc/sysdeps/linux/arc/bits/syscalls.h
@@ -45,14 +45,12 @@  extern int __syscall_error (int);
 
 #define ERRNO_ERRANDS(_sys_result)          \
         __asm__ volatile (                  \
-        "st.a blink, [sp, -4] \n\t"         \
         CALL_ERRNO_SETTER                   \
-        "ld.ab blink, [sp, 4] \n\t"         \
         :"+r" (_sys_result)                 \
         :                                   \
         :"r1","r2","r3","r4","r5","r6",     \
-         "r7","r8","r9","r10","r11","r12"   \
-        );
+         "r7","r8","r9","r10","r11","r12",  \
+	 "blink");
 
 #endif /* IS_IN_rtld */
 
-- 
2.11.0