Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[sanitizer_common] Use 38-bit mmap range for Fuchsia #69387

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

PiJoules
Copy link
Contributor

46cb8d9 unconditionally changed the mmap range to 2^48 for all riscv sanitizers. This changes the allocator tunings for the 32-bit allocator for riscv and led to a severe performance regression for our lsan tests. This effectively revers the tuning change but only for Fuchsia.

Once we enable the 64-bit allocator for everything riscv, this value will be irrelevant since it's only relevant for the 32-bit allocator.

@llvmbot
Copy link
Member

llvmbot commented Oct 17, 2023

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: None (PiJoules)

Changes

46cb8d9 unconditionally changed the mmap range to 2^48 for all riscv sanitizers. This changes the allocator tunings for the 32-bit allocator for riscv and led to a severe performance regression for our lsan tests. This effectively revers the tuning change but only for Fuchsia.

Once we enable the 64-bit allocator for everything riscv, this value will be irrelevant since it's only relevant for the 32-bit allocator.


Full diff: https://github.com/llvm/llvm-project/pull/69387.diff

1 Files Affected:

  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_platform.h (+9-1)
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_platform.h b/compiler-rt/lib/sanitizer_common/sanitizer_platform.h
index 5280416f8bd3029..4ee05034bed8f29 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_platform.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_platform.h
@@ -303,7 +303,15 @@
 #    define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 40)
 #  endif
 #elif SANITIZER_RISCV64
-#  define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 47)
+// FIXME: Rather than hardcoding the VMA here, we should rely on
+// GetMaxUserVirtualAddress(). This will require some refactoring though since
+// many places either hardcode some value or SANITIZER_MMAP_RANGE_SIZE is
+// assumed to be some constant integer.
+#  if SANITIZER_FUCHSIA
+#    define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 38)
+#  else
+#    define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 47)
+#  endif
 #elif defined(__aarch64__)
 #  if SANITIZER_APPLE
 #    if SANITIZER_OSX || SANITIZER_IOSSIM

46cb8d9 unconditionally changed the
mmap range to 2^48 for all riscv sanitizers. This changes the allocator
tunings for the 32-bit allocator for riscv and led to a severe
performance regression for our lsan tests. This effectively revers the
tuning change but only for Fuchsia.

Once we enable the 64-bit allocator for everything riscv, this value
will be irrelevant since it's only relevant for the 32-bit allocator.
@PiJoules PiJoules force-pushed the continue-to-use-38-vma-for-fuchsia branch from bf11134 to 86cee59 Compare October 17, 2023 21:24
Copy link
Contributor

@frobtech frobtech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@PiJoules PiJoules merged commit bd0fc48 into llvm:main Oct 18, 2023
2 checks passed
@PiJoules PiJoules deleted the continue-to-use-38-vma-for-fuchsia branch October 18, 2023 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants