mcgrathr added inline comments.
================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:29
+
+uptr kHighMemEnd;
+uptr kHighMemBeg;
----------------
leonardchan wrote:
> 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.
MemIsApp is defined in this file so don't define any globals on its account (if
it needs anything, make it static in this file).
HWASAN_ALIASING_MODE is not supported for Fuchsia and we don't need to make
sure it compiles. Just leave out anything related to it entirely for now.
================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:34
+bool InitShadow() {
+ __hwasan_shadow_memory_dynamic_address = 0;
+
----------------
leonardchan wrote:
> 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.
That sounds good. But we can make that a separate refactoring cleanup of its
own. It's fine to just define it to 0 here and leave a TODO comment about
removing the runtime variable altogether on Fuchsia.
That's a refactoring you could either land separately on its own first before
landing the Fuchsia port, or do afterwards as a separate cleanup change.
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
