Skip to content

[scudo] Small cleanup of memory tagging code part 2. #168807

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Nov 21, 2025
Merged

[scudo] Small cleanup of memory tagging code part 2. #168807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0e987ea
[scudo] Small cleanup of memory tagging code part 2.
cferris1000 Nov 20, 2025
2e23682
Clang format.
cferris1000 Nov 20, 2025
ce54836
consexprs added
cferris1000 Nov 20, 2025
34ffed3
Go back to no constexpr.
cferris1000 Nov 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions compiler-rt/lib/scudo/standalone/combined.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,7 @@ class Allocator {
Primary.Options.set(OptionBit::DeallocTypeMismatch);
if (getFlags()->delete_size_mismatch)
Primary.Options.set(OptionBit::DeleteSizeMismatch);
if (allocatorSupportsMemoryTagging<AllocatorConfig>() &&
systemSupportsMemoryTagging())
if (systemSupportsMemoryTagging())
Primary.Options.set(OptionBit::UseMemoryTagging);

QuarantineMaxChunkSize =
Expand Down Expand Up @@ -689,16 +688,15 @@ class Allocator {
Base = untagPointer(Base);
const uptr From = Base;
const uptr To = Base + Size;
bool MayHaveTaggedPrimary =
allocatorSupportsMemoryTagging<AllocatorConfig>() &&
systemSupportsMemoryTagging();
const Options Options = Primary.Options.load();
bool MayHaveTaggedPrimary = useMemoryTagging<AllocatorConfig>(Options);
Copy link
Collaborator

@vitalybuka vitalybuka Nov 20, 2025

Choose a reason for hiding this comment

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

could useMemoryTagging be a constexpr ?

Copy link
Contributor Author

@cferris1000 cferris1000 Nov 20, 2025

Choose a reason for hiding this comment

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

It can and it does now.

Copy link
Contributor

@pcc pcc Nov 20, 2025

Choose a reason for hiding this comment

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

It seems a bit confusing for this to be constexpr because it reads a runtime value (options bit).

Copy link
Contributor Author

@cferris1000 cferris1000 Nov 20, 2025

Choose a reason for hiding this comment

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

That's true. I don't really have a very strong opinion on this, so I'm happy to remove it, or leave it.

It sounds like like it could be confusing, so I'll remove it unless there is a strong reason to re-add it.

auto Lambda = [this, From, To, MayHaveTaggedPrimary, Callback,
Arg](uptr Block) {
if (Block < From || Block >= To)
return;
uptr Chunk;
Chunk::UnpackedHeader Header;
if (MayHaveTaggedPrimary) {
if (UNLIKELY(MayHaveTaggedPrimary)) {
// A chunk header can either have a zero tag (tagged primary) or the
// header tag (secondary, or untagged primary). We don't know which so
// try both.
Expand Down
6 changes: 2 additions & 4 deletions compiler-rt/lib/scudo/standalone/memtag.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ inline void enableSystemMemoryTaggingTestOnly() {

#else // !SCUDO_CAN_USE_MTE

inline bool systemSupportsMemoryTagging() { return false; }
inline constexpr bool systemSupportsMemoryTagging() { return false; }

inline NORETURN bool systemDetectsMemoryTagFaultsTestOnly() {
UNREACHABLE("memory tagging not supported");
Expand Down Expand Up @@ -261,9 +261,7 @@ inline uptr loadTag(uptr Ptr) {

#else

inline NORETURN bool systemSupportsMemoryTagging() {
UNREACHABLE("memory tagging not supported");
}
inline constexpr bool systemSupportsMemoryTagging() { return false; }

inline NORETURN bool systemDetectsMemoryTagFaultsTestOnly() {
UNREACHABLE("memory tagging not supported");
Expand Down
1 change: 0 additions & 1 deletion compiler-rt/lib/scudo/standalone/tests/memtag_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ TEST(MemtagBasicDeathTest, Unsupported) {
EXPECT_DEATH(untagPointer((uptr)0), "not supported");
EXPECT_DEATH(extractTag((uptr)0), "not supported");

EXPECT_DEATH(systemSupportsMemoryTagging(), "not supported");
EXPECT_DEATH(systemDetectsMemoryTagFaultsTestOnly(), "not supported");
EXPECT_DEATH(enableSystemMemoryTaggingTestOnly(), "not supported");

Expand Down
4 changes: 1 addition & 3 deletions compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,7 @@
const scudo::uptr PageSize = scudo::getPageSizeCached();

template <typename Config> static scudo::Options getOptionsForConfig() {
if (!Config::getMaySupportMemoryTagging() ||
!scudo::archSupportsMemoryTagging() ||
!scudo::systemSupportsMemoryTagging())
if (!scudo::systemSupportsMemoryTagging())
return {};
scudo::AtomicOptions AO;
AO.set(scudo::OptionBit::UseMemoryTagging);
Expand Down