-
-
Yep, I have another fix with changing
__csigsetjmp
. I am going to update review shortly, when we discuss cons and pros of the current (v1) fix. I proposed this fix because it is small and simple.Question is how many applications are shipped without recompilation? If we look at OI or other distros they all do build for whole repo. Or you were asking about another applications?
-
The
setjmp()
andsigsetjmp()
routines that end up using__csigsetjmp()
are a part of our (stable) ABI. We can't change the way those work (as visible from outside), or we will break existing binaries. However it is achieved, those symbols inlibc
need to continue to work the same way as visible from applications.With respect to how many applications are shipped without recompilation, I would venture that it's a lot. Some folks, myself included, have a variety of binaries compiled quite some time ago that we have no need or desire to recompile. On a somewhat more industrial scale, Joyent produces pkgsrc binary packages that work on essentially all illumos distributions. Indeed, even in OpenIndiana, there is a mixture of short and long term usage of binaries that are not or cannot be recompiled; e.g., the closed binaries leftover from OpenSolaris, and some other bits and pieces.
I'd be inclined to put up the fix for the bug in
__csigsetjmp()
. Once that fix is in, it's not clear we would need to change the header here -- though depending on how we fix the alignment issue it may still be advantageous for new binaries. -
If we're doing an alignment thing here, that suggests that the compiler is emitting some SSE/AVX instructions assuming alignment of a structure that isn't actually aligned. Suggesting that new things potentially align themselves to improve their future performance is fine, but it feels like it's working around the problem, mainly that some function is assuming stricter it's receiving structures with stricter alignment than is actually true. One has to assume it'll always be easy for something in another language to cons this up as well, whether that's rust, golang, etc. It seems like fixing it one piece of software to handle this case, libc, is better than trying to fix anything that uses a sigjmp_buf.
-
@Joshua That makes sense. I am updating review with fix in libc. It does not require re-compilation of exisiting binaries.
@Robert That is not compiler's issue and problem is almost in the code: it doesn't follow strict aliasing rules
(cast pointer aligned to long to type aligned to double long) and by C99 standard it has UB:6.3.2.3 Pointers
7. A pointer to an object or incomplete type may be converted to a pointer to a different object or incomplete type. If the
resulting pointer is not correctly aligned57) for the pointed-to type, the behavior is undefined. -
Vitaliy, my point wasn't that it was a compiler bug, but rather that we should fix it at the source of the issue, which was in libc. I suspect this isn't going to be the last of these types of bugs in the system that are exposed as one changes optimization or tuning levels.
-
@Robert, so v2 fix (in libc) is in the right way as you proposed ?
-
Any comments?
-
-
12747 sigsetjmp should allow for 8 byte aligned buffer on amd64
Review Request #2565 — Created May 25, 2020 and updated
Information | |
---|---|
vgusev | |
illumos-gate | |
master | |
12747 | |
e0b0db8... | |
Reviewers | |
general | |
This solution fixes libc. No need recompilation of existing binaries.
Before patch:
$ /usr/bin/amd64/mdb > Segmentation FaultAfter patch:
$ LD_PRELOAD=/tmp/libc.so.1 /usr/bin/amd64/mdb > ::version mdb 1.1 (DEBUG)
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+11 -5) |
Description: |
|
||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
-
-
usr/src/lib/libc/amd64/threads/asm_subr.s (Diff revision 2) Is the reason that we're not doing any adjustments here an assumption that we're coming in with suitable alignment on the stack already and therefore we have _LONG_ALIGNMENT and don't need to further adjust the stack? It's probably worth a comment stating that we're relying on the stack's natural alignment and calling conventions for this unlike the other places.
-
usr/src/lib/libc/inc/sigjmp_struct.h (Diff revision 2) Why are both of these here? Shouldn't all we need is __amd64?
-
usr/src/lib/libc/inc/sigjmp_struct.h (Diff revision 2) I would add a comment that explains why we need this. It'd be worth understanding if SPARCv9 requires the same thing or not. Is the reason that we're not doing something similar for 32-bit systems because we're assuming that the structure natural alignment of four bytes will be the same as the pointer?
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+16 -5) |
-
Apologies for the delay in getting back to you, Vitaliy. The general code in this looks fine. Though I think it's worth improving the comments (and maybe it's worth updating the bug id with a clearer synopsis of what went wrong).
-
usr/src/lib/libc/inc/sigjmp_struct.h (Diff revision 3) Can you please call it amd64 and not x86_64 to match the rest of the system please?
Change Summary:
Use AMD64 in comment for SIGJMP2UCONTEXT
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+16 -5) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+22 -6) |
Change Summary:
Correct comment
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+22 -6) |
Change Summary:
Correct subject.
Summary: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 7 (+22 -6) |
-
Thanks for the follow ups here. I still believe we should add a comment where we've been discussing.
-
I added a more detailed explanation for the symptoms to the bug. I've also reworked the comments below, and there's a whitespace nit. Once you fix that up, I think we're good to go! Thanks for the work on this so far.
-
usr/src/lib/libc/amd64/threads/asm_subr.s (Diff revision 7) I think this comment could be clearer; e.g.,
/* * Ensure that a "ucontext_t" will fit within a "sigjmp_buf", including the * extra 8 bytes we may need for correct alignment on AMD64. */
-
usr/src/lib/libc/inc/sigjmp_struct.h (Diff revision 7) I think this comment could be clearer; e.g.,
/* * The "sigjmp_buf" type is an array of long and thus can have 8-byte alignment * on AMD64 systems. The "ucontext_t" type has a stricter 16-byte alignment * requirement, so we must round the pointer up when casting. * * This is not required on other architectures: * - SPARC does not store the ucontext_t in the sigjmp_buf * - i386 only requires 4-byte alignment for ucontext_t */
-
Change Summary:
Changes:
-
Correct commenst according to Joshua's notes.
-
Fix csctyle complaint about old code
"C style: usr/src/lib/libc/amd64/threads/machdep.c: 70:
continuation line should be indented by 4 spaces" - Fix extra space before sizeof
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+26 -7) |