Page MenuHomeFreeBSD

Fix compiler flags related to kernel coverage
ClosedPublic

Authored by tuexen on Feb 9 2019, 2:48 PM.

Details

Summary

r343746 ensured that the kernel compiles with gcc, but assumes that -fno-sanitize=all implies -fno-sanitize-coverage=trace-pc,trace-cmp, which is not true on clang 7 and gcc 8.2. This patch uses explicit flags and fixes PR 235611.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

tuexen created this revision.Feb 9 2019, 2:48 PM
lwhsu added a subscriber: lwhsu.Feb 9 2019, 4:50 PM

I tested this patch on a kernel build with clang in base and with the amd64-xtoolchain-gcc cross toolchain.

emaste added inline comments.Feb 10 2019, 2:58 PM
sys/conf/kern.mk
28 ↗(On Diff #53729)

hyphens in the variable name is unusual

tuexen updated this revision to Diff 53743.Feb 10 2019, 4:27 PM

Use underscores instead of hyphens as suggested by Ed.

tuexen added inline comments.Feb 10 2019, 4:29 PM
sys/conf/kern.mk
28 ↗(On Diff #53729)

Fixed. I should have figured out that by myself... Thanks for the comment!

andrew added inline comments.Feb 11 2019, 11:10 AM
sys/conf/files
3811 ↗(On Diff #53743)

This could be written ..."${NORMAL_C:N-fsanitize*}

sys/conf/kern.mk
28 ↗(On Diff #53729)

I'm a bit concerned we need to keep these flags in sync in multiple places. I think it would be safer to just filter out the flags we don't want.

tuexen added inline comments.Feb 11 2019, 1:40 PM
sys/conf/files
3811 ↗(On Diff #53743)

Just using compile-with "${NORMAL_C:N-fsanitize*}" does not work. No -fno-sanitize-coverage... will be used during make. Do I need to implement something to get this pattern supported?

andrew added inline comments.Feb 11 2019, 1:51 PM
sys/conf/files
3811 ↗(On Diff #53743)

It should remove any argument from the compile command line that matches the glob. This will therefore remove the -fsanitize-coverage=... flag as that will match. It will also remove any other future sanitizers, however I think this will be needed to remove any risk of them recursing into each other.

tuexen updated this revision to Diff 53778.Feb 11 2019, 2:27 PM

Follow Andrew's advice.

tuexen marked 2 inline comments as done.Feb 11 2019, 2:28 PM
tuexen added inline comments.
sys/conf/files
3811 ↗(On Diff #53743)

It actually does. I forgot the config step when building the kernel (the old way)...

emaste accepted this revision.Feb 11 2019, 3:06 PM

OK with me

This revision is now accepted and ready to land.Feb 11 2019, 3:06 PM
andrew accepted this revision.Feb 11 2019, 3:25 PM
tuexen marked an inline comment as done.Feb 11 2019, 3:38 PM
This revision was automatically updated to reflect the committed changes.