Memmove fix #2151
Memmove fix #2151
Conversation
src/sysfiles.h
Outdated
| @@ -612,6 +613,9 @@ extern Obj SyReadStringFid(Int fid); | |||
| extern Obj SyReadStringFile(Int fid); | |||
| extern Obj SyReadStringFileGeneric(Int fid); | |||
|
|
|||
| // Internal implementation of memmove, to avoid issues with glibc | |||
| void *SyMemmove(void *dst, const void* src, UInt size); | |||
There was a problem hiding this comment.
It would be nice to get the memmove back inline in 64 bit environments so that small moves can be integrated with the surrounding code. Presumably that could be done by having a static inline function in the header that compiled to either memmove or a call to the real implementation.
There was a problem hiding this comment.
True, or even (which I normally disaprove) just doing #define SyMemmove memmove
There was a problem hiding this comment.
Indeed, what I'd expect here is something like this:
#ifdef USE_CUSTOM_MEMMOVE
void *SyMemmove(void *dst, const void* src, size_t size);
#else
#define SyMemmove(dst, src, size) memmove(dst, src, size)
#endifNote the use of size_t instead of UInt for size, to make sure we match the memmove signature.
src/sysfiles.h
Outdated
| @@ -612,6 +613,9 @@ extern Obj SyReadStringFid(Int fid); | |||
| extern Obj SyReadStringFile(Int fid); | |||
| extern Obj SyReadStringFileGeneric(Int fid); | |||
|
|
|||
| // Internal implementation of memmove, to avoid issues with glibc | |||
| void *SyMemmove(void *dst, const void* src, UInt size); | |||
There was a problem hiding this comment.
Indeed, what I'd expect here is something like this:
#ifdef USE_CUSTOM_MEMMOVE
void *SyMemmove(void *dst, const void* src, size_t size);
#else
#define SyMemmove(dst, src, size) memmove(dst, src, size)
#endifNote the use of size_t instead of UInt for size, to make sure we match the memmove signature.
src/sysfiles.c
Outdated
| @@ -3598,6 +3598,56 @@ Obj SyReadStringFid(Int fid) { | |||
| #endif | |||
|
|
|||
|
|
|||
| #ifdef SYS_IS_64_BIT | |||
There was a problem hiding this comment.
I dislike hurting all 32bit systems, even those which use glibc. E.g. insert this into the .h file (of course a proper configure check would be better)
#if !defined(SYS_IS_64_BIT) && defined(__GNU_LIBRARY__)
#define USE_CUSTOM_MEMMOVE 1
#endif
src/sysfiles.c
Outdated
| // The memmove in glibc on 32-bit SSE2 systems, contained in | ||
| // __memmove_sse2_unaligned, is buggy in at least versions | ||
| // 2.21 - 2.25 when crossing the 2GB boundary, so GAP must | ||
| // include it's own simple memmove implementation. |
There was a problem hiding this comment.
"it's" -> "its"
Perhaps also link to the glibc bug report. Oh, and they just release 2.26 which also has the bug.
| while (size > 0) { | ||
| *d-- = *s--; | ||
| size--; | ||
| } |
There was a problem hiding this comment.
Once again, we could really benefit from test setup for pure C code, so that one can write unit tests for functions like this.
ChrisJefferson
force-pushed
the
memmove-fix
branch
from
1bf49c1 to
9a95882
Compare
The memmove in glibc on 32-bit SSE2 systems, contained in __memmove_sse2_unaligned, is buggy in at least versions 2.21 - 2.25 when crossing the 2GB boundary, so GAP must include it's own simple memmove implementation.
ChrisJefferson
force-pushed
the
memmove-fix
branch
from
9a95882 to
3e6f4f7
Compare
|
Now fixed up (hopefully) |
| // This is about 4x slower than glibc, but | ||
| // is simple and also complicated enough that | ||
| // gcc or clang seem unable to "optimise" it back | ||
| // into a call to memmove. |
There was a problem hiding this comment.
Under which condition did you test this? E.g. did you also try it with -O3?
There was a problem hiding this comment.
I tried with -O3 and -flto. I checked using objdump -t that the resulting GAP executable doesn't have any references to memmove.
fingolfin
added
kind: bug
Codecov Report
@@ Coverage Diff @@
## master #2151 +/- ##
==========================================
+ Coverage 69.34% 69.39% +0.04%
==========================================
Files 484 485 +1
Lines 253917 255460 +1543
==========================================
+ Hits 176085 177266 +1181
- Misses 77832 78194 +362
|
fingolfin
added
release notes: added
This is a basic attempt to fix the problem of broken memmove on 32-bit glibc.
This is purposefully designed to be as simple as possible. We could obviously do various cleverer things than this, but I find the bug hard to test, so I didn't want to do anything particularly complicated.