This is an archive of the discontinued LLVM Phabricator instance.

[scudo] Implement and enable MemMapLinux
ClosedPublic

Authored by Chia-hungDuan on Mar 20 2023, 1:39 PM.

Details

Summary

Most of the implementations are copied from linux.cpp and we will be
keeping those memory functions in linux.cpp for a while until we are
able to switch to use MemMap completely.

The remaining part is SizeClassAllocator32 which hasn't been switched to
use MemMap interface

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Mar 20 2023, 1:39 PM
Chia-hungDuan requested review of this revision.Mar 20 2023, 1:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 20 2023, 1:39 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Slightly improve the semantic of MemMapLinux::remap()

cferris requested changes to this revision.Apr 3 2023, 3:37 PM
cferris added inline comments.
compiler-rt/lib/scudo/standalone/mem_map_linux.cpp
39–40

Is this still necessary? I'm not sure if you need to make this compile on old versions of Android, but PR_SET_VMA and PR_SET_VMA_ANON_NAME are available from the bionic kernel headers.

If you need to make this work on older versions of Android, maybe this should be changed to see if PR_SET_VMA is defined.

141

What happens if reserving fails? Should this function return a bool so that caller can fail gracefully?

This revision now requires changes to proceed.Apr 3 2023, 3:37 PM
Chia-hungDuan marked an inline comment as done.

Address review comment

Update commit message

Chia-hungDuan retitled this revision from [scudo] Implement MemMapLinux to [scudo] Implement and enable MemMapLinux.Aug 3 2023, 3:55 PM
Chia-hungDuan edited the summary of this revision. (Show Details)
cferris requested changes to this revision.Aug 3 2023, 7:02 PM

A few questions.

compiler-rt/lib/scudo/standalone/mem_map_linux.cpp
95

This is a bit confusing. Why is the MapBase being set to Addr + Size? During the unmap, it doesn't seem like this shoudl happen.

96

Another slightly confusing part, it seems like unmapping would increase the total capacity, not decrease it.

This revision now requires changes to proceed.Aug 3 2023, 7:02 PM
Chia-hungDuan edited the summary of this revision. (Show Details)

Add more comments

compiler-rt/lib/scudo/standalone/mem_map_linux.cpp
39–40

This is copied from the old file linux.cpp, I added a TODO to mark it and we can discuss the necessity later

95

The unamp() supports partial unmapping but the result pages still have to be contiguous. So we check if the unmap() is either aligned to the begin addr or end addr in MemMapBase::unamp().

So far, this is not used so it's more like a contract for future. Like blocks merging in secondary.

96

Added few comments here. Let me know if it explains more.

141

Done. The API has been changed to return boolean.

cferris accepted this revision.Aug 4 2023, 4:37 PM

LGTM the new comments answered my questions.

This revision is now accepted and ready to land.Aug 4 2023, 4:37 PM
This revision was automatically updated to reflect the committed changes.