[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;
----------------
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