Jump to conversation
Unresolved conversations (0)
Nice work!

Nice work!

All of your conversations have been resolved.

Resolved conversations (4)
@jyknight jyknight Oct 15, 2024
(Of course this review isn't about the x86-64 longjmp, but just I see it's also broken similarly.)
Outdated
libc/src/setjmp/x86_64/longjmp.cpp
nickdesaulniers
@jyknight jyknight Oct 15, 2024
I believe this asm is wrong: the input values could be referring to the registers you're writing in the asm body. You can add explicit clobber constraints for ebx/esi/edi, but I don't think you can validly clobber ebp/esp, which I believe thus means you can't use an "m" constraint at all, because the address could be an offset to ebp/esp. You can instead pass the field offsets via "i" constraints, and a specific register that holds buf. (I know it was already discussed and decided against, but I do want to say that I still do think it'd be safer/clearer/simpler to just use raw asm for these functions -- because being careful to ensure that you're correctly following rules of inline-asm for functions which are explicitly about breaking the typical rules (...in a controlled manner...) is _hard_!)
libc/src/setjmp/x86_64/longjmp.cpp
jyknight nickdesaulniers
@nickdesaulniers nickdesaulniers Oct 15, 2024
Fortunately/unfortunately, I could not reinterpret_cast `buf_eip` to a no-return function pointer and call it.
Outdated
libc/src/setjmp/x86_64/longjmp.cpp
@nickdesaulniers nickdesaulniers Oct 15, 2024
We can avoid a specific clobber by putting `buf` param as an input: ```diff diff --git a/libc/src/setjmp/x86_64/longjmp.cpp b/libc/src/setjmp/x86_64/longjmp.cpp index f27e5f055d6f..d0145e5f4fe9 100644 --- a/libc/src/setjmp/x86_64/longjmp.cpp +++ b/libc/src/setjmp/x86_64/longjmp.cpp @@ -19,17 +19,15 @@ namespace LIBC_NAMESPACE_DECL { #ifdef __i386__ [[noreturn]] -LLVM_LIBC_FUNCTION(void, longjmp, (jmp_buf, int val)) { +LLVM_LIBC_FUNCTION(void, longjmp, (jmp_buf buf, int val)) { asm(R"( - mov 4(%%esp), %%edx - - mov %c[ebx](%%edx), %%ebx - mov %c[esi](%%edx), %%esi - mov %c[edi](%%edx), %%edi - mov %c[ebp](%%edx), %%ebp - mov %c[esp](%%edx), %%esp + mov %c[ebx](%[buf]), %%ebx + mov %c[esi](%[buf]), %%esi + mov %c[edi](%[buf]), %%edi + mov %c[ebp](%[buf]), %%ebp + mov %c[esp](%[buf]), %%esp - jmp *%c[eip](%%edx) + jmp *%c[eip](%[buf]) )" :: [ebx] "i"(offsetof(__jmp_buf, ebx)), @@ -38,6 +36,7 @@ LLVM_LIBC_FUNCTION(void, longjmp, (jmp_buf, int val)) { [ebp] "i"(offsetof(__jmp_buf, ebp)), [esp] "i"(offsetof(__jmp_buf, esp)), [eip] "i"(offsetof(__jmp_buf, eip)), + [buf] "r"(buf), [val] "a"(val == 0 ? 1 : val) : "edx"); __builtin_unreachable(); ```
Outdated
libc/src/setjmp/x86_64/longjmp.cpp
nickdesaulniers