[PAC][libunwind] Fix gcc build of libunwind and compiler-rt

[ojhunt]

This adds guards on the ptrauth feature checks so that they are only performed if __has_feature is actually available.

[llvmbot]

@llvm/pr-subscribers-libcxxabi

@llvm/pr-subscribers-libunwind

Author: Oliver Hunt (ojhunt)

Changes

This adds guards on the ptrauth feature checks so that they are only performed if __has_feature is actually available.

---

Full diff: https://github.com/llvm/llvm-project/pull/164535.diff

3 Files Affected:

• (modified) libunwind/include/__libunwind_config.h (+7-5)
• (modified) libunwind/src/UnwindRegistersRestore.S (+8-2)
• (modified) libunwind/src/UnwindRegistersSave.S (+8-2)

diff --git a/libunwind/include/__libunwind_config.h b/libunwind/include/__libunwind_config.h
index 343934e885368..980d11ef5d4f2 100644
--- a/libunwind/include/__libunwind_config.h
+++ b/libunwind/include/__libunwind_config.h
@@ -212,11 +212,13 @@
 # define _LIBUNWIND_HIGHEST_DWARF_REGISTER 287
 #endif // _LIBUNWIND_IS_NATIVE_ONLY
 
-#if __has_feature(ptrauth_calls) && __has_feature(ptrauth_returns)
-#  define _LIBUNWIND_TARGET_AARCH64_AUTHENTICATED_UNWINDING 1
-#elif __has_feature(ptrauth_calls) != __has_feature(ptrauth_returns)
-#  error "Either both or none of ptrauth_calls and ptrauth_returns "\
-         "is allowed to be enabled"
+#if defined(__has_feature)
+#  if __has_feature(ptrauth_calls) && __has_feature(ptrauth_returns)
+#    define _LIBUNWIND_TARGET_AARCH64_AUTHENTICATED_UNWINDING 1
+#  elif __has_feature(ptrauth_calls) != __has_feature(ptrauth_returns)
+#    error "Either both or none of ptrauth_calls and ptrauth_returns "\
+           "is allowed to be enabled"
+#  endif
 #endif
 
 #endif // ____LIBUNWIND_CONFIG_H__
diff --git a/libunwind/src/UnwindRegistersRestore.S b/libunwind/src/UnwindRegistersRestore.S
index 1ab4c43b673b4..2d8322bea092d 100644
--- a/libunwind/src/UnwindRegistersRestore.S
+++ b/libunwind/src/UnwindRegistersRestore.S
@@ -634,6 +634,12 @@ Lnovec:
 
 #elif defined(__aarch64__)
 
+#ifdef __has_feature
+#define __has_ptrauth_calls __has_feature(ptrauth_calls)
+#else
+#define __has_ptrauth_calls 0
+#endif
+
 #if defined(__ARM_FEATURE_GCS_DEFAULT)
 .arch_extension gcs
 #endif
@@ -690,7 +696,7 @@ DEFINE_LIBUNWIND_FUNCTION(__libunwind_Registers_arm64_jumpto)
   ldr    x16,     [x0, #0x0F8]  // load sp into scratch
   ldr    lr,      [x0, #0x100]  // restore pc into lr
 
-#if __has_feature(ptrauth_calls)
+#if __has_ptrauth_calls
   // The LR is signed with its address inside the register state.  Time
   // to resign to be a regular ROP protected signed pointer
   add    x1, x0, #0x100
@@ -711,7 +717,7 @@ DEFINE_LIBUNWIND_FUNCTION(__libunwind_Registers_arm64_jumpto)
 Lnogcs:
 #endif
 
-#if __has_feature(ptrauth_calls)
+#if __has_ptrauth_calls
   retab
 #else
   ret    x30                    // jump to pc
diff --git a/libunwind/src/UnwindRegistersSave.S b/libunwind/src/UnwindRegistersSave.S
index 31a177f4a0df4..ce67306571816 100644
--- a/libunwind/src/UnwindRegistersSave.S
+++ b/libunwind/src/UnwindRegistersSave.S
@@ -763,6 +763,12 @@ LnoR2Fix:
 
 #elif defined(__aarch64__)
 
+#ifdef __has_feature
+#define __has_ptrauth_calls __has_feature(ptrauth_calls)
+#else
+#define __has_ptrauth_calls 0
+#endif
+
 //
 // extern int __unw_getcontext(unw_context_t* thread_state)
 //
@@ -772,7 +778,7 @@ LnoR2Fix:
   .p2align 2
 DEFINE_LIBUNWIND_FUNCTION(__unw_getcontext)
 
-#if __has_feature(ptrauth_calls)
+#if __has_ptrauth_calls
   pacibsp
 #endif
 
@@ -817,7 +823,7 @@ DEFINE_LIBUNWIND_FUNCTION(__unw_getcontext)
 #endif
   mov    x0, #0                   // return UNW_ESUCCESS
 
-#if __has_feature(ptrauth_calls)
+#if __has_ptrauth_calls
   retab
 #else
   ret

[ojhunt]

Should fix the gcc build mentioned in #143230

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff origin/main HEAD --extensions c,h -- compiler-rt/lib/builtins/gcc_personality_v0.c libunwind/include/__libunwind_config.h --diff_from_common_commit

⚠️
The reproduction instructions above might return results for more than one PR
in a stack if you are using a stacked PR workflow. You can limit the results by
changing origin/main to the base branch/commit you want to compare against.
⚠️

View the diff from clang-format here.

diff --git a/libunwind/include/__libunwind_config.h b/libunwind/include/__libunwind_config.h
index 980d11ef5..709d49cbb 100644
--- a/libunwind/include/__libunwind_config.h
+++ b/libunwind/include/__libunwind_config.h
@@ -213,12 +213,12 @@
 #endif // _LIBUNWIND_IS_NATIVE_ONLY
 
 #if defined(__has_feature)
-#  if __has_feature(ptrauth_calls) && __has_feature(ptrauth_returns)
-#    define _LIBUNWIND_TARGET_AARCH64_AUTHENTICATED_UNWINDING 1
-#  elif __has_feature(ptrauth_calls) != __has_feature(ptrauth_returns)
-#    error "Either both or none of ptrauth_calls and ptrauth_returns "\
+#if __has_feature(ptrauth_calls) && __has_feature(ptrauth_returns)
+#define _LIBUNWIND_TARGET_AARCH64_AUTHENTICATED_UNWINDING 1
+#elif __has_feature(ptrauth_calls) != __has_feature(ptrauth_returns)
+#error "Either both or none of ptrauth_calls and ptrauth_returns "\
            "is allowed to be enabled"
-#  endif
+#endif
 #endif
 
 #endif // ____LIBUNWIND_CONFIG_H__

[dyung]

Will this cover the use in compiler-rt/lib/builtins/gcc_personality_v0.c?

[ojhunt]

Will this cover the use in compiler-rt/lib/builtins/gcc_personality_v0.c?

adding guards to that as well. There are enough has_feature guards there that I assumed it was a clang only project.

[dyung]

Will this cover the use in compiler-rt/lib/builtins/gcc_personality_v0.c?

adding guards to that as well. There are enough has_feature guards there that I assumed it was a clang only project.

Thanks! We definitely do build internally with GCC as well as clang, and I'm pretty sure others do build with gcc as well (although compiler-rt may not be built as often as llvm or clang).

[arichardson]

Any reason this doesn't use the standard solution for this issue?

#ifndef __has_feature
#define __has_feature(...) 0
#endif

Would make the diff smaller and is easier to add to a central header.

[ojhunt]

Any reason this doesn't use the standard solution for this issue?

#ifndef __has_feature
#define __has_feature(...) 0
#endif

Would make the diff smaller and is easier to add to a central header.

Specifically for the purpose of avoiding adding the definition to API headers.

[philnik777]

At least the libc++abi checks are completely unnecessary. We only support a single GCC version and that has __has_feature. I'm not quite clear what the libunwind requirements are, but I'm pretty sure the docs are out of date on that.

[arichardson]

Any reason this doesn't use the standard solution for this issue?

#ifndef __has_feature
#define __has_feature(...) 0
#endif

Would make the diff smaller and is easier to add to a central header.

Specifically for the purpose of avoiding adding the definition to API headers.

But you could still add it to internal files where you currently used an alternate approach?

[ojhunt]

At least the libc++abi checks are completely unnecessary. We only support a single GCC version and that has __has_feature. I'm not quite clear what the libunwind requirements are, but I'm pretty sure the docs are out of date on that.

That's a small mercy. I was surprised gcc was failing on has_feature guards as they were used specifically to avoid exposing ptrauth to compilers lacking support :-/

[ojhunt]

At least the libc++abi checks are completely unnecessary. We only support a single GCC version and that has __has_feature. I'm not quite clear what the libunwind requirements are, but I'm pretty sure the docs are out of date on that.

Actually, I just looked at the original report and it reports a failure in gcc13.x, but gcc is apparently up to 15.x - is 13.x actually supported? (I had assumed it was current gcc being reported)

[ojhunt]

Hmm, the unwind docs claim gcc 4.7 or clang 3.5 which seems optimistic?

[philnik777]

At least the libc++abi checks are completely unnecessary. We only support a single GCC version and that has __has_feature. I'm not quite clear what the libunwind requirements are, but I'm pretty sure the docs are out of date on that.

Actually, I just looked at the original report and it reports a failure in gcc13.x, but gcc is apparently up to 15.x - is 13.x actually supported? (I had assumed it was current gcc being reported)

libc++abi requires GCC 15. As I said, it's not really clear for libunwind for me. IMO we should just require the latest GCC there as well as that's the only tested version AFAIK, but that's not something we should do in a random patch.

Hmm, the unwind docs claim gcc 4.7 or clang 3.5 which seems optimistic?

Yeah, exactly. I would be very surprised if that was actually true.

[mstorsjo]

Hmm, the unwind docs claim gcc 4.7 or clang 3.5 which seems optimistic?

Yeah, exactly. I would be very surprised if that was actually true.

Well contrary to libcxxabi and libcxx, libunwind doesn't really do much advanced that requires specific compiler features (other than new additions like PAC stuff), so it wouldn't surprise me if it essentially worked fine, potentially modulo one or two trivial details or so.

[philnik777]

Hmm, the unwind docs claim gcc 4.7 or clang 3.5 which seems optimistic?

Yeah, exactly. I would be very surprised if that was actually true.

Well contrary to libcxxabi and libcxx, libunwind doesn't really do much advanced that requires specific compiler features (other than new additions like PAC stuff), so it wouldn't surprise me if it essentially worked fine, potentially modulo one or two trivial details or so.

That's true, but there is new code added from time to time, and I'm not convinced that it doesn't hit some bug in some compiler version given our test coverage. I'm generally in favour of "if you don't test it, don't claim support for it". Or do you think we should aim to support such old compiler versions? I would think that at least the general LLVM minimum requirements should apply (i.e. Clang 5 and GCC 7.4). That wouldn't help in this case, but these compiler versions might at least be actually tested by someone somewhere.

[mstorsjo]

That's true, but there is new code added from time to time, and I'm not convinced that it doesn't hit some bug in some compiler version given our test coverage. I'm generally in favour of "if you don't test it, don't claim support for it". Or do you think we should aim to support such old compiler versions? I would think that at least the general LLVM minimum requirements should apply (i.e. Clang 5 and GCC 7.4). That wouldn't help in this case, but these compiler versions might at least be actually tested by someone somewhere.

Yeah, bumping the requirement to those versions should at least be quite safe. I agree that it's perhaps not good to expressly claim to support something ancient that really isn't tested, but I don't see a need to have it as strictly versioned as e.g. libcxx/libcxxabi either, by expressly disallowing building with older versions.

[ojhunt]

Among other things I realized I didn't actually intended to include changes to libcxxabi in the first place (I started doing the work before finding other uses of __has_feature in it, so am hoping this will be fine).

For the .c and .S I've just adopted the noop #define __has_feature

A bit of search shows gcc13.3 was released in 2024 so supporting it seems reasonable.

[dyung]

A bit of search shows gcc13.3 was released in 2024 so supporting it seems reasonable.

FWIW, gcc 13.3 is the version included with ubuntu 24.04.03 LTS.

[ojhunt]

A bit of search shows gcc13.3 was released in 2024 so supporting it seems reasonable.

FWIW, gcc 13.3 is the version included with ubuntu 24.04.03 LTS.

yeah, from the release date it seems reasonable to accept it - I initially assumed a major release a year which would have made it quite a few years old, rather than a 2024 release. Given 2024 support seems reasonable.

[ojhunt]

Hmm, the unwind docs claim gcc 4.7 or clang 3.5 which seems optimistic?

Yeah, exactly. I would be very surprised if that was actually true.

Well contrary to libcxxabi and libcxx, libunwind doesn't really do much advanced that requires specific compiler features (other than new additions like PAC stuff), so it wouldn't surprise me if it essentially worked fine, potentially modulo one or two trivial details or so.

it does have a bunch of template-y stuff which can easily fall into the "weirdly requiring new compiler" but that isn't happening atm :D

[ojhunt]

At least the libc++abi checks are completely unnecessary. We only support a single GCC version and that has __has_feature. I'm not quite clear what the libunwind requirements are, but I'm pretty sure the docs are out of date on that.

Actually, I just looked at the original report and it reports a failure in gcc13.x, but gcc is apparently up to 15.x - is 13.x actually supported? (I had assumed it was current gcc being reported)

answering explicitly for others - 13.x is a 2024 release so supporting it seems reasonable

[ojhunt]

@philnik777 you had made a change request, but I pushed a rebase and the interests of obnoxiousness GitHub decided to forget the comment entirely

[ojhunt]

It's entirely reasonable to assume that if it's a gcc that doesn't support __has_feature it doesn't support pointer auth :D

[mstorsjo]

Looks ok to me

[ojhunt]

Any reason this doesn't use the standard solution for this issue?

#ifndef __has_feature
#define __has_feature(...) 0
#endif

Would make the diff smaller and is easier to add to a central header.

Specifically for the purpose of avoiding adding the definition to API headers.

But you could still add it to internal files where you currently used an alternate approach?

Yeah, I changed to this approach in the {.c,.S} files as it does make things cleaner - I initially overly paranoid about #define __has_feature(...)

[ojhunt]

I'll land now just to fix gcc, but if @philnik777 or @arichardson would like any changes please ping me (here or discord) and I'll happily make the followups