[PATCH] D48509: Improve crash unwinding on Fuchsia

[Roland McGrath via Phabricator via llvm-commits]

mcgrathr added a comment.

Almost there, but some nits.



================
Comment at: lib/fuzzer/FuzzerUtilFuchsia.cpp:44
+// Forward declaration of assembly trampoline needed to resume crashed threads.
+void CrashTrampolineAsm() __asm__("CrashTrampolineAsm");
+
----------------
Clarify in the comment that this appears to have external linkage to C++ and that's why it's not in the anonymous namespace, but the assembly definition inside MakeTrampoline(), below, actually defines the symbol with internal linkage only.


================
Comment at: lib/fuzzer/FuzzerUtilFuchsia.cpp:59
+  } else {
+    Printf("libFuzzer: %s succeeded\n", Syscall);
+  }
----------------
You don't want this printf for the success case in production code.


================
Comment at: lib/fuzzer/FuzzerUtilFuchsia.cpp:82
+// For the crash handler, we need to call Fuzzer::StaticCrashSignalCallback
+// without POSIX signal handlers.  To achieve this, we use and assembly function
+// to add the necessary CFI unwinding information and a C function to bridge
----------------
typo: s/and/an/


================
Comment at: lib/fuzzer/FuzzerUtilFuchsia.cpp:84
+// to add the necessary CFI unwinding information and a C function to bridge
+// from that back into C++/
+
----------------
typo: trailing slash should be period


================
Comment at: lib/fuzzer/FuzzerUtilFuchsia.cpp:89
+// unwinding and expose the necessary APIs through sanitizer_common and/or ASAN
+// to allow the exception handling thread to gather the crash state directly.
+#if defined(__x86_64__)
----------------
Alternatively, Fuchsia may in future actually implement basic signal handling for the machine trap signals.


================
Comment at: lib/fuzzer/FuzzerUtilFuchsia.cpp:142
+  OP_NUM(29)                             \
+  OP_REG(lr)                             \
+  OP_REG(sp)
----------------
lr is not special, it's just an alias for x30 so I'd use OP_NUM(30) here.
Only sp (and pc) are really special.


================
Comment at: lib/fuzzer/FuzzerUtilFuchsia.cpp:157
+#define ASM_OPERAND_NUM(num)                                 \
+  [r##num] "i"(offsetof(zx_thread_state_general_regs_t, r) + \
+               (num * sizeof(uint64_t))),
----------------
This can use offsetof(..., r[num]) instead of local arithmetic.  It's not standard but it works in Clang and GCC and always will.


================
Comment at: lib/fuzzer/FuzzerUtilFuchsia.cpp:163
+__attribute__((noreturn))
+extern "C" void StaticCrashHandler() {
   Fuzzer::StaticCrashSignalCallback();
----------------
Make this static (no extern "C") and pass it into the asm.


================
Comment at: lib/fuzzer/FuzzerUtilFuchsia.cpp:171
+// Creates the trampoline with the necessary CFI information to unwind through
+// to the crashing call stack.
+__attribute__((used))
----------------
Clarify that this function is never really used as such and that's why the attribute is necessary.
It's just a container around the __asm__ so it can use operands for compile-time computed constants.


================
Comment at: lib/fuzzer/FuzzerUtilFuchsia.cpp:174
+void MakeTrampoline() {
+  uint64_t dummy = 0;  // ignored; used to swallow the trailing comma.
+  __asm__(".cfi_endproc\n"
----------------
No need for a variable here.


================
Comment at: lib/fuzzer/FuzzerUtilFuchsia.cpp:179
+"CrashTrampolineAsm:\n"
+    ".cfi_startproc\n"
+    ".cfi_signal_frame\n"
----------------
This can be ".cfi_startproc simple" because you supply all the details explicitly.


================
Comment at: lib/fuzzer/FuzzerUtilFuchsia.cpp:185
+    FOREACH_REGISTER(CFI_OFFSET_REG, CFI_OFFSET_NUM)
+    "call StaticCrashHandler\n"
+    "ud2\n"
----------------
This can be "call %c[StaticCrashHandler]" with the operand being "i' (StaticCrashHandler).


================
Comment at: lib/fuzzer/FuzzerUtilFuchsia.cpp:192
+    FOREACH_REGISTER(CFI_OFFSET_REG, CFI_OFFSET_NUM)
+    "bl StaticCrashHandler\n"
+#else
----------------
This can be "bl %[StaticCrashHandler]" with the operand being "i' (StaticCrashHandler).
Make this the last operand and it solves to comma-eating problem too.


================
Comment at: lib/fuzzer/FuzzerUtilFuchsia.cpp:205
+#endif
+      "i" (dummy));
+}
----------------
Just use "i" (0) here.


================
Comment at: lib/fuzzer/FuzzerUtilFuchsia.cpp:212
+  struct ScopedCrashContext {
+    ScopedCrashContext() : Port(ZX_HANDLE_INVALID), Thread(ZX_HANDLE_INVALID) {}
+    ~ScopedCrashContext() {
----------------
Can use initialized member decls and default constructor.
I'd make a ScopedHandle instead and just have two of them.


================
Comment at: lib/fuzzer/FuzzerUtilFuchsia.cpp:261
+      -(uintptr_t)16;
+  memcpy(reinterpret_cast<void *>(StackPtr), &GeneralRegisters,
+         sizeof(GeneralRegisters));
----------------
This should use __unsanitized_memcpy from <zircon/sanitizer.h>.
Otherwise it's theoretically possible for this thread to reenter the sanitizer runtime and that could go badly.


================
Comment at: lib/fuzzer/FuzzerUtilFuchsia.cpp:267
+#elif defined(__aarch64__)
+  uintptr_t StackPtr = GeneralRegisters.sp - sizeof(GeneralRegisters);
+  memcpy(reinterpret_cast<void *>(StackPtr), &GeneralRegisters,
----------------
Also needs rounded down to modulo 16.


================
Comment at: lib/fuzzer/FuzzerUtilFuchsia.cpp:307
 
-  // Create an exception port
-  zx_handle_t *ExceptionPort = new zx_handle_t;
-  if ((rc = _zx_port_create(0, ExceptionPort)) != ZX_OK) {
-    Printf("libFuzzer: zx_port_create failed: %s\n", _zx_status_get_string(rc));
-    exit(1);
-  }
+  // Set up the crash handler and wait until its ready before proceeding.
+  zx_handle_t Event;
----------------
typo: s/its/it's/


https://reviews.llvm.org/D48509