This is an archive of the discontinued LLVM Phabricator instance.

builtins test: Move clear_cache_test.c from a mprotect()ed global to a mmap()ed variable
ClosedPublic

Authored by thakis on Sep 23 2019, 1:05 PM.

Details

Summary

ld64 in the macOS 10.15 SDK gives __DATA a maxprot of 3, meaning it can't be made executable at runtime by default.

Change clear_cache_test.c to use mmap()ed data that's mapped as writable and executable from the beginning, instead of trying to mprotect()ing a __DATA variable as executable. This fixes the test on macOS with the 10.15 SDK.

PR43407.

Diff Detail

Event Timeline

thakis created this revision.Sep 23 2019, 1:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2019, 1:05 PM

delcypher: ping :)

delcypher requested changes to this revision.Sep 24 2019, 12:45 PM
delcypher added inline comments.
compiler-rt/test/builtins/Unit/clear_cache_test.c
46–58

start, and end aren't being used anymore. We should just remove them.

If we decide not to use page_size we should remove it too (and the code that computes `page_size).

48

I don't like repetition of the 128 constant. We could define a global const (e.g. kExecutionBufferSize) and use it here.
However, we aren't using page_size anymore here and arguably we could use it here instead of 128 (which seems like a really arbitrary size). However, mmap has to round up to the page size anyway so maybe dropping page_size and the code that computes it is the cleanest thing to do?

This revision now requires changes to proceed.Sep 24 2019, 12:45 PM
thakis updated this revision to Diff 221616.Sep 24 2019, 4:03 PM

comments

Thanks, good comments. All done.

delcypher accepted this revision.Sep 24 2019, 11:24 PM

I just realised. It's kind of weird that this test has code to support ARM but the test is marked as unsupported for arm. Anyway fixing that is out of scope for this patch so LGTM.

This revision is now accepted and ready to land.Sep 24 2019, 11:24 PM
This revision was automatically updated to reflect the committed changes.