This is an archive of the discontinued LLVM Phabricator instance.

Release pages to OS when setting 0 label
ClosedPublic

Authored by stephan.yichao.zhao on Oct 10 2020, 6:36 PM.

Details

Summary

This is a follow up patch of https://reviews.llvm.org/D88755.

When set 0 label for an address range, we can release pages within the
corresponding shadow address range to OS, and set only addresses outside
the pages to be 0.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2020, 6:36 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
stephan.yichao.zhao requested review of this revision.Oct 10 2020, 6:36 PM
morehouse added inline comments.Oct 12 2020, 10:57 AM
compiler-rt/lib/dfsan/dfsan.cpp
277–280

Nit: Let's rename this something more descriptive. e.g., WriteShadowIfDifferent.

278

Every call site seems to cast a uptr to dfsan_label *. Let's move the cast into this function and rename the parameter something more descriptive (e.g., shadow_addr).

307

Both MSan and ASan do the release only when size is larger than 16 pages. Should we have similar logic here?

Maybe it's less important to avoid the system call in this case for DFSan... ASan/MSan can use memset to clear a small number of pages, which can use vector instructions for efficiency. But DFSan has to use a loop since shadow labels take up 2 bytes. So maybe the threshold where madvise becomes faster is lower for DFSan.

@eugenis WDYT?

eugenis added inline comments.Oct 12 2020, 11:10 AM
compiler-rt/lib/dfsan/dfsan.cpp
307

If you have a specific application in mind, I suggest directly measuring run time and process RSS. Otherwise, the 16 page threshold makes sense to me, AFAIR it had the best trade-off for ASan.

stephan.yichao.zhao marked 4 inline comments as done.Oct 18 2020, 10:18 AM
stephan.yichao.zhao added inline comments.
compiler-rt/lib/dfsan/dfsan.cpp
307

It seems that 32k is the boundary between calls from malloc/free and utility zero-out. Added comments.

stephan.yichao.zhao marked an inline comment as done.

addressed comments

morehouse accepted this revision.Oct 19 2020, 3:21 PM
morehouse added inline comments.
compiler-rt/lib/dfsan/dfsan.cpp
318

Since kNumPagesThreshold is only used here, let's define it here instead.

This revision is now accepted and ready to land.Oct 19 2020, 3:21 PM
stephan.yichao.zhao marked an inline comment as done.

moved constant definition.