This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] [test] Fix page address logic in clear_cache_test
ClosedPublic

Authored by mgorny on Jan 18 2017, 12:56 AM.

Details

Summary

Fix the logic used to calculate page boundaries in clear_cache_test to
use correct masks -- e.g. -4096 rather than -4095. The latter gives
incorrect result since:

-4095 -> 0xfffff001
-4096 -> 0xfffff000 (== ~4095)

The issue went unnoticed so far because the array alignment caused
the last bit not to be set. However, on 32-bit x86 no such alignment is
enforced and the wrong page address caused the test to fail.

Furthermore, obtain the page size from the system instead of hardcoding
4096.

Diff Detail

Event Timeline

mgorny created this revision.Jan 18 2017, 12:56 AM
compnerd edited edge metadata.Jan 18 2017, 7:17 PM

I wonder if we should take this a bit further. Use something like a PAGE_SIZE constant to avoid this confusion entirely. Furthremore, different targets could use different page sizes. IIRC, SPARC has a 8K page size by default.

compnerd requested changes to this revision.Jan 18 2017, 7:17 PM
This revision now requires changes to proceed.Jan 18 2017, 7:17 PM
mgorny updated this revision to Diff 84947.Jan 19 2017, 12:14 AM
mgorny edited edge metadata.
mgorny retitled this revision from [compiler-rt] [test] Fix page address logic in clear_cache_test to use binary negation to [compiler-rt] [test] Fix page address logic in clear_cache_test.
mgorny edited the summary of this revision. (Show Details)

Updated to use page size logic on POSIX and Windows systems. I haven't tested the latter, though.

compnerd added inline comments.Jan 19 2017, 8:24 PM
test/builtins/Unit/clear_cache_test.c
71

Shouldnt this read:

char *end = (char *)((uintptr_t)(&execution_buffer[128 + get_page_size()] & (-get_page_size())));
mgorny updated this revision to Diff 85208.Jan 20 2017, 3:42 PM
mgorny marked an inline comment as done.

Fixed the missing change. Also created a local variable to avoid calling the sysconf three times.

compnerd accepted this revision.Jan 21 2017, 8:44 AM

Would be nice to clang-format those lines, but its no worse than before.

This revision is now accepted and ready to land.Jan 21 2017, 8:44 AM
This revision was automatically updated to reflect the committed changes.