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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
