Hi, @rjmccall
I updated the patch to address your previous comments, could you help review again?
Thanks very much.

Address previous comments

In D146188#4224902, @rjmccall wrote:

In D146188#4223249, @xiongji90 wrote:

Hi, @rjmccall and @sepavloff
In UserManual, we include a section Controlling Floating Point Behavior: https://github.com/llvm/llvm-project/blob/main/clang/docs/UsersManual.rst#controlling-floating-point-behavior , now we need to add a new section for floating-point environment, do we need to add the new section in parallel with Controlling Floating Point Behavior section or just put the section under Controlling Floating Point Behavior? And current Controlling Floating Point Behavior includes following description:
Clang provides a number of ways to control floating point behavior, including with command line options and source pragmas.
So, we need to update it to Clang provides a number of ways to control floating point behavior, including with command line options, source pragmas and builtins. and mention builtin_flt_rounds, builtin_set_flt_rounds in this section. Does this meet your expectation?

I think you should add a subsection to Controlling Floating Point Behavior about the floating-point environment. You should not mention these builtins there specifically — if you want an example function that reads or modifies the environment, you should use one of the standard APIs like the FLT_ROUNDS macro or the fesetround function. You should also add the documentation for these builtins where you've currently got it, and that documentation can cross-reference to Controlling Floating Point Behavior. Does that make sense?

Add floating point environment section in UserManual.rst

Hi, @rjmccall and @sepavloff
In UserManual, we include a section Controlling Floating Point Behavior: https://github.com/llvm/llvm-project/blob/main/clang/docs/UsersManual.rst#controlling-floating-point-behavior , now we need to add a new section for floating-point environment, do we need to add the new section in parallel with Controlling Floating Point Behavior section or just put the section under Controlling Floating Point Behavior? And current Controlling Floating Point Behavior includes following description:
Clang provides a number of ways to control floating point behavior, including with command line options and source pragmas.
So, we need to update it to Clang provides a number of ways to control floating point behavior, including with command line options, source pragmas and builtins. and mention builtin_flt_rounds, builtin_set_flt_rounds in this section. Does this meet your expectation?
Thanks very much.

Combine description of builtin_flt_rounds and builtin_set_flt_rounds

In D145765#4189153, @aaron.ballman wrote:

Thanks for the additional tests! Can you also add a release note to clang/docs/ReleaseNotes.rst and document the new builtin in clang\docs\LanguageExtensions.rst?

Simplify test and update release doc

Add SemaChecking test for incompatible input arguments for __builtin_set_flt_rounds.

Add semachecking test for unsupported platform.

Hi, @andrew.w.kaylor @rjmccall @sepavloff @aaron.ballman
This patch re-lands previous patch to add builtin_set_flt_rounds, previous patch broke PPC64 buildbot due to test case bug. Previously, tests for builtin_set_flt_rounds was located in CodeGen/builtin.c which would run on all triples but this builtin was restricted to work only on Intel and Arm platform, so the builtin.c will fail on targets such as PPC64. I move the test into a separate file and use following commands to run the lit test:
RUN: %clang_cc1 -triple x86_64-gnu-linux %s -emit-llvm -o - | FileCheck %s
RUN: %clang_cc1 -triple x86_64-windows-msvc %s -emit-llvm -o - | FileCheck %s
RUN: %clang_cc1 -triple aarch64-gnu-linux %s -emit-llvm -o - | FileCheck %s
RUN: %clang_cc1 -triple aarch64-windows-msvc %s -emit-llvm -o - | FileCheck %s
Could you help review again?

Hi, @sepavloff
Do you have any concern about this patch?
Thanks very much.

Hi, @rjmccall and @sepavloff
I updated built name to "builtin_set_flt_rounds" as suggested and restrict the built to be used on x86, arm, aarch64 target. I did my local test on x86 and x86_64 and found arm and aarch64 code gen have already supported "llvm.set.rounding" :https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/ARM/ARMISelLowering.cpp#L6398 and https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp#L4528
So enabling the builtin on these targets should be OK. To document, is it ok to create another patch to add doc for both "builtin_flt_rounds" and "__builtin_set_flt_rounds"?
Thanks very much.

Update the builtin name to __builtin_set_flt_rounds and restrict it to be used only on x86, x86_64, arm, aarch64 target.

In D144454#4163688, @rjmccall wrote:

I see. If we're going to take the target-independent values specified by FLT_ROUNDS, then the original builtin name is more appropriate. Of course, this has the disadvantage of not allowing target-specific values that might exist beyond those specified in the standard; are we pretty certain that's not a problem in practice?

Working on x86, ARM, and AArch64 is great, but I'm a little worried about adding another builtin that works only on major targets and probably crashes on others. I suppose we've got some number of those already, though.

In D144454#4163658, @sepavloff wrote:

C standard function fesetround accepts rounding mode in the form of target-specific values like FE_TONEAREST. They usually represent corresponding bits in control register, so are different for different targets. llvm.set_rounding in contrast accepts rounding mode as target-independent value, the encoding is same as used in FLT_ROUNDS.

The difference between fesetround and llvm.set_rounding is same as for fegetround and FLT_ROUNDS. They differ in how rounding mode is specified, - as target-specific or as target-independent values respectively.

The intrinsic llvm.set_rounding was introduced to facilitate work on IR, because IR is (mainly) target-agnostic and defines like FE_TONEAREST are not available there. It would be nice to have a C builtin counterpart for FLT_ROUNDS, I don't know why the standard does not define it.

Implementation of fesetround with set_rounding would be very useful, but it requires translation of target-specific values for rounding mode into universal values and also checking availability of FPU, as fesetround returns value if rounding mode cannot be set. Both these checks are likely to require separate builtins.

In D144454#4162129, @rjmccall wrote:

In D144454#4157717, @xiongji90 wrote:

In D144454#4142253, @rjmccall wrote:

New builtins should be documented in the user manual.

There are standard pragmas for changing the rounding mode, right? What's the interaction between the ability to set this dynamically with a builtin call and those pragmas?

Hi, @rjmccall
Current status is LLVM has added llvm.get.rounding and llvm.set.rounding intrinsic and has also added "__builtin_flt_rounds" for llvm.get.rounding but there is no corresponding for llvm.set.rounding intrinisc. According to original patch to add llvm.set.rounding: https://reviews.llvm.org/D74729 and LLVM document for this intrinsic: https://llvm.org/docs/LangRef.html#id1506
The introduction of llvm.set.rounding intrinsic can provide same functionality as C library function "fesetround" and avoid unnecessary dependency on C library. Currently, this intrinsic can't benefit C/C++ developers since we don't have a corresponding builtin and we even can't add a C/C++ test for this intrinsic.

If this builtin is defined to have the same behavior as fesetround, the obvious name would be __builtin_fesetround. You seem to have patterned it off of __builtin_flt_rounds, but that is not an arbitrarily-chosen name, it's taken from the standard FLT_ROUNDS macro and is expected to work as an implementation of it.

We already understand how fesetround is supposed to interact with the standard pragmas about the FP environment, so that indirectly answers that part of my question. I assume you've modeled your intrinsic correctly in LLVM so that the optimizer understands it can't reorder the environment-sensitive math intrinsics around calls to the new intrinsic?

If this builtin is just an intrinsic implementation of fesetround, I don't think we need further documentation of it.

I assume the intrinsic is only implemented on certain targets? If so, we might need to restrict uses of the builtin as well (and make sure that __has_builtin only returns true on those targets).

Change the builtin name to __builtin_fesetround

In D144454#4142253, @rjmccall wrote:

New builtins should be documented in the user manual.

There are standard pragmas for changing the rounding mode, right? What's the interaction between the ability to set this dynamically with a builtin call and those pragmas?

Update test for llvm.set.rounding

In D144454#4142253, @rjmccall wrote:

New builtins should be documented in the user manual.

There are standard pragmas for changing the rounding mode, right? What's the interaction between the ability to set this dynamically with a builtin call and those pragmas?

Hi, @MaskRay
Could you take a look at this PR? This trivial patch aims to fix a potential typo.

Hi, @MaskRay
Unnecessary check in forced unwind is removed. This patch is to align with libgcc's behavior in stack unwinding process on CET enabled platform. libgcc added check in stacking unwinding phase2, we need to go through the stacks to count how many stack frames to skip when CET is enabled and we can enhance security by comparing return addr in normal stack against CET shadow stack at the same time. If return addr in the 2 stacks don't match, it means the normal stack has been corrupted by someone and we can report fatal error in advance.
We also ran libunwind, libcxx, libcxxabi tests with CET enabled with latest patch, all tests passed.

In D136667#3897494, @MaskRay wrote:

Is this check a libgcc behavior? Is it intended for just C++ exceptions (regular phase2) or also for _Unwind_ForcedUnwind?

Can you craft an example to demonstrate a failure caught by the new check? It is going to be brittle so I can understand that a test is likely inappropriate.

For C++ exceptions exception_object->exception_class != 0 is always true while for _Unwind_ForcedUnwind exception_class is typically zero, so I am not sure having the code in unwind_phase2_forced is useful...

In D136317#3881482, @MaskRay wrote:

Can you add the stacktrace to the description?

In D136667#3883963, @MaskRay wrote:

Is there any way to validate correctness without a CET-capable machine?
Is there any emulator

Hi, @hjl.tools
Could you take a look at this patch, we aim to align with libgcc's behavior in CET shadow stack unwinding process in order to enhance security.

Hi, @MaskRay and @compnerd
Could you help review this patch?
Thanks very much.

Hi, @MaskRay and @compnerd
Could you help review this little fix for libunwind CET build break?

Hi, @MaskRay and @compnerd
I have updated the patch based on your comments and may need your approval again.
Thanks very much for the review.

Hi, @compnerd
Kind ping~

Hi, @compnerd
Kind ping~

Hi, @compnerd
Could you check again if the latest update meets your requirement?
Thanks very much.

Hi, @compnerd , @MaskRay and @manojgupta
I have updated the patches according to previous comments, could you help review it?

In D109811#3065945, @MaskRay wrote:

If no customization is needed (like libc++/libc++abi D107560), why not just require explicit CXXFLAGS? I don't see a need for a new CMake option.

In D109811#3065945, @MaskRay wrote:

If no customization is needed (like libc++/libc++abi D107560), why not just require explicit CXXFLAGS? I don't see a need for a new CMake option.

Hi, @MaskRay
I removed endbr in builtin .S file according to your suggestion, could you help review again?
Thanks very much.

In D109811#3052510, @MaskRay wrote:

In D109811#3050475, @xiongji90 wrote:

In D109811#3021479, @MaskRay wrote:

Ugh, wait. Can functions like __floatundisf be indirect call targets? Compilers generate direct calls to them and we don't expect users to indirectly call them.

I also don't find AArch64 bti notation.

Hi, @MaskRay
I agree that we don't expect users to call those builtin functions indirectly but compiler doesn't have any mechanism to forbid users to do that currently. As long as those builtin functions are exported, user can call them directly or indirectly and this may lead to CET IBT violation.

That will be pilot error. Unsupporting that use case is ok.

If there is legitimate use case (I doubt), we can add endbr as needed. The defensive approach is unnecessary.

I suggest to keep the "endbr" here, keeping the "endbr" in builtin functions just adds several endbr instructions into final binary and it can make CET enabling more unified, all functions will start with "endbr" when IBT is enabled, it will also avoid crash when some users are playing tricks with those builtin function via indirect calls.

I believe libgcc has much fewer endbr annotations.

Thanks very much.

In D109811#3021479, @MaskRay wrote:

Ugh, wait. Can functions like __floatundisf be indirect call targets? Compilers generate direct calls to them and we don't expect users to indirectly call them.

I also don't find AArch64 bti notation.

In D109811#3045740, @manojgupta wrote:

I tried to use it locally and ran into following issues:

ld.lld: warning: /usr/lib64/clang/14.0.0/lib/linux/clang_rt.crtbegin-x86_64.o: -z force-ibt: file does not have GNU_PROPERTY_X86_FEATURE_1_IBT property
ld.lld: warning: /usr/lib64/clang/14.0.0/lib/linux/clang_rt.crtend-x86_64.o: -z force-ibt: file does not have GNU_PROPERTY_X86_FEATURE_1_IBT property

Apparently crt* files are not built with fcf-protection=full flag.

Hi, @compnerd , @MaskRay and @efriedma

include <cet.h> and define CET endbr macros only when "CET" is defined. When using gcc or clang, "CET" is defined when -fcf-protection option is used.

Hi, @MaskRay
could you help review this patch?
Thanks very much.

In D105968#2963188, @MaskRay wrote:

In D105968#2962009, @xiongji90 wrote:

In D105968#2961508, @MaskRay wrote:

There are still many unresolved comments. Please mark resolved ones as done and then click "Submit".

Hi, @MaskRay
Left unresolved comments are resolved and I also rebased the patch to latest llvm libunwind code.

Thanks very much.

You marked your own comments as "Done" but not reviewers' comments as "Done".

So I currently see 35 among 79 comments are done, while the rest are not.

Please mark resolved ones as done.

If you need a reviewer to commit this patch on your behalf, please provide git commit --amend --author="name <email>".