leonardchan added inline comments.
================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:29
+
+uptr kHighMemEnd;
+uptr kHighMemBeg;
----------------
mcgrathr wrote:
> These need comments about what they are and why they need to exist as runtime
> variables at all.
`kHighMemEnd` and `kHighMemBeg` are used only by `MemIsApp` which is only used
in `hwasan_thread.cpp` for some debugging checks:
```
if (stack_bottom_) {
int local;
CHECK(AddrIsInStack((uptr)&local));
CHECK(MemIsApp(stack_bottom_));
CHECK(MemIsApp(stack_top_ - 1));
}
```
Rather than having these, we could just use their values
(`__sanitizer::ShadowBounds.memory_limit - 1` and
`__sanitizer::ShadowBounds.shadow_limit`) directly in `MemIsApp` to avoid these
globals.
`kAliasRegionStart` is used in `HwasanAllocatorInit` for setting up the
allocator, but is only relevant for an experimental x86 implementation that
uses page aliasing for placing tags in heap allocations (see D98875). I didn't
look too much into the machinery for this since I didn't think we would be
using hwasan for x86 anytime soon, but we can explore this now if we want to
plan ahead. We could also make it such that this is just defined as 0 on x86
platforms, similar to `SHADOW_OFFSET` in asan.
================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:34
+bool InitShadow() {
+ __hwasan_shadow_memory_dynamic_address = 0;
+
----------------
mcgrathr wrote:
> 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.
It's only used for converting between application memory and shadow memory:
```
// hwasan_mapping.h
inline uptr MemToShadow(uptr untagged_addr) {
return (untagged_addr >> kShadowScale) +
__hwasan_shadow_memory_dynamic_address;
}
inline uptr ShadowToMem(uptr shadow_addr) {
return (shadow_addr - __hwasan_shadow_memory_dynamic_address) << kShadowScale;
}
```
Perhaps we could have something similar to the `SHADOW_OFFSET` macro in asan
where it can be defined to either a constant or
`__hwasan_shadow_memory_dynamic_address` on different platforms and these
functions can just use the macro.
================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:37
+ // This initializes __sanitizer::ShadowBounds.
+ kHighMemEnd = GetMaxUserVirtualAddress();
+ kHighMemBeg = __sanitizer::ShadowBounds.shadow_limit;
----------------
mcgrathr wrote:
> Isn't this `__sanitizer::GetMaxUserVirtualAddress()` ?
It is. It looks there's
```
namespace __hwasan {
using namespace __sanitizer;
}
```
in `sanitizer_internal_defs.h` included through `hwasan.h`. Will add the
`__sanitizer::` bits.
================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:59
+
+// ---------------------- TSD ---------------- {{{
+
----------------
mcgrathr wrote:
> 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.
>
Removed.
================
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();
----------------
mcgrathr wrote:
> 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.
>
>
Moved these in D104085 and left the remaining functions here.
================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:74
+
+void __sanitizer_exit() { __hwasan::HwasanAtExit(); }
+} // extern "C"
----------------
mcgrathr wrote:
> 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.
>
Ah, if the functions in `HwasanAtExit` are no-ops and it only sets the exit
code to 1 with `internal__exit` on an error, then it might not be necessary to
have this here.
================
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) {
----------------
mcgrathr wrote:
> Why isn't this just in hwasan.cpp?
Removed from this and will move to hwasan.cpp in a separate patch. I imagine
other platforms would've had their own versions of this we didn't want to
centralize, but it looks like they all share the linux one.
================
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.
----------------
mcgrathr wrote:
> 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.
>
Removed and will rename in a separate patch.
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
