Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc] fix -Wcast-function-type via union rather than reinterpret_cast #76875

Merged
merged 1 commit into from
Jan 3, 2024

Conversation

nickdesaulniers
Copy link
Member

The GCC build is producing the following diagnostic:

llvm-project/libc/src/signal/linux/signal_utils.h: In member function
‘__llvm_libc_18_0_0_git::KernelSigaction&
__llvm_libc_18_0_0_git::KernelSigaction::operator=(const sigaction&)’:
llvm-project/libc/src/signal/linux/signal_utils.h:38:20: warning:
cast between incompatible function types from ‘void (*)(int, siginfo_t*,
void*)’ to ‘void (*)(int)’ [-Wcast-function-type]
   38 |       sa_handler = reinterpret_cast<HandlerType *>(sa.sa_sigaction);
      |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
llvm-project/libc/src/signal/linux/signal_utils.h: In member function
‘__llvm_libc_18_0_0_git::KernelSigaction::operator sigaction() const’:
llvm-project/libc/src/signal/linux/signal_utils.h:51:25: warning:
cast between incompatible function types from ‘void (*)(int)’ to ‘void
(*)(int, siginfo_t*, void*)’ [-Wcast-function-type]
   51 |       sa.sa_sigaction = reinterpret_cast<SiginfoHandlerType *>(sa_handler);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Two issues here:

  1. Clang supports -Wcast-function-type, but not as part of the -Wextra group.
  2. The existing implementation tried to work around the oddity that is the
    kernel's struct sigaction != POSIX via reinterpret_cast in a way that's not
    compatible with -Wcast-function-type. Just use a union which is well defined
    (and two function pointers are the same size.)

Link: #76872

Fixes: #74617

The GCC build is producing the following diagnostic:

    llvm-project/libc/src/signal/linux/signal_utils.h: In member function
    ‘__llvm_libc_18_0_0_git::KernelSigaction&
    __llvm_libc_18_0_0_git::KernelSigaction::operator=(const sigaction&)’:
    llvm-project/libc/src/signal/linux/signal_utils.h:38:20: warning:
    cast between incompatible function types from ‘void (*)(int, siginfo_t*,
    void*)’ to ‘void (*)(int)’ [-Wcast-function-type]
       38 |       sa_handler = reinterpret_cast<HandlerType *>(sa.sa_sigaction);
          |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    llvm-project/libc/src/signal/linux/signal_utils.h: In member function
    ‘__llvm_libc_18_0_0_git::KernelSigaction::operator sigaction() const’:
    llvm-project/libc/src/signal/linux/signal_utils.h:51:25: warning:
    cast between incompatible function types from ‘void (*)(int)’ to ‘void
    (*)(int, siginfo_t*, void*)’ [-Wcast-function-type]
       51 |       sa.sa_sigaction = reinterpret_cast<SiginfoHandlerType *>(sa_handler);
          |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Two issues here:
1. Clang supports -Wcast-function-type, but not as part of the -Wextra group.
2. The existing implementation tried to work around the oddity that is the
kernel's struct sigaction != POSIX via reinterpret_cast in a way that's not
compatible with -Wcast-function-type. Just use a union which is well defined
(and two function pointers are the same size.)

Link: llvm#76872

Fixes: llvm#74617
@llvmbot llvmbot added the libc label Jan 3, 2024
@llvmbot
Copy link

llvmbot commented Jan 3, 2024

@llvm/pr-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

Changes

The GCC build is producing the following diagnostic:

llvm-project/libc/src/signal/linux/signal_utils.h: In member function
‘__llvm_libc_18_0_0_git::KernelSigaction&amp;
__llvm_libc_18_0_0_git::KernelSigaction::operator=(const sigaction&amp;)’:
llvm-project/libc/src/signal/linux/signal_utils.h:38:20: warning:
cast between incompatible function types from ‘void (*)(int, siginfo_t*,
void*)’ to ‘void (*)(int)’ [-Wcast-function-type]
   38 |       sa_handler = reinterpret_cast&lt;HandlerType *&gt;(sa.sa_sigaction);
      |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
llvm-project/libc/src/signal/linux/signal_utils.h: In member function
‘__llvm_libc_18_0_0_git::KernelSigaction::operator sigaction() const’:
llvm-project/libc/src/signal/linux/signal_utils.h:51:25: warning:
cast between incompatible function types from ‘void (*)(int)’ to ‘void
(*)(int, siginfo_t*, void*)’ [-Wcast-function-type]
   51 |       sa.sa_sigaction = reinterpret_cast&lt;SiginfoHandlerType *&gt;(sa_handler);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Two issues here:

  1. Clang supports -Wcast-function-type, but not as part of the -Wextra group.
  2. The existing implementation tried to work around the oddity that is the
    kernel's struct sigaction != POSIX via reinterpret_cast in a way that's not
    compatible with -Wcast-function-type. Just use a union which is well defined
    (and two function pointers are the same size.)

Link: #76872

Fixes: #74617


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

1 Files Affected:

  • (modified) libc/src/signal/linux/signal_utils.h (+6-6)
diff --git a/libc/src/signal/linux/signal_utils.h b/libc/src/signal/linux/signal_utils.h
index ca6fd3aeb1a6ed..5e9dd9a5c53ab0 100644
--- a/libc/src/signal/linux/signal_utils.h
+++ b/libc/src/signal/linux/signal_utils.h
@@ -27,15 +27,12 @@ namespace LIBC_NAMESPACE {
 // handler taking siginfo_t * argument, one can set sa_handler to sa_sigaction
 // if SA_SIGINFO is set in sa_flags.
 struct KernelSigaction {
-  using HandlerType = void(int);
-  using SiginfoHandlerType = void(int, siginfo_t *, void *);
-
   LIBC_INLINE KernelSigaction &operator=(const struct sigaction &sa) {
     sa_flags = sa.sa_flags;
     sa_restorer = sa.sa_restorer;
     sa_mask = sa.sa_mask;
     if (sa_flags & SA_SIGINFO) {
-      sa_handler = reinterpret_cast<HandlerType *>(sa.sa_sigaction);
+      sa_sigaction = sa.sa_sigaction;
     } else {
       sa_handler = sa.sa_handler;
     }
@@ -48,13 +45,16 @@ struct KernelSigaction {
     sa.sa_mask = sa_mask;
     sa.sa_restorer = sa_restorer;
     if (sa_flags & SA_SIGINFO)
-      sa.sa_sigaction = reinterpret_cast<SiginfoHandlerType *>(sa_handler);
+      sa.sa_sigaction = sa_sigaction;
     else
       sa.sa_handler = sa_handler;
     return sa;
   }
 
-  HandlerType *sa_handler;
+  union {
+    void (*sa_handler)(int);
+    void (*sa_sigaction)(int, siginfo_t *, void *);
+  };
   unsigned long sa_flags;
   void (*sa_restorer)(void);
   // Our public definition of sigset_t matches that of the kernel's definition.

@nickdesaulniers
Copy link
Member Author

cc @SchrodingerZhu

Copy link
Contributor

@SchrodingerZhu SchrodingerZhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@nickdesaulniers nickdesaulniers merged commit e68a032 into llvm:main Jan 3, 2024
5 checks passed
@nickdesaulniers nickdesaulniers deleted the cast-function-type branch January 3, 2024 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[libc] -Wcast-function-type in libc/src/signal/linux/signal_utils.h
4 participants