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] -Wcast-function-type in libc/src/signal/linux/signal_utils.h #74617

Closed
nickdesaulniers opened this issue Dec 6, 2023 · 5 comments · Fixed by #76875
Closed

[libc] -Wcast-function-type in libc/src/signal/linux/signal_utils.h #74617

nickdesaulniers opened this issue Dec 6, 2023 · 5 comments · Fixed by #76875

Comments

@nickdesaulniers
Copy link
Member

[270/361] Building CXX object projects/libc/src/signal/linux/CMakeFiles/libc.src.signal.linux.raise.dir/raise.cpp.o
In file included from /android0/llvm-project/libc/src/signal/linux/raise.cpp:10:
/android0/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&)’:
/android0/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);
      |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/android0/llvm-project/libc/src/signal/linux/signal_utils.h: In member function ‘__llvm_libc_18_0_0_git::KernelSigaction::operator sigaction() const’:
/android0/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);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I'm trying to get -Werror re-enabled in #74506; building with GCC flags the above warning.

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 6, 2023

@llvm/issue-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

``` [270/361] Building CXX object projects/libc/src/signal/linux/CMakeFiles/libc.src.signal.linux.raise.dir/raise.cpp.o In file included from /android0/llvm-project/libc/src/signal/linux/raise.cpp:10: /android0/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&)’: /android0/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); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /android0/llvm-project/libc/src/signal/linux/signal_utils.h: In member function ‘__llvm_libc_18_0_0_git::KernelSigaction::operator sigaction() const’: /android0/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); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ``` I'm trying to get -Werror re-enabled in https://github.com//pull/74506; building with GCC flags the above warning.

@nickdesaulniers
Copy link
Member Author

https://android.googlesource.com/platform/bionic/+/refs/heads/main/libc/include/bits/signal_types.h?autodive=0%2F%2F#60

alludes to the issue that is mentioned near the warning.

// The POSIX definition of struct sigaction and the sigaction data structure
// expected by the rt_sigaction syscall differ in their definition. So, we

@SchrodingerZhu
Copy link
Contributor

Casting to an intermediate value should suppress the warning. See https://godbolt.org/z/PncMfvfz7

using T0 = void (*)(int, void*, void*);
using T1 = void (*)(int);

T0 test(T1 f) {
    return reinterpret_cast<T0>(f);
}

T0 test1(T1 f) {
    void * ptr = reinterpret_cast<void *>(f);
    return reinterpret_cast<T0>(ptr);
}

@nickdesaulniers
Copy link
Member Author

Sure, we can probably get by with laundering the type through a void*. Though the comment in bionic makes me think that our current implementation is wrong; or might need to be adjusted further in the future.

@SchrodingerZhu
Copy link
Contributor

SchrodingerZhu commented Dec 6, 2023

Would HandlerType = void (*) (int, ...) work then?

Casting around

void (*a)(int, ...);
void (*b)((int, void*, void*);

will just work without warning. Per psABI's view, it seems to be safe enough.

However, I suppose you are talking about separating them based on implementations rather than figuring out an generic type that just works.

nickdesaulniers added a commit to nickdesaulniers/llvm-project that referenced this issue Jan 3, 2024
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
nickdesaulniers added a commit that referenced this issue Jan 3, 2024
#76875)

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants