[PATCH] D91466: [WIP][clang][Fuchsia] Support HWASan for Fuchsia

[Leonard Chan via Phabricator via cfe-commits]

leonardchan planned changes to this revision.
leonardchan added inline comments.


================
Comment at: compiler-rt/lib/hwasan/hwasan_dynamic_shadow.cpp:120
+
+void InitShadowGOT() {}
+
----------------
Add comment. Fuchsia never uses dynamic shadow, which is what this sets up in the Linux implementation.

>From linux implemenation:
```
  // Call the ifunc resolver for __hwasan_shadow and fill in its GOT entry. This
  // needs to be done before other ifunc resolvers (which are handled by libc)
  // because a resolver might read __hwasan_shadow.
```


================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:46-51
+void InitThreads() {
+  uptr alloc_size = UINT64_C(1) << kShadowBaseAlignment;
+  uptr thread_start = reinterpret_cast<uptr>(
+      MmapAlignedOrDieOnFatalError(alloc_size, alloc_size, __func__));
+  InitThreadList(thread_start, thread_start + alloc_size);
+}
----------------
Compare hwasan thread tracking implementation with asan + fuchsia implementation. HWASan uses different sanitizer machinery.


================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:53
+
+void InitPrctl() {}
+
----------------
This can be replaced with a zircon-equivalent check that checks if TBI is enabled. This function is Linux-specific.

TODO: File a bug that tracks this once the TBI stuff is addressed.


================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:60
+
+void InstallAtExitHandler() {}
+
----------------
See equivalent for other asan+lsan implementations. We use fuchsia sanitizer hooks instead of atexit. `__sanitizer_exit_hook` is what's used.


================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:64-74
+extern "C" void __hwasan_thread_enter() {
+  hwasanThreadList().CreateCurrentThread()->InitRandomState();
+}
+
+extern "C" void __hwasan_thread_exit() {
+  Thread *t = GetCurrentThread();
+  // Make sure that signal handler can not see a stale current thread pointer.
----------------
Don't define these. Move this logic into the hooks.

Ask why these are in the public header when we already have the thread hooks.


================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:105-111
+struct AccessInfo {
+  uptr addr;
+  uptr size;
+  bool is_store;
+  bool is_load;
+  bool recover;
+};
----------------
leonardchan wrote:
> This could probably be shared code with the Linux implementation. Move out of hwasan.
This is compiler-defined and should be abstracted out. Shouldn't have duplicate definitions.


================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:105-130
+struct AccessInfo {
+  uptr addr;
+  uptr size;
+  bool is_store;
+  bool is_load;
+  bool recover;
+};
----------------
This could probably be shared code with the Linux implementation. Move out of hwasan.


================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:146-147
+                                       uptr *registers_frame, size_t outsize) {
+  if (!hwasan_inited)
+    return;
+
----------------
Ideally, the checks on these uninitialized globals should not be called.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91466/new/

https://reviews.llvm.org/D91466