-
Notifications
You must be signed in to change notification settings - Fork 984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
arm32le: segfault in lookup_static() with musl libc #622
Comments
That's probably not the best assumption... arm32le is not tested in GitHub automation, and the last work that I know for sure was done on it was for a project that is now defunct. I'm sure it was working and tested at some point, but bit rot may have set in. Hopefully someone more familiar with that part of Chez will be able to take a look. |
Unfortunately, |
I debugged this a bit further and one problem seems to be the implementation of the hash function Lines 88 to 94 in e47faca
On I am not entirely sure why the
|
As it turns out, the problem is that GCC optimizes out the 0002d09c <symhash>:
2d09c: e92d4010 push {r4, lr}
2d0a0: e1a04000 mov r4, r0
2d0a4: ebff6bc8 bl 7fcc <strlen@plt>
2d0a8: e0842000 add r2, r4, r0
2d0ac: e1520004 cmp r2, r4
2d0b0: 1a000003 bne 2d0c4 <symhash+0x28>
2d0b4: e59f1018 ldr r1, [pc, #24] ; 2d0d4 <symhash+0x38>
2d0b8: eb00aeee bl 58c78 <__aeabi_idivmod>
2d0bc: e1a00001 mov r0, r1
2d0c0: e8bd8010 pop {r4, pc}
2d0c4: e4d41001 ldrb r1, [r4], #1
2d0c8: e0803080 add r3, r0, r0, lsl #1
2d0cc: e0810003 add r0, r1, r3
2d0d0: eafffff5 b 2d0ac <symhash+0x10>
2d0d4: 000001c9 andeq r0, r0, r9, asr #3 This also explains the return value of the symhash function in the GDB session from my previous comment ( My hypothesis is that GCC perform this optimization because --- a/c/foreign.c
+++ b/c/foreign.c
@@ -89,7 +89,7 @@ static iptr symhash(const char *s) {
iptr n, h;
h = n = strlen(s);
- while (n--) h = h * multiplier + *s++;
+ while (n--) h = h * multiplier + (signed char)*s++;
return (h & 0x7fffffff) % buckets;
} This is compiled as follows: 0002d09c <symhash>:
2d09c: e92d4010 push {r4, lr}
2d0a0: e1a04000 mov r4, r0
2d0a4: ebff6bc8 bl 7fcc <strlen@plt>
2d0a8: e0842000 add r2, r4, r0
2d0ac: e1540002 cmp r4, r2
2d0b0: 1a000004 bne 2d0c8 <symhash+0x2c>
2d0b4: e59f101c ldr r1, [pc, #28] ; 2d0d8 <symhash+0x3c>
2d0b8: e3c00102 bic r0, r0, #-2147483648 ; 0x80000000
2d0bc: eb00aeef bl 58c80 <__aeabi_idivmod>
2d0c0: e1a00001 mov r0, r1
2d0c4: e8bd8010 pop {r4, pc}
2d0c8: e0d410d1 ldrsb r1, [r4], #1
2d0cc: e0803080 add r3, r0, r0, lsl #1
2d0d0: e0810003 add r0, r1, r3
2d0d4: eafffff4 b 2d0ac <symhash+0x10>
2d0d8: 000001c9 andeq r0, r0, r9, asr #3 In this disassembly |
This is a really nice rundown of the issue. Thank you for figuring this out! I had a couple of false starts installing Alpine Linux on my Pi, but I put some time into it--unfortunately to no avail. It looks like the That said, I will install a more up-to-date glib-based distro on my Pi, and if the changes work, then I'm comfortable going ahead with #625. |
Any updates on this? I personally just reproduced this using |
I resolved the new entries with in the LOG, and once the mats pass, I'll merge #625 today. |
Without this patch, the loop body can easily cause an integer overflow. In this case, it is possible for the local variable h to take a negative value. The bitwise-and with 0x7fffffff is supposed to handle this case by setting the sign-bit to zero. However, GCC may optimize out the bitwise-and if char is an unsigned value since it assumes that signed integer overflow does not take place and in this case arithmetic on h would only be performed on unsigned values. This is, for example, the case on the ARMv6 architecture. On ARMv6, symhash might thus return negative values, which can lead to a segmentation fault when they are used to index vectors etc. This commit attempts to address this issue by rewriting the code in a way that it doesn't rely on undefined behavior (i.e. signed integer overflow). Alternatively, it might also be possible to cast the local variable s explicitly to a signed type to prevent the bitwise-and with 0x7fffffff from being optimized out. However, such a change would still rely on undefined behavior. Fixes #622
Hello, I am a contributor to the Alpine Linux distribution (which uses musl libc). I am currently trying to improve our ChezScheme package. As part of this effort, I am trying to make the package available for more architectures (currently it is only supported on x86/x86_64). I started working on a port for armv6. While working on this, I ran into the following segfault during the build process on armv6:
To me, it looks like it segfaults when iterating through the
S_G.foreign_static
vector inlookup_static
for the(cs)lookup_foreign_entry
symbol:ChezScheme/c/foreign.c
Lines 100 to 102 in e47faca
Since
lookup_static
is called fromS_foreign_init
here I would have assumed the vector to be only filled withSnil
but for some reasonScar(p)
ends up creating a pointer which causes a segmentation fault on dereference. Since I am not an expert on the ChezScheme code base I am presently not sure what is causing this and was hoping someone with more advanced knowledge of the ChezScheme code might have an idea what could be causing the segfault (it is likely musl-related since I assume that arm32le is well tested in conjunction with glibc).The full build log: alpine-edge-chez-scheme-armhf.txt
PS: I also had to slightly patch the code to address #527 and #621.
The text was updated successfully, but these errors were encountered: