Page MenuHomePhabricator

Reland [sanitizer] Support Intel CET
ClosedPublic

Authored by hjl.tools on Oct 5 2021, 2:57 PM.

Details

Summary
  1. Include <cet.h> in sanitizer_common/sanitizer_asm.h, if it exists, to mark Intel CET support when Intel CET is enabled.
  2. Define _CET_ENDBR as empty if it isn't defined.
  3. Add _CET_ENDBR to function entries in assembly codes so that ENDBR instruction will be generated when Intel CET is enabled.

Tested on Intel Tiger Lake with Intel CET enabled.

Diff Detail

Event Timeline

hjl.tools requested review of this revision.Oct 5 2021, 2:57 PM
hjl.tools created this revision.
dvyukov accepted this revision.Oct 5 2021, 9:54 PM
This revision is now accepted and ready to land.Oct 5 2021, 9:54 PM
This revision was landed with ongoing or failed builds.Oct 6 2021, 10:14 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2021, 10:14 AM

Thanks!

Instead of creating a new diff, you may change the local git description, then run arc diff --verbatim --head=head 'HEAD^' to update the Differential's subject.

Some intro to arc (https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line)

Thanks!

Instead of creating a new diff, you may change the local git description, then run arc diff --verbatim --head=head 'HEAD^' to update the Differential's subject.

Some intro to arc (https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line)

Thanks for the tip.

This is causing problems with gcc versions that don't have cet.h yet. I think cet.h wasn't added until gcc 8.

MaskRay added a comment.EditedOct 6 2021, 4:18 PM

This is causing problems with gcc versions that don't have cet.h yet. I think cet.h wasn't added until gcc 8.

Use -DLLVM_ENABLE_RUNTIMES=compiler-rt instead of -DLLVM_ENABLE_PROJECTS=compiler-rt to build with freshly built Clang with cet.h in its resource directory.

I think the trend is that older GCC will be unsupported when building runtime (including libc++/libc++abi/libunwind/compiler-rt; "building" != "using", "using" needs to support a wide range of compilers), even if most other toplevel projects of llvm-project target GCC>=5.

E.g.
https://lists.llvm.org/pipermail/llvm-dev/2021-March/148881.html "Compiler support in libc++" says "We support the latest major release of GCC, as advertised on https://gcc.gnu.org/releases.html."
So it seems fairly reasonable to me that a quite old GCC (7.x) cannot build in -DLLVM_ENABLE_PROJECTS=compiler-rt mode. @phosek

This is causing problems with gcc versions that don't have cet.h yet. I think cet.h wasn't added until gcc 8.

Please try this:

MaskRay added a subscriber: phosek.Oct 6 2021, 4:27 PM

This is causing problems with gcc versions that don't have cet.h yet. I think cet.h wasn't added until gcc 8.

Please try this:

Thank you. Are you planning to push this patch to main? This is affecting one of our internal bots that's building the main branch, so either maskray's solution or yours would work if it goes into main.

This is causing problems with gcc versions that don't have cet.h yet. I think cet.h wasn't added until gcc 8.

Please try this:

Thank you. Are you planning to push this patch to main? This is affecting one of our internal bots that's building the main branch, so either maskray's solution or yours would work if it goes into main.

I can submit it if it works for you.

This is causing problems with gcc versions that don't have cet.h yet. I think cet.h wasn't added until gcc 8.

Please try this:

Thank you. Are you planning to push this patch to main? This is affecting one of our internal bots that's building the main branch, so either maskray's solution or yours would work if it goes into main.

I'd prefer we don't work around such usage which is unsupported. This is an opportunity for users to migrate to -DLLVM_ENABLE_RUNTIMES=compiler-rt

I'd prefer we don't work around such usage which is unsupported. This is an opportunity for users to migrate to -DLLVM_ENABLE_RUNTIMES=compiler-rt

I agree, it would be awkward. We'll give your solution a try.

thakis added a subscriber: thakis.Oct 7 2021, 7:10 PM

This breaks building on macOS: https://bugs.chromium.org/p/chromium/issues/detail?id=1257863

Please take a look.

xiongji90 added inline comments.Oct 7 2021, 7:13 PM
compiler-rt/lib/sanitizer_common/sanitizer_asm.h
70

Hi, @hjl.tools
We had better guard the cet.h header using "linux" macro, since CET is only implemented on x86.x86_64 linux platforms currently.

Thanks very much.

MaskRay added inline comments.Oct 7 2021, 7:24 PM
compiler-rt/lib/sanitizer_common/sanitizer_asm.h
70

Probably use __ELF__ so that other ELF OSes can pick up CET easily.

xiongji90 added inline comments.Oct 7 2021, 7:39 PM
compiler-rt/lib/sanitizer_common/sanitizer_asm.h
70

Hi, @MaskRay
If CET is not implemented on all ELF based OS, I am afraid using "ELF" may introduce risk, we may encounter " 'cet.h' is not found" error.
Thanks very much.

MaskRay added inline comments.Oct 7 2021, 7:43 PM
compiler-rt/lib/sanitizer_common/sanitizer_asm.h
70

FreeBSD 13.0 has /usr/lib/clang/11.0.1/include/cet.h

Clang resource headers generally include files for all arches, I don't understand why @thakis's macOS platform doesn't have cet.h, though.

aeubanks added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_asm.h
70

Other places that include <cet.h> only do so on Linux. I'm reverting this for now while we figure out the non-Linux story.

hjl.tools added inline comments.Oct 7 2021, 9:12 PM
compiler-rt/lib/sanitizer_common/sanitizer_asm.h
70

There is a patch here

. Does it work for you?

This is causing problems with gcc versions that don't have cet.h yet. I think cet.h wasn't added until gcc 8.

Please try this:

Thank you. Are you planning to push this patch to main? This is affecting one of our internal bots that's building the main branch, so either maskray's solution or yours would work if it goes into main.

I'd prefer we don't work around such usage which is unsupported. This is an opportunity for users to migrate to -DLLVM_ENABLE_RUNTIMES=compiler-rt

Is this actually unsupported? Do we currently claim to only support building sanitizers via the runtime build? (I remember something like that for libc++, but not for sanitizers)
https://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library says we allow gcc 5.1.

I agree we should ideally migrate to using the runtimes build, but that's not trivial (e.g. in our case it also turns on LLVM_ENABLE_PER_TARGET_RUNTIME_DIR, which we've turned off).

compiler-rt/lib/sanitizer_common/sanitizer_asm.h
70

That would probably work, but that's a solution to a slightly different problem of too old of a host compiler. I'm fine with relanding this change either with your patch or a check for Linux.

hjl.tools updated this revision to Diff 378192.Oct 8 2021, 6:05 AM
hjl.tools retitled this revision from [sanitizer] Support Intel CET to Reland [sanitizer] Support Intel CET.
hjl.tools edited the summary of this revision. (Show Details)
hjl.tools added reviewers: aeubanks, wolfgangp, thakis.
MaskRay reopened this revision.Oct 8 2021, 10:19 AM
This revision is now accepted and ready to land.Oct 8 2021, 10:19 AM
MaskRay accepted this revision.Oct 8 2021, 10:20 AM
MaskRay added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_asm.h
70

Use && to pack two #if together.

This revision was landed with ongoing or failed builds.Oct 8 2021, 10:23 AM
This revision was automatically updated to reflect the committed changes.
hjl.tools added inline comments.Oct 8 2021, 10:25 AM
compiler-rt/lib/sanitizer_common/sanitizer_asm.h
70

Use && to pack two #if together.

Oops. I have pushed my original patch. Should I create an incremental patch?