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.

Sign up for GitHub

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

Jump to bottom

[clang] move -Wcast-function-type under -Wextra #76872

Closed
nickdesaulniers opened this issue Jan 3, 2024 · 5 comments · Fixed by #77178
Closed

[clang] move -Wcast-function-type under -Wextra #76872

nickdesaulniers opened this issue Jan 3, 2024 · 5 comments · Fixed by #77178
Labels
clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer good first issue https://github.com/llvm/llvm-project/contribute

Comments

@nickdesaulniers
Copy link
Member

via #74617, I was looking into why I observe this diagnostic with gcc but not clang. It looks like our project (llvmlibc) builds with -Wextra but not -Wcast-function-type. GCC 8 added support for that flag, and did so under -Wextra.
@nickdesaulniers nickdesaulniers added good first issue https://github.com/llvm/llvm-project/contribute clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer labels Jan 3, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 3, 2024

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

  1. In the comments of the issue, request for it to be assigned to you.
  2. Fix the issue locally.
  3. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
  4. Create a Git commit.
  5. Run git clang-format HEAD~1 to format your changes.
  6. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 3, 2024

@llvm/issue-subscribers-good-first-issue

Author: Nick Desaulniers (nickdesaulniers)

via https://github.com//issues/74617, I was looking into why I observe this diagnostic with gcc but not clang. It looks like our project (llvmlibc) builds with -Wextra but not -Wcast-function-type. GCC 8 added support for that flag, and did so under -Wextra.

@nickdesaulniers nickdesaulniers mentioned this issue Jan 3, 2024
Merged
nickdesaulniers added a commit to nickdesaulniers/llvm-project that referenced this issue Jan 3, 2024
@nickdesaulniers
[libc] fix -Wcast-function-type via union rather than reinterpret_cast
b4a5b32 
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
@nickdesaulniers
[libc] fix -Wcast-function-type via union rather than reinterpret_cast (
e68a032 
#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
@Abhinkop
Copy link
Contributor

Abhinkop commented Jan 4, 2024

Can I pick up this issue?

@nickdesaulniers
Copy link
Member Author

nickdesaulniers commented Jan 4, 2024 •
edited
Loading

@Abhinkop sure. I'd expect you to change clang/include/clang/Basic/DiagnosticGroups.td's record of Extra to include CastFunctionType, then update the test for -Wcast-function-type to add an additional RUN with just -Wextra and produce the same diagnostics.

@Abhinkop Abhinkop mentioned this issue Jan 6, 2024
Merged
@Abhinkop
Copy link
Contributor

Abhinkop commented Jan 6, 2024

@nickdesaulniers Here is the pull request #77178.

AaronBallman pushed a commit that referenced this issue Mar 20, 2024
@Abhinkop
[clang] move -Wcast-function-type under -Wextra (#77178)
1de7e6c 
The -Wcast-fuction-type-strict has been moved under dignstic group
-Wextra.
Edited the test cases for -Wcast-fuction-type-strict and
-Wcast-fuction-type in Sema an SemaCXX.

Added a new test case which include a functionality that was already in
the -Wextra group, i.e -Wignored-qualifiers with
-Wcast-fuction-type-strict.

Fixes: #76872
chencha3 pushed a commit to chencha3/llvm-project that referenced this issue Mar 23, 2024
@Abhinkop @chencha3
[clang] move -Wcast-function-type under -Wextra (llvm#77178)
8f371a6 
The -Wcast-fuction-type-strict has been moved under dignstic group
-Wextra.
Edited the test cases for -Wcast-fuction-type-strict and
-Wcast-fuction-type in Sema an SemaCXX.

Added a new test case which include a functionality that was already in
the -Wextra group, i.e -Wignored-qualifiers with
-Wcast-fuction-type-strict.

Fixes: llvm#76872
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer good first issue https://github.com/llvm/llvm-project/contribute
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[clang] move -Wcast-function-type under -Wextra Abhinkop/llvm-project
3 participants
@nickdesaulniers @Abhinkop @llvmbot