[compiler-rt] [sanitizer_common][PowerPC64] Fix internal_clone() error handling (PR #99908)

[Ulrich Weigand via llvm-commits]

================
@@ -1630,13 +1630,22 @@ uptr internal_clone(int (*fn)(void *), void *child_stack, int flags, void *arg,
       "sc\n\t"
 
       /* Return to parent */
+      "0:\n\t"
+      "neg %0,3\n\t"
+      "b 2f\n\t"
       "1:\n\t"
-      "mr %0, 3\n\t"
+      "mr %0,3\n\t"
+      "2:\n\t"
+
       : "=r"(res)
       : "0"(-1), "i"(EINVAL), "i"(__NR_clone), "i"(__NR_exit), "r"(__fn),
         "r"(__cstack), "r"(__flags), "r"(__arg), "r"(__ptidptr), "r"(__newtls),
         "r"(__ctidptr), "i"(FRAME_SIZE), "i"(FRAME_TOC_SAVE_OFFSET)
       : "cr0", "cr1", "memory", "ctr", "r0", "r27", "r28", "r29");
+  if ((uptr)res >= (uptr)-4095) {
----------------
uweigand wrote:

This is interesting.  First, we see that the implementation of `internal_iserror` has to match the implementation of  `internal_syscall` as far as error handling is concerned.  There seem to be two distinct approaches here:
- There is the implementation in `sanitizer_syscall_generic.inc`, which just uses the platform's `syscall` interface.  This (at least on Linux) is defined to indicate an error by a return value of -1 and an error code in `errno`.
- Some (but not all) Linux architectures use a different implementation that uses inline asm to perform the system call. On the platforms where this is done, error is indicated by a small negative value (which corresponds to the negated error code).  Most of those platforms indicate in a comment that this way is preferable as it does not clobber the libc `errno` variable.

Of course, either way is fine as long as the two match, which is the case as `internal_iserror` and `internal_syscall` are defined in the same architecture-specific file.

However, then there is one "magic" system call `internal_clone`.  There is no generic Linux version of this, but only inline asm versions.  Note that the caller of `internal_clone` uses the `internal_iserror` routine on its return value, so it implicitly expects `internal_clone` to signal errors in the same way as `internal_syscall`.   For those platforms where `internal_syscall` is also an inline asm, this assumption seems to be correct.

However, there are some platforms that use the *generic* `internal_syscall` (and `internal_iserror`) implementation *and* an inline asm version of `internal_clone` - as far as I can see, these are mips, powerpc64, i386, and s390x.  For these, there seems to be a disconnect -  the return value does *not* match the expected behavior, so a failing `internal_clone` call will likely not be detected correctly.

This bug was fixed for s390x here: https://github.com/llvm/llvm-project/commit/4c32e3d967ecbc4e7f90fef546b5ed38bbd2f7a6, but the other platforms still appear broken to me.  The PR looks like an attempt to fix powerpc64 along those same lines - note that for a complete fix, the `return -EINVAL` near the top of the function would also have to be updated.

In general, it seems that for the platforms in this group, we either need to fix `internal_clone` to behave like `syscall` (and set `errno`), or else introduce a platform-specific `internal_syscall` and `internal_iserror` that avoid touching `errno` completely (if that is preferable).


https://github.com/llvm/llvm-project/pull/99908