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

[msan] Increase kNumStackOriginDescrs constant #92838

Merged
merged 2 commits into from
May 21, 2024

Conversation

thurstond
Copy link
Contributor

@thurstond thurstond commented May 20, 2024

This increases the constant size of kNumStackOriginDescrs to 4M (64GB of BSS across two arrays), which ought to be enough for anybody.

This is the easier alternative suggested by eugenis@ in #92826.

This increases the constant size of kNumStackOriginDescrs to
64M (1GB of BSS across two arrays), which ought to be enough for
anybody.

This is the easier alternative suggested by eugenis@ in
llvm#92826.
@llvmbot
Copy link
Collaborator

llvmbot commented May 21, 2024

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

Author: Thurston Dang (thurstond)

Changes

This increases the constant size of kNumStackOriginDescrs to 64M (1GB of BSS across two arrays), which ought to be enough for anybody.

This is the easier alternative suggested by eugenis@ in #92826.


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

1 Files Affected:

  • (modified) compiler-rt/lib/msan/msan.cpp (+7-1)
diff --git a/compiler-rt/lib/msan/msan.cpp b/compiler-rt/lib/msan/msan.cpp
index a2fc27de1901b..8bef6812f21c2 100644
--- a/compiler-rt/lib/msan/msan.cpp
+++ b/compiler-rt/lib/msan/msan.cpp
@@ -100,7 +100,13 @@ int msan_report_count = 0;
 
 // Array of stack origins.
 // FIXME: make it resizable.
-static const uptr kNumStackOriginDescrs = 1024 * 1024;
+// Although BSS memory doesn't cost anything until used, it is limited to 2GB
+// in some configurations (e.g., relocation R_X86_64_PC32 out of range:
+// 8600110908 is not in [-2147483648, 2147483647]; references section '.bss')
+// hence kNumStackOriginDescrs is limited to roughly 2GB / sizeof(uptr) / 2
+// == 128M per array (StackOriginDescr, StackOriginPC). We set it at 64M each
+// in case other parts of MSan want more BSS space in the future.
+static const uptr kNumStackOriginDescrs = 64 * 1024 * 1024;
 static const char *StackOriginDescr[kNumStackOriginDescrs];
 static uptr StackOriginPC[kNumStackOriginDescrs];
 static atomic_uint32_t NumStackOriginDescrs;

Copy link
Contributor

@eugenis eugenis left a comment

Choose a reason for hiding this comment

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

Hmm I did not consider the relocation offset. This can realistically push some other .bss variable out of the 2Gb range if the linker is not smart enough to place the largest object last. The 2Gb is not just MSan rtl budget, but for the whole program.

Lets bring this down to 4M origins, that should be enough for the foreseeable future.

@thurstond
Copy link
Contributor Author

Hmm I did not consider the relocation offset. This can realistically push some other .bss variable out of the 2Gb range if the linker is not smart enough to place the largest object last. The 2Gb is not just MSan rtl budget, but for the whole program.

Lets bring this down to 4M origins, that should be enough for the foreseeable future.

Done!

@thurstond thurstond requested a review from eugenis May 21, 2024 18:26
Copy link
Contributor

@eugenis eugenis left a comment

Choose a reason for hiding this comment

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

Thank you.

@thurstond thurstond merged commit 57a5079 into llvm:main May 21, 2024
4 checks passed
thurstond added a commit that referenced this pull request May 21, 2024
thurstond added a commit that referenced this pull request May 28, 2024
…erPC) (#93117)

The original pull request
(#92838) was reverted due to a
PowerPC buildbot breakage
(df626dd).
This reland limits the scope of the change to non-PowerPC platforms. I
am unaware of any PowerPC use cases that would benefit from a larger
kNumStackOriginDescrs constant.

Original CL description: This increases the constant size of
kNumStackOriginDescrs to 4M (64GB of BSS across two arrays), which ought
to be enough for anybody.

This is the easier alternative suggested by eugenis@ in
#92826.
vg0204 pushed a commit to vg0204/llvm-project that referenced this pull request May 29, 2024
…erPC) (llvm#93117)

The original pull request
(llvm#92838) was reverted due to a
PowerPC buildbot breakage
(llvm@df626dd).
This reland limits the scope of the change to non-PowerPC platforms. I
am unaware of any PowerPC use cases that would benefit from a larger
kNumStackOriginDescrs constant.

Original CL description: This increases the constant size of
kNumStackOriginDescrs to 4M (64GB of BSS across two arrays), which ought
to be enough for anybody.

This is the easier alternative suggested by eugenis@ in
llvm#92826.
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.

4 participants