-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Split -Wcast-function-type into a separate group #86131
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
Split -Wcast-function-type into a separate group #86131
Conversation
We want to add -Wcast-function-type to -Wextra (as done in 1de7e6c), but we do not want to add -Wcast-function-type-strict in at the same time (https://lab.llvm.org/buildbot/#/builders/57/builds/33601/steps/5/logs/stdio). This moves the existing warning to a new group (-Wcast-function-type-mismatch), puts the new group under the existing -Wcast-function-type warning group, and adds -Wcast-function-type-mismatch to -Wextra
CC @Abhinkop |
@llvm/pr-subscribers-clang Author: Aaron Ballman (AaronBallman) ChangesWe want to add -Wcast-function-type to -Wextra (as done in 1de7e6c), but we do not want to add -Wcast-function-type-strict in at the same time (https://lab.llvm.org/buildbot/#/builders/57/builds/33601/steps/5/logs/stdio). This moves the existing warning to a new group (-Wcast-function-type-mismatch), puts the new group under the existing -Wcast-function-type warning group, and adds -Wcast-function-type-mismatch to -Wextra. Full diff: https://github.com/llvm/llvm-project/pull/86131.diff 5 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index a9c55ef662a0b9..6939fb5b4bc953 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -196,7 +196,25 @@ Modified Compiler Flags
``-Wreturn-type``, and moved some of the diagnostics previously controlled by
``-Wreturn-type`` under this new flag. Fixes #GH72116.
-- Added ``-Wcast-function-type`` as a warning enabled by ``-Wextra``. #GH76872
+- Added ``-Wcast-function-type-mismatch`` under the ``-Wcast-function-type``
+ warning group. Moved the diagnostic previously controlled by
+ ``-Wcast-function-type`` to the new warning group and added
+ ``-Wcast-function-type-mismatch`` to ``-Wextra``. #GH76872
+
+ .. code-block:: c
+
+ int x(long);
+ typedef int (f2)(void*);
+ typedef int (f3)();
+
+ void func(void) {
+ // Diagnoses under -Wcast-function-type, -Wcast-function-type-mismatch,
+ // -Wcast-function-type-strict, -Wextra
+ f2 *b = (f2 *)x;
+ // Diagnoses under -Wcast-function-type, -Wcast-function-type-strict
+ f3 *c = (f3 *)x;
+ }
+
Removed Compiler Flags
-------------------------
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index bf03d4e8f67ee1..44035e2fd16f2e 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -573,7 +573,10 @@ def SelTypeCast : DiagGroup<"cast-of-sel-type">;
def FunctionDefInObjCContainer : DiagGroup<"function-def-in-objc-container">;
def BadFunctionCast : DiagGroup<"bad-function-cast">;
def CastFunctionTypeStrict : DiagGroup<"cast-function-type-strict">;
-def CastFunctionType : DiagGroup<"cast-function-type", [CastFunctionTypeStrict]>;
+def CastFunctionTypeMismatch : DiagGroup<"cast-function-type-mismatch">;
+def CastFunctionType : DiagGroup<"cast-function-type",
+ [CastFunctionTypeStrict,
+ CastFunctionTypeMismatch]>;
def ObjCPropertyImpl : DiagGroup<"objc-property-implementation">;
def ObjCPropertyNoAttribute : DiagGroup<"objc-property-no-attribute">;
def ObjCPropertyAssignOnObjectType : DiagGroup<"objc-property-assign-on-object-type">;
@@ -1038,7 +1041,7 @@ def Extra : DiagGroup<"extra", [
EmptyInitStatement,
StringConcatation,
FUseLdPath,
- CastFunctionType,
+ CastFunctionTypeMismatch,
]>;
def Most : DiagGroup<"most", [
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 2646942a53e30d..3d8d1e420ccfef 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9058,7 +9058,7 @@ def warn_bad_function_cast : Warning<
InGroup<BadFunctionCast>, DefaultIgnore;
def warn_cast_function_type : Warning<
"cast %diff{from $ to $ |}0,1converts to incompatible function type">,
- InGroup<CastFunctionType>, DefaultIgnore;
+ InGroup<CastFunctionTypeMismatch>, DefaultIgnore;
def warn_cast_function_type_strict : Warning<warn_cast_function_type.Summary>,
InGroup<CastFunctionTypeStrict>, DefaultIgnore;
def err_cast_pointer_to_non_pointer_int : Error<
diff --git a/clang/test/Sema/warn-cast-function-type-strict.c b/clang/test/Sema/warn-cast-function-type-strict.c
index 8c88f275d2b336..b0a70cf324b711 100644
--- a/clang/test/Sema/warn-cast-function-type-strict.c
+++ b/clang/test/Sema/warn-cast-function-type-strict.c
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 %s -fsyntax-only -Wcast-function-type -verify
-// RUN: %clang_cc1 %s -fsyntax-only -Wcast-function-type-strict -verify
+// RUN: %clang_cc1 %s -fsyntax-only -Wcast-function-type -verify=expected,strict
+// RUN: %clang_cc1 %s -fsyntax-only -Wcast-function-type-strict -verify=expected,strict
// RUN: %clang_cc1 %s -fsyntax-only -Wextra -Wno-ignored-qualifiers -verify
int t(int array[static 12]);
@@ -32,13 +32,13 @@ f10 *j;
void foo(void) {
a = (f1 *)x;
b = (f2 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f2 *' (aka 'int (*)(void *)') converts to incompatible function type}} */
- c = (f3 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f3 *' (aka 'int (*)()') converts to incompatible function type}} */
+ c = (f3 *)x; /* strict-warning {{cast from 'int (*)(long)' to 'f3 *' (aka 'int (*)()') converts to incompatible function type}} */
d = (f4 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f4 *' (aka 'void (*)()') converts to incompatible function type}} */
- e = (f5 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f5 *' (aka 'void (*)(void)') converts to incompatible function type}} */
+ e = (f5 *)x; /* strict-warning {{cast from 'int (*)(long)' to 'f5 *' (aka 'void (*)(void)') converts to incompatible function type}} */
f = (f6 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f6 *' (aka 'int (*)(long, int)') converts to incompatible function type}} */
- g = (f7 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f7 *' (aka 'int (*)(long, ...)') converts to incompatible function type}} */
+ g = (f7 *)x; /* strict-warning {{cast from 'int (*)(long)' to 'f7 *' (aka 'int (*)(long, ...)') converts to incompatible function type}} */
h = (f8 *)t;
i = (f9 *)u;
// FIXME: return type qualifier should not be included in the function type . Warning should be absent after this issue is fixed. https://github.com/llvm/llvm-project/issues/39494 .
- j = (f10 *)v; /* expected-warning {{cast from 'const int (*)(int)' to 'f10 *' (aka 'int (*)(int)') converts to incompatible function type}} */
+ j = (f10 *)v; /* strict-warning {{cast from 'const int (*)(int)' to 'f10 *' (aka 'int (*)(int)') converts to incompatible function type}} */
}
diff --git a/clang/test/SemaCXX/warn-cast-function-type-strict.cpp b/clang/test/SemaCXX/warn-cast-function-type-strict.cpp
index b3164afde5a0ca..8887b3c4c5d535 100644
--- a/clang/test/SemaCXX/warn-cast-function-type-strict.cpp
+++ b/clang/test/SemaCXX/warn-cast-function-type-strict.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 %s -fblocks -fsyntax-only -Wcast-function-type -verify
-// RUN: %clang_cc1 %s -fblocks -fsyntax-only -Wcast-function-type-strict -verify
+// RUN: %clang_cc1 %s -fblocks -fsyntax-only -Wcast-function-type -verify=expected,strict
+// RUN: %clang_cc1 %s -fblocks -fsyntax-only -Wcast-function-type-strict -verify=expected,strict
// RUN: %clang_cc1 %s -fblocks -fsyntax-only -Wextra -verify
int x(long);
@@ -33,11 +33,11 @@ void foo() {
a = (f1 *)x;
b = (f2 *)x; // expected-warning {{cast from 'int (*)(long)' to 'f2 *' (aka 'int (*)(void *)') converts to incompatible function type}}
b = reinterpret_cast<f2 *>(x); // expected-warning {{cast from 'int (*)(long)' to 'f2 *' (aka 'int (*)(void *)') converts to incompatible function type}}
- c = (f3 *)x; // expected-warning {{cast from 'int (*)(long)' to 'f3 *' (aka 'int (*)(...)') converts to incompatible function type}}
+ c = (f3 *)x; // strict-warning {{cast from 'int (*)(long)' to 'f3 *' (aka 'int (*)(...)') converts to incompatible function type}}
d = (f4 *)x; // expected-warning {{cast from 'int (*)(long)' to 'f4 *' (aka 'void (*)(...)') converts to incompatible function type}}
- e = (f5 *)x; // expected-warning {{cast from 'int (*)(long)' to 'f5 *' (aka 'void (*)()') converts to incompatible function type}}
+ e = (f5 *)x; // strict-warning {{cast from 'int (*)(long)' to 'f5 *' (aka 'void (*)()') converts to incompatible function type}}
f = (f6 *)x; // expected-warning {{cast from 'int (*)(long)' to 'f6 *' (aka 'int (*)(long, int)') converts to incompatible function type}}
- g = (f7 *)x; // expected-warning {{cast from 'int (*)(long)' to 'f7 *' (aka 'int (*)(long, ...)') converts to incompatible function type}}
+ g = (f7 *)x; // strict-warning {{cast from 'int (*)(long)' to 'f7 *' (aka 'int (*)(long, ...)') converts to incompatible function type}}
mf p1 = (mf)&S::foo; // expected-warning {{cast from 'void (S::*)(int *)' to 'mf' (aka 'void (S::*)(int)') converts to incompatible function type}}
|
This seems reasonable to me. From the perspective of the Linux kernel, this seems like it should be a no-op, as we enable
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also tested this locally, and it resolves the issue I saw earlier on the PPC bots.
Thank you all for the review help on this! |
Thanks @AaronBallman. Sorry for reporting this late. |
I think that this would occur primarily in the whole code base. Because a compilation that has added -Wextra diagnostic group would provide new warnings and coupling, this with -Werror would break the build. I think we have to take a look at these builds and investigate why it is done, and for the time being, we can use the -Wno-cast-function-type-mismatch to make the builds go green. |
@AaronBallman @amy-kwan I am working on a patch for this build breaking in https://lab.llvm.org/buildbot/#/builders/57/builds/33656. Is there anything else I should also be considering? |
Reverts: breaks hip tests 999d4f8 Split -Wcast-function-type into a separate group (llvm#86131) Change-Id: Ia384ccb1581257a44b34390acc17d1b7516e98ad
Thank you for working on the patch! I would change the definition of |
Yes. I have done that. The tests failed because zlib was not enabled. Running them with a rebuild after force-enabling zlib now. Will keep you updated on the results. |
I have also found one more that is not related to the sanitizers, but with Would it be possible to also resolve this one, as well? Just figured I mentioned since I am assuming the patch in progress resolves the compiler-rt ones specifically. |
Oof, that one is a bit messier to resolve -- it's mixing unrelated reference and pointer types and hoping the casts correctly convert back and forth properly. The fix to that should probably be done separately from the sanitizer fix so we get reasonable code review from LLVM folks. |
Yes, I will create a separate patch. I'm not sure it would be able to do it in this patch. I am halfway through verifying the fix in this patch. Builds and tests take a long time on my resource constraint machine. |
@AaronBallman @amy-kwan Here is the pull request for compiler-rt patch #86290. I am not able to add reviewers to that pull request. |
Thanks @Abhinkop for the patch! I have approved the compiler-rt patch since it does appear to resolve the check-runtimes/check-all issue for the asan test case. If you also would like me to test a follow up patch for |
Fixed a small issue of matching pthread signature, which was causing the build to break for the compiler-rt project after adding -Wcast-function-type-mismatch to -Wextra dignostic group (llvm#77178 & llvm#86131).
Hi @Abhinkop! |
Hey I was able to make some progress. I have raised a pr just now. #86504. But I am not sure exactly what might be the repercussions of my change. I am waiting for the builds. I have also asked on discord. Let's wait and see if someone replies. |
Changing type of DiagnosticHandlerTy due to adding -Wcast-function-type-mismatch to -Wextra group(#86131 (comment)). Changed the reference argument DiagnosticInfo to a pointer and edited the test cases failing due to this change. Added another small change where Gtest api was throwing an warning due varargs argument not being passed.
We want to add -Wcast-function-type to -Wextra (as done in 1de7e6c), but we do not want to add -Wcast-function-type-strict in at the same time (https://lab.llvm.org/buildbot/#/builders/57/builds/33601/steps/5/logs/stdio). This moves the existing warning to a new group (-Wcast-function-type-mismatch), puts the new group under the existing -Wcast-function-type warning group, and adds -Wcast-function-type-mismatch to -Wextra. Change-Id: I30b415f834c3655d74000c247d8cb7ff98426042
Changing type of DiagnosticHandlerTy due to adding -Wcast-function-type-mismatch to -Wextra group(llvm/llvm-project#86131 (comment)). Changed the reference argument DiagnosticInfo to a pointer and edited the test cases failing due to this change. Added another small change where Gtest api was throwing an warning due varargs argument not being passed.
Changing type of DiagnosticHandlerTy due to adding -Wcast-function-type-mismatch to -Wextra group(llvm/llvm-project#86131 (comment)). Changed the reference argument DiagnosticInfo to a pointer and edited the test cases failing due to this change. Added another small change where Gtest api was throwing an warning due varargs argument not being passed.
This warning creates issues under Windows, where reinterpret-casting from FARPROC to the actual function type is common. #92738 Reinterpret-casting a function pointer to another function pointer type, then back to the original, is well defined in C++. In contrast, using |
I think the diagnostic is working as expected in this case -- we cannot tell that the user is doing a round-trip cast, we can only see there's a single cast between incompatible function pointer types.
or
is exactly what this is intended to diagnose. Did you have a change in mind you'd like to see? |
Since you aren't diagnosing reinterpret-casting a function pointer from But perhaps I was mistaken. |
The idiomatic code doesn't diagnose?
I get no diagnostics from this when I test on Windows, so I feel like I must be missing something. |
I get the warning for your code.
Make sure you're passing /W4 to clang-cl. |
The above was a 64 bit build, in 32 bit the warning is
|
Ah, derp, the issue was me not passing |
On Windows, GetProcAddress() is the API used to dynamically load function pointers (similar to dlsym on Linux). This API returns a function pointer (a typedef named FARPROC), which means that casting from the call to the eventual correct type is technically a function type mismatch on the cast. However, because this is idiomatic code on Windows, we should accept it unless -Wcast-function-type-strict is passed. This was brought up in post-commit review feedback on llvm#86131
I put up #135660 for review, thank you for bringing this up! |
) On Windows, GetProcAddress() is the API used to dynamically load function pointers (similar to dlsym on Linux). This API returns a function pointer (a typedef named FARPROC), which means that casting from the call to the eventual correct type is technically a function type mismatch on the cast. However, because this is idiomatic code on Windows, we should accept it unless -Wcast-function-type-strict is passed. This was brought up in post-commit review feedback on #86131
… code (#135660) On Windows, GetProcAddress() is the API used to dynamically load function pointers (similar to dlsym on Linux). This API returns a function pointer (a typedef named FARPROC), which means that casting from the call to the eventual correct type is technically a function type mismatch on the cast. However, because this is idiomatic code on Windows, we should accept it unless -Wcast-function-type-strict is passed. This was brought up in post-commit review feedback on llvm/llvm-project#86131
…#135660) On Windows, GetProcAddress() is the API used to dynamically load function pointers (similar to dlsym on Linux). This API returns a function pointer (a typedef named FARPROC), which means that casting from the call to the eventual correct type is technically a function type mismatch on the cast. However, because this is idiomatic code on Windows, we should accept it unless -Wcast-function-type-strict is passed. This was brought up in post-commit review feedback on llvm#86131
…#135660) On Windows, GetProcAddress() is the API used to dynamically load function pointers (similar to dlsym on Linux). This API returns a function pointer (a typedef named FARPROC), which means that casting from the call to the eventual correct type is technically a function type mismatch on the cast. However, because this is idiomatic code on Windows, we should accept it unless -Wcast-function-type-strict is passed. This was brought up in post-commit review feedback on llvm#86131
Yes please. :-) And thanks very much. |
You're welcome, thanks for bringing the issue up and sticking with me until I understood it. :-D I've posted #135798 to backport to 20.x |
…#135660) On Windows, GetProcAddress() is the API used to dynamically load function pointers (similar to dlsym on Linux). This API returns a function pointer (a typedef named FARPROC), which means that casting from the call to the eventual correct type is technically a function type mismatch on the cast. However, because this is idiomatic code on Windows, we should accept it unless -Wcast-function-type-strict is passed. This was brought up in post-commit review feedback on llvm#86131
…#135660) On Windows, GetProcAddress() is the API used to dynamically load function pointers (similar to dlsym on Linux). This API returns a function pointer (a typedef named FARPROC), which means that casting from the call to the eventual correct type is technically a function type mismatch on the cast. However, because this is idiomatic code on Windows, we should accept it unless -Wcast-function-type-strict is passed. This was brought up in post-commit review feedback on llvm#86131
We want to add -Wcast-function-type to -Wextra (as done in 1de7e6c), but we do not want to add -Wcast-function-type-strict in at the same time (https://lab.llvm.org/buildbot/#/builders/57/builds/33601/steps/5/logs/stdio).
This moves the existing warning to a new group (-Wcast-function-type-mismatch), puts the new group under the existing -Wcast-function-type warning group, and adds -Wcast-function-type-mismatch to -Wextra.