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

[Leonard Chan via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
leonardchan marked 2 inline comments as done.
Closed by commit rGa11aea68a4b3: [compiler-rt][hwasan] Define fuchsia 
implementations of required hwasan… (authored by leonardchan).

Changed prior to commit:
  https://reviews.llvm.org/D103936?vs=352194=357269#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103936

Files:
  compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
  compiler-rt/lib/sanitizer_common/sanitizer_fuchsia.cpp


Index: compiler-rt/lib/sanitizer_common/sanitizer_fuchsia.cpp
===
--- compiler-rt/lib/sanitizer_common/sanitizer_fuchsia.cpp
+++ compiler-rt/lib/sanitizer_common/sanitizer_fuchsia.cpp
@@ -156,8 +156,10 @@
 
 sanitizer_shadow_bounds_t ShadowBounds;
 
+void InitShadowBounds() { ShadowBounds = __sanitizer_shadow_bounds(); }
+
 uptr GetMaxUserVirtualAddress() {
-  ShadowBounds = __sanitizer_shadow_bounds();
+  InitShadowBounds();
   return ShadowBounds.memory_limit - 1;
 }
 
Index: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
===
--- compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
+++ compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
@@ -30,6 +30,19 @@
 
 namespace __hwasan {
 
+bool InitShadow() {
+  __sanitizer::InitShadowBounds();
+  CHECK_NE(__sanitizer::ShadowBounds.shadow_limit, 0);
+
+  return true;
+}
+
+bool MemIsApp(uptr p) {
+  CHECK(GetTagFromPointer(p) == 0);
+  return __sanitizer::ShadowBounds.shadow_limit <= p &&
+ p <= (__sanitizer::ShadowBounds.memory_limit - 1);
+}
+
 // These are known parameters passed to the hwasan runtime on thread creation.
 struct Thread::InitState {
   uptr stack_bottom, stack_top;
@@ -130,6 +143,26 @@
   hwasanThreadList().ReleaseThread(thread);
 }
 
+// Not implemented because Fuchsia does not use signal handlers.
+void HwasanOnDeadlySignal(int signo, void *info, void *context) {}
+
+// Not implemented because Fuchsia does not use interceptors.
+void InitializeInterceptors() {}
+
+// Not implemented because this is only relevant for Android.
+void AndroidTestTlsSlot() {}
+
+// TSD was normally used on linux as a means of calling the hwasan thread exit
+// handler passed to pthread_key_create. This is not needed on Fuchsia because
+// we will be using __sanitizer_thread_exit_hook.
+void HwasanTSDInit() {}
+void HwasanTSDThreadInit() {}
+
+// On linux, this just would call `atexit(HwasanAtExit)`. The functions in
+// HwasanAtExit are unimplemented for Fuchsia and effectively no-ops, so this
+// function is unneeded.
+void InstallAtExitHandler() {}
+
 }  // namespace __hwasan
 
 extern "C" {


Index: compiler-rt/lib/sanitizer_common/sanitizer_fuchsia.cpp
===
--- compiler-rt/lib/sanitizer_common/sanitizer_fuchsia.cpp
+++ compiler-rt/lib/sanitizer_common/sanitizer_fuchsia.cpp
@@ -156,8 +156,10 @@
 
 sanitizer_shadow_bounds_t ShadowBounds;
 
+void InitShadowBounds() { ShadowBounds = __sanitizer_shadow_bounds(); }
+
 uptr GetMaxUserVirtualAddress() {
-  ShadowBounds = __sanitizer_shadow_bounds();
+  InitShadowBounds();
   return ShadowBounds.memory_limit - 1;
 }
 
Index: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
===
--- compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
+++ compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
@@ -30,6 +30,19 @@
 
 namespace __hwasan {
 
+bool InitShadow() {
+  __sanitizer::InitShadowBounds();
+  CHECK_NE(__sanitizer::ShadowBounds.shadow_limit, 0);
+
+  return true;
+}
+
+bool MemIsApp(uptr p) {
+  CHECK(GetTagFromPointer(p) == 0);
+  return __sanitizer::ShadowBounds.shadow_limit <= p &&
+ p <= (__sanitizer::ShadowBounds.memory_limit - 1);
+}
+
 // These are known parameters passed to the hwasan runtime on thread creation.
 struct Thread::InitState {
   uptr stack_bottom, stack_top;
@@ -130,6 +143,26 @@
   hwasanThreadList().ReleaseThread(thread);
 }
 
+// Not implemented because Fuchsia does not use signal handlers.
+void HwasanOnDeadlySignal(int signo, void *info, void *context) {}
+
+// Not implemented because Fuchsia does not use interceptors.
+void InitializeInterceptors() {}
+
+// Not implemented because this is only relevant for Android.
+void AndroidTestTlsSlot() {}
+
+// TSD was normally used on linux as a means of calling the hwasan thread exit
+// handler passed to pthread_key_create. This is not needed on Fuchsia because
+// we will be using __sanitizer_thread_exit_hook.
+void HwasanTSDInit() {}
+void HwasanTSDThreadInit() {}
+
+// On linux, this just would call `atexit(HwasanAtExit)`. The functions in
+// HwasanAtExit are unimplemented for Fuchsia and effectively no-ops, so this
+// function is unneeded.
+void InstallAtExitHandler() {}
+
 }  // namespace __hwasan
 
 

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

[Roland McGrath via Phabricator via cfe-commits]

mcgrathr accepted this revision.
mcgrathr added a comment.
This revision is now accepted and ready to land.

lgtm with minor changes + clang-format.




Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:29
+  // now. Otherwise, ShadowBounds will be a zero-initialized global.
+  ShadowBounds = __sanitizer_shadow_bounds();
+  CHECK_NE(__sanitizer::ShadowBounds.shadow_limit, 0);

It feels sketchy to modify a variable defined in sanitizer_common directly like 
that.
Let's move this call into an `InitShadowBounds()` in 
sanitizer_common/sanitizer_fuchsia.h factored out of GetMaxUserVirtualAddress.




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

leonardchan wrote:
> 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.
On further reflection, it's suboptimal that GetMaxUserVirtualAddress always 
resets the global.  It does that because in the asan implementation it's only 
called once at startup anyway so that was the minimal refactoring there.

Here since we're using ShadowBounds directly in this same code, I think using 
it directly here is more readable (as well as more optimal).



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

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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

*ping* Anything else that should be updated?


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

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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan updated this revision to Diff 352194.
leonardchan added a comment.

Rebase and remove `__hwasan_shadow_memory_dynamic_address`. We don't need to 
set it anymore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103936

Files:
  compiler-rt/lib/hwasan/CMakeLists.txt
  compiler-rt/lib/hwasan/hwasan_fuchsia.cpp


Index: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
===
--- /dev/null
+++ compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
@@ -0,0 +1,63 @@
+//===-- hwasan_fuchsia.cpp --*- C++ 
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+///
+/// \file
+/// This file is a part of HWAddressSanitizer and contains Fuchsia-specific
+/// code.
+///
+//===--===//
+
+#include "sanitizer_common/sanitizer_fuchsia.h"
+#if SANITIZER_FUCHSIA
+
+#include "hwasan.h"
+#include "hwasan_interface_internal.h"
+#include "hwasan_report.h"
+#include "hwasan_thread.h"
+#include "hwasan_thread_list.h"
+
+namespace __hwasan {
+
+bool InitShadow() {
+  // Call this explicitly to set the ShadowBounds global so we can reference it
+  // now. Otherwise, ShadowBounds will be a zero-initialized global.
+  ShadowBounds = __sanitizer_shadow_bounds();
+  CHECK_NE(__sanitizer::ShadowBounds.shadow_limit, 0);
+
+  return true;
+}
+
+bool MemIsApp(uptr p) {
+  CHECK(GetTagFromPointer(p) == 0);
+  return __sanitizer::ShadowBounds.shadow_limit <= p &&
+ p <= __sanitizer::GetMaxUserVirtualAddress();
+}
+
+// Not implemented because Fuchsia does not use signal handlers.
+void HwasanOnDeadlySignal(int signo, void *info, void *context) {}
+
+// Not implemented because Fuchsia does not use interceptors.
+void InitializeInterceptors() {}
+
+// Not implemented because this is only relevant for Android.
+void AndroidTestTlsSlot() {}
+
+// TSD was normally used on linux as a means of calling the hwasan thread exit
+// handler passed to pthread_key_create. This is not needed on Fuchsia because
+// we will be using __sanitizer_thread_exit_hook.
+void HwasanTSDInit() {}
+void HwasanTSDThreadInit() {}
+
+// On linux, this just would call `atexit(HwasanAtExit)`. The functions in
+// HwasanAtExit are unimplemented for Fuchsia and effectively no-ops, so this
+// function is unneeded.
+void InstallAtExitHandler() {}
+
+}  // namespace __hwasan
+
+#endif  // SANITIZER_FUCHSIA
Index: compiler-rt/lib/hwasan/CMakeLists.txt
===
--- compiler-rt/lib/hwasan/CMakeLists.txt
+++ compiler-rt/lib/hwasan/CMakeLists.txt
@@ -7,6 +7,7 @@
   hwasan_allocation_functions.cpp
   hwasan_dynamic_shadow.cpp
   hwasan_exceptions.cpp
+  hwasan_fuchsia.cpp
   hwasan_globals.cpp
   hwasan_interceptors.cpp
   hwasan_interceptors_vfork.S


Index: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
===
--- /dev/null
+++ compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
@@ -0,0 +1,63 @@
+//===-- hwasan_fuchsia.cpp --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+///
+/// \file
+/// This file is a part of HWAddressSanitizer and contains Fuchsia-specific
+/// code.
+///
+//===--===//
+
+#include "sanitizer_common/sanitizer_fuchsia.h"
+#if SANITIZER_FUCHSIA
+
+#include "hwasan.h"
+#include "hwasan_interface_internal.h"
+#include "hwasan_report.h"
+#include "hwasan_thread.h"
+#include "hwasan_thread_list.h"
+
+namespace __hwasan {
+
+bool InitShadow() {
+  // Call this explicitly to set the ShadowBounds global so we can reference it
+  // now. Otherwise, ShadowBounds will be a zero-initialized global.
+  ShadowBounds = __sanitizer_shadow_bounds();
+  CHECK_NE(__sanitizer::ShadowBounds.shadow_limit, 0);
+
+  return true;
+}
+
+bool MemIsApp(uptr p) {
+  CHECK(GetTagFromPointer(p) == 0);
+  return __sanitizer::ShadowBounds.shadow_limit <= p &&
+ p <= __sanitizer::GetMaxUserVirtualAddress();
+}
+
+// Not implemented because Fuchsia does not use signal handlers.
+void HwasanOnDeadlySignal(int signo, void *info, void *context) {}
+
+// Not implemented because Fuchsia does not use interceptors.
+void InitializeInterceptors() {}
+
+// Not implemented because this is only relevant for Android.
+void AndroidTestTlsSlot() {}

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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan updated this revision to Diff 351931.
leonardchan marked 10 inline comments as done.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103936

Files:
  compiler-rt/lib/hwasan/CMakeLists.txt
  compiler-rt/lib/hwasan/hwasan_fuchsia.cpp


Index: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
===
--- /dev/null
+++ compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
@@ -0,0 +1,68 @@
+//===-- hwasan_fuchsia.cpp --*- C++ 
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+///
+/// \file
+/// This file is a part of HWAddressSanitizer and contains Fuchsia-specific
+/// code.
+///
+//===--===//
+
+#include "sanitizer_common/sanitizer_fuchsia.h"
+#if SANITIZER_FUCHSIA
+
+#include "hwasan.h"
+#include "hwasan_interface_internal.h"
+#include "hwasan_report.h"
+#include "hwasan_thread.h"
+#include "hwasan_thread_list.h"
+
+namespace __hwasan {
+
+bool InitShadow() {
+  // TODO(leonardchan): Remove the runtime variable altogether once we 
implement
+  // something similar to SHADOW_OFFSET for hwasan. Ideally, the runtime would
+  // just know this value is zero everywhere it's used.
+  __hwasan_shadow_memory_dynamic_address = 0;
+
+  // Call this explicitly to set the ShadowBounds global so we can reference it
+  // now. Otherwise, ShadowBounds will be a zero-initialized global.
+  ShadowBounds = __sanitizer_shadow_bounds();
+  CHECK_NE(__sanitizer::ShadowBounds.shadow_limit, 0);
+
+  return true;
+}
+
+bool MemIsApp(uptr p) {
+  CHECK(GetTagFromPointer(p) == 0);
+  return __sanitizer::ShadowBounds.shadow_limit <= p &&
+ p <= __sanitizer::GetMaxUserVirtualAddress();
+}
+
+// Not implemented because Fuchsia does not use signal handlers.
+void HwasanOnDeadlySignal(int signo, void *info, void *context) {}
+
+// Not implemented because Fuchsia does not use interceptors.
+void InitializeInterceptors() {}
+
+// Not implemented because this is only relevant for Android.
+void AndroidTestTlsSlot() {}
+
+// TSD was normally used on linux as a means of calling the hwasan thread exit
+// handler passed to pthread_key_create. This is not needed on Fuchsia because
+// we will be using __sanitizer_thread_exit_hook.
+void HwasanTSDInit() {}
+void HwasanTSDThreadInit() {}
+
+// On linux, this just would call `atexit(HwasanAtExit)`. The functions in
+// HwasanAtExit are unimplemented for Fuchsia and effectively no-ops, so this
+// function is unneeded.
+void InstallAtExitHandler() {}
+
+}  // namespace __hwasan
+
+#endif  // SANITIZER_FUCHSIA
Index: compiler-rt/lib/hwasan/CMakeLists.txt
===
--- compiler-rt/lib/hwasan/CMakeLists.txt
+++ compiler-rt/lib/hwasan/CMakeLists.txt
@@ -7,6 +7,7 @@
   hwasan_allocation_functions.cpp
   hwasan_dynamic_shadow.cpp
   hwasan_exceptions.cpp
+  hwasan_fuchsia.cpp
   hwasan_globals.cpp
   hwasan_interceptors.cpp
   hwasan_interceptors_vfork.S


Index: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
===
--- /dev/null
+++ compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
@@ -0,0 +1,68 @@
+//===-- hwasan_fuchsia.cpp --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+///
+/// \file
+/// This file is a part of HWAddressSanitizer and contains Fuchsia-specific
+/// code.
+///
+//===--===//
+
+#include "sanitizer_common/sanitizer_fuchsia.h"
+#if SANITIZER_FUCHSIA
+
+#include "hwasan.h"
+#include "hwasan_interface_internal.h"
+#include "hwasan_report.h"
+#include "hwasan_thread.h"
+#include "hwasan_thread_list.h"
+
+namespace __hwasan {
+
+bool InitShadow() {
+  // TODO(leonardchan): Remove the runtime variable altogether once we implement
+  // something similar to SHADOW_OFFSET for hwasan. Ideally, the runtime would
+  // just know this value is zero everywhere it's used.
+  __hwasan_shadow_memory_dynamic_address = 0;
+
+  // Call this explicitly to set the ShadowBounds global so we can reference it
+  // now. Otherwise, ShadowBounds will be a zero-initialized global.
+  ShadowBounds = __sanitizer_shadow_bounds();
+  CHECK_NE(__sanitizer::ShadowBounds.shadow_limit, 0);
+
+  return true;
+}
+
+bool MemIsApp(uptr p) {
+  

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

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

[Leonard Chan via Phabricator via cfe-commits]

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

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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan updated this revision to Diff 351510.
leonardchan marked an inline comment as done.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103936

Files:
  compiler-rt/lib/hwasan/CMakeLists.txt
  compiler-rt/lib/hwasan/hwasan_fuchsia.cpp


Index: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
===
--- /dev/null
+++ compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
@@ -0,0 +1,69 @@
+//===-- hwasan_fuchsia.cpp --*- C++ 
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+///
+/// \file
+/// This file is a part of HWAddressSanitizer and contains Fuchsia-specific
+/// code.
+///
+//===--===//
+
+#include "sanitizer_common/sanitizer_fuchsia.h"
+#if SANITIZER_FUCHSIA
+
+#include "hwasan.h"
+#include "hwasan_interface_internal.h"
+#include "hwasan_report.h"
+#include "hwasan_thread.h"
+#include "hwasan_thread_list.h"
+
+namespace __hwasan {
+
+// This is used by the hwasan allocator when page aliasing on x86 is enabled,
+// but should always remain zero on non-x86 implementations.
+uptr kAliasRegionStart = 0;
+
+bool InitShadow() {
+  __hwasan_shadow_memory_dynamic_address = 0;
+
+  // Call this explicitly to set the ShadowBounds global so we can reference it
+  // now. Otherwise, ShadowBounds will be a zero-initialized global.
+  ShadowBounds = __sanitizer_shadow_bounds();
+  CHECK_NE(__sanitizer::ShadowBounds.shadow_limit, 0);
+
+  return true;
+}
+
+bool MemIsApp(uptr p) {
+  CHECK(GetTagFromPointer(p) == 0);
+  return __sanitizer::ShadowBounds.shadow_limit <= p &&
+ p <= __sanitizer::GetMaxUserVirtualAddress();
+}
+
+// Not implemented because Fuchsia does not use signal handlers.
+void HwasanOnDeadlySignal(int signo, void *info, void *context) {}
+
+// Not implemented because Fuchsia does not use interceptors.
+void InitializeInterceptors() {}
+
+// Not implemented because this is only relevant for Android.
+void AndroidTestTlsSlot() {}
+
+// TSD was normally used on linux as a means of calling the hwasan thread exit
+// handler passed to pthread_key_create. This is not needed on Fuchsia because
+// we will be using __sanitizer_thread_exit_hook.
+void HwasanTSDInit() {}
+void HwasanTSDThreadInit() {}
+
+// On linux, this just would call `atexit(HwasanAtExit)`. The functions in
+// HwasanAtExit are unimplemented for Fuchsia and effectively no-ops, so this
+// function is unneeded.
+void InstallAtExitHandler() {}
+
+}  // namespace __hwasan
+
+#endif  // SANITIZER_FUCHSIA
Index: compiler-rt/lib/hwasan/CMakeLists.txt
===
--- compiler-rt/lib/hwasan/CMakeLists.txt
+++ compiler-rt/lib/hwasan/CMakeLists.txt
@@ -7,6 +7,7 @@
   hwasan_allocation_functions.cpp
   hwasan_dynamic_shadow.cpp
   hwasan_exceptions.cpp
+  hwasan_fuchsia.cpp
   hwasan_globals.cpp
   hwasan_interceptors.cpp
   hwasan_interceptors_vfork.S


Index: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
===
--- /dev/null
+++ compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
@@ -0,0 +1,69 @@
+//===-- hwasan_fuchsia.cpp --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+///
+/// \file
+/// This file is a part of HWAddressSanitizer and contains Fuchsia-specific
+/// code.
+///
+//===--===//
+
+#include "sanitizer_common/sanitizer_fuchsia.h"
+#if SANITIZER_FUCHSIA
+
+#include "hwasan.h"
+#include "hwasan_interface_internal.h"
+#include "hwasan_report.h"
+#include "hwasan_thread.h"
+#include "hwasan_thread_list.h"
+
+namespace __hwasan {
+
+// This is used by the hwasan allocator when page aliasing on x86 is enabled,
+// but should always remain zero on non-x86 implementations.
+uptr kAliasRegionStart = 0;
+
+bool InitShadow() {
+  __hwasan_shadow_memory_dynamic_address = 0;
+
+  // Call this explicitly to set the ShadowBounds global so we can reference it
+  // now. Otherwise, ShadowBounds will be a zero-initialized global.
+  ShadowBounds = __sanitizer_shadow_bounds();
+  CHECK_NE(__sanitizer::ShadowBounds.shadow_limit, 0);
+
+  return true;
+}
+
+bool MemIsApp(uptr p) {
+  CHECK(GetTagFromPointer(p) == 0);
+  return __sanitizer::ShadowBounds.shadow_limit <= p &&
+ p <= 

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

Let me know if these functions should be split up or can land together.


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

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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan created this revision.
leonardchan added reviewers: phosek, mcgrathr, charco.
leonardchan added a project: Sanitizers.
Herald added subscribers: jfb, mgorny, dberris.
leonardchan requested review of this revision.
Herald added a subscriber: Sanitizers.

This contains all the definitions required by hwasan for the fuchsia 
implementation and can be landed independently from the remaining parts of 
D91466 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D103936

Files:
  compiler-rt/lib/hwasan/CMakeLists.txt
  compiler-rt/lib/hwasan/hwasan_fuchsia.cpp

Index: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
===
--- /dev/null
+++ compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
@@ -0,0 +1,108 @@
+//===-- hwasan_fuchsia.cpp --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+///
+/// \file
+/// This file is a part of HWAddressSanitizer and contains Fuchsia-specific
+/// code.
+///
+//===--===//
+
+#include "sanitizer_common/sanitizer_fuchsia.h"
+#if SANITIZER_FUCHSIA
+
+#include "hwasan.h"
+#include "hwasan_interface_internal.h"
+#include "hwasan_report.h"
+#include "hwasan_thread.h"
+#include "hwasan_thread_list.h"
+
+SANITIZER_INTERFACE_ATTRIBUTE
+THREADLOCAL uptr __hwasan_tls;
+
+namespace __hwasan {
+
+uptr kHighMemEnd;
+uptr kHighMemBeg;
+uptr kAliasRegionStart = 0;  // Always 0 on non-x86.
+
+bool InitShadow() {
+  __hwasan_shadow_memory_dynamic_address = 0;
+
+  // This initializes __sanitizer::ShadowBounds.
+  kHighMemEnd = GetMaxUserVirtualAddress();
+  kHighMemBeg = __sanitizer::ShadowBounds.shadow_limit;
+
+  CHECK_EQ(kHighMemEnd, __sanitizer::ShadowBounds.memory_limit - 1);
+  CHECK_EQ(kHighMemBeg, __sanitizer::ShadowBounds.shadow_limit);
+  CHECK_NE(kHighMemBeg, 0);
+
+  return true;
+}
+
+void InitThreads() {
+  uptr alloc_size = UINT64_C(1) << kShadowBaseAlignment;
+  uptr thread_start = reinterpret_cast(
+  MmapAlignedOrDieOnFatalError(alloc_size, alloc_size, __func__));
+  InitThreadList(thread_start, alloc_size);
+}
+
+bool MemIsApp(uptr p) {
+  CHECK(GetTagFromPointer(p) == 0);
+  return kHighMemBeg <= p && p <= kHighMemEnd;
+}
+
+// -- TSD  {{{
+
+extern "C" {
+void __sanitizer_thread_start_hook(void *hook, thrd_t self) {
+  hwasanThreadList().CreateCurrentThread()->InitRandomState();
+}
+
+void __sanitizer_thread_exit_hook(void *hook, thrd_t self) {
+  Thread *t = GetCurrentThread();
+  // Make sure that signal handler can not see a stale current thread pointer.
+  atomic_signal_fence(memory_order_seq_cst);
+  if (t)
+hwasanThreadList().ReleaseThread(t);
+}
+
+void __sanitizer_exit() { __hwasan::HwasanAtExit(); }
+}  // extern "C"
+
+uptr *GetCurrentThreadLongPtr() { return &__hwasan_tls; }
+
+Thread *GetCurrentThread() {
+  uptr *ThreadLongPtr = GetCurrentThreadLongPtr();
+  if (UNLIKELY(*ThreadLongPtr == 0))
+return nullptr;
+  auto *R = (StackAllocationsRingBuffer *)ThreadLongPtr;
+  return hwasanThreadList().GetThreadByBufferAddress((uptr)R->Next());
+}
+
+// Entry point stub for interoperability between __hwasan_tag_mismatch (ASM) and
+// 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) {
+  __hwasan::HwasanTagMismatch(addr, access_info, registers_frame, outsize);
+}
+
+// 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) {}
+void InitializeInterceptors() {}
+void AndroidTestTlsSlot() {}
+void HwasanTSDInit() {}
+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.
+void InitPrctl() {}
+void InstallAtExitHandler() {}
+
+}  // namespace __hwasan
+
+#endif  // SANITIZER_FUCHSIA
Index: compiler-rt/lib/hwasan/CMakeLists.txt
===
--- compiler-rt/lib/hwasan/CMakeLists.txt
+++ compiler-rt/lib/hwasan/CMakeLists.txt
@@ -7,6 +7,7 @@
   hwasan_allocation_functions.cpp
   hwasan_dynamic_shadow.cpp
   hwasan_exceptions.cpp
+  hwasan_fuchsia.cpp
   hwasan_globals.cpp
   hwasan_interceptors.cpp
   hwasan_interceptors_vfork.S
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits