[PATCH] D103936: [compiler-rt][hwasan] Define fuchsia implementations of required hwasan functions

[Roland McGrath via Phabricator via cfe-commits]

mcgrathr added inline comments.


================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:29
+
+uptr kHighMemEnd;
+uptr kHighMemBeg;
----------------
These need comments about what they are and why they need to exist as runtime variables at all.


================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:34
+bool InitShadow() {
+  __hwasan_shadow_memory_dynamic_address = 0;
+
----------------
What actually refers to this?
The optimal implementation for Fuchsia would just know everywhere at compile time that it's a fized value.
If there's reason for the runtime variable to exist at all, it should have comments.


================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:37
+  // This initializes __sanitizer::ShadowBounds.
+  kHighMemEnd = GetMaxUserVirtualAddress();
+  kHighMemBeg = __sanitizer::ShadowBounds.shadow_limit;
----------------
Isn't this `__sanitizer::GetMaxUserVirtualAddress()` ?


================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:59
+
+// ---------------------- TSD ---------------- {{{
+
----------------
This comment isn't very meaningful, since it only really applies to the two functions after these three.
This is also a weird comment syntax not used elsewhere in this file.



================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:62
+extern "C" {
+void __sanitizer_thread_start_hook(void *hook, thrd_t self) {
+  hwasanThreadList().CreateCurrentThread()->InitRandomState();
----------------
As we discussed before, this is insufficient plumbing for the thread tracking.
It probably makes sense to either do all the necessary refactoring for the thread tracking plumbing first, or else start this file simpler without anything related to thread tracking, and then add more stuff later.

Also, as a matter of style it's best to define C++ functions in the __hwasan namespace or a local anonymous namespace, and then have an `extern "C"` block at the end where the libc hook API implementation functions are just simple wrapper calls with no more than a line or two in them.

See asan_fuchsia.cpp for a good example of how to arrange the hook functions.




================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:74
+
+void __sanitizer_exit() { __hwasan::HwasanAtExit(); }
+}  // extern "C"
----------------
This is not the name of the hook.  `__sanitizer_process_exit_hook` is the name of the hook.  This is a good example of a simple wrapper.

However, it's unclear whether this should use the exit hook or not.  We don't use that hook in asan, we just use its normal atexit method.  In hwasan, the atexit hook doesn't really do much.  ReportStats is a no-op in hwasan, and DumpProcessMap is always a no-op on Fuchsia anyway.  So all it's really doing in practice is changing the exit code, which is what's a perfect fit for the sanitizer hook.



================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:89
+// the rest of the mismatch handling code (C++).
+extern "C" void __hwasan_tag_mismatch4(uptr addr, uptr access_info,
+                                       uptr *registers_frame, size_t outsize) {
----------------
Why isn't this just in hwasan.cpp?


================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:95
+// These are defined because they are called from __hwasan_init, but empty
+// because they are not needed.
+void HwasanOnDeadlySignal(int signo, void *info, void *context) {}
----------------
I'd put blank lines between these.
Most or all of them merit an individual comment about how Fuchsia handles things differently and thus doesn't need that particular thing.



================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:101
+void HwasanTSDThreadInit() {}
+// TODO: On linux, this was used to check that TBI is enabled. Once we work this
+// out on Fuchsia, we should come back here and do an equivalent check.
----------------
I'd put this comment inside the function.  Seems like a good idea to rename this function to something not so Linuxy, e.g. InitializeOsSupport.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103936