[libcxx-commits] [PATCH] D107560: [libc++][libc++abi][CET] Support building libc++ and libc++abi with CET enabled.

[xiongji90 via Phabricator via libcxx-commits]

xiongji90 added a comment.

In D107560#2945032 <https://reviews.llvm.org/D107560#2945032>, @Mordante wrote:

> In D107560#2938901 <https://reviews.llvm.org/D107560#2938901>, @xiongji90 wrote:
>
>> In D107560#2933130 <https://reviews.llvm.org/D107560#2933130>, @Mordante wrote:
>>
>>>> This patch is used to build libc++ and libc++abi with CET enabled. If developers want to build applications with CET enabled and use libc++ and libc++abi, they must link CET enabled libc++ and libc++abi libraries. We introduce 2 options: LIBCXX_ENABLE_CET and LIBCXXABI_ENABLE_CET to enable CET building. This patch also enables running all libcxx and libcxxabi tests with CET enabled.
>>>
>>> I wonder what happens if only one of the two libraries is build with the flag enabled. Will these libraries still be compatible but with CET disabled? Or will they be incompatible?
>>
>> Hi, @Mordante 
>> Do you mean if we build libc+, libc++abi with CET enabled and use them in a non-CET environment? If so, they can still work.  After applying "-fcf-protection=full", "endbr32/64" instruction will be added to the beginning of all functions, the "endbr32/64" instruction will be regarded as "nop" instruction in non-CET environment.
>> Thanks very much.
>
> You've added two flags `LIBCXX_ENABLE_CET` and `LIBCXXABI_ENABLE_CET`. I'm more wondering about when the user enables only `LIBCXXABI_ENABLE_CET` will this work?
>
> So I'm concerned that there are two flags, but it might be required that both have the same value.

Hi, @Mordante 
Do you have some scenario that libcxxabi is used alone withouth libcxx? If there  is no such scenario, I also agree that LIBCXXABI_ENABLE_CET is unnecessary. Maybe we can do following:

1. Just use LIBCXX_ENABLE_CET in libcxxabi and remove LIBCXXABI_ENABLE_CET
2. We can use such code in libcxxabi CMakeFile "option(LIBCXXABI_ENABLE_CET "Build libc++abi with CET enabled." ${LIBCXX_ENABLE_CET})"

> libc++ and libc++abi already have their own internal `LIBCXX(ABI|)_COMPILE_FLAGS` in CMake. So maybe adding a way to set their initial value from CMake would be a more generic approach. This would make it easier for Intel and other vendors to use their own special flags without needing changes to libc++ and libc++abi.

Do you mean we should add a way to set LIBCXX(ABI)_COMPILE_FLAGS in CMakeFile instead of adding the compiling flag to common CXX flag?

Thanks very much.



================
Comment at: libcxx/CMakeLists.txt:158
 
+if (LIBCXX_ENABLE_CET AND MSVC)
+  message(FATAL_ERROR "libc++ CET support is not available for MSVC!")
----------------
Mordante wrote:
> xiongji90 wrote:
> > Mordante wrote:
> > > Can you also validate the build is on the x86 platform?
> > > Question regarding x86, do you mean the 32-bit platform or both the 32-bit and 64-bit platform.
> > > To me x86 means only the 32-bit platform.
> > Hi, @Mordante 
> > "Can you also validate the build is on the x86 platform?"
> > 
> > Do you mean we need to check whether we are building the library on a x86 platform?
> > 
> > "Question regarding x86, do you mean the 32-bit platform or both the 32-bit and 64-bit platform."
> > 
> > Sorry for the confusion, I mean both 32-bit and 64-bit platform and I have tested both 32-bit and 64-bit.
> > Thanks very much.
> > 
> libc++ is available on more platforms than x86 and X86-64. So I think we should either warn for all invalid combinations or none. The test `target_add_compile_flags_if_supported` already validates whether the flag is supported.
Hi, @Mordante 
Since CET is only implemented in Intel platform, I can check whether the compiler supports "-fcf-protection=full" and report error if it doesn't support.
Thanks very much.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107560/new/

https://reviews.llvm.org/D107560