Silence -Wcast-function-type warnings on idiomatic Windows code (#135… #135798
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AaronBallman
added
clang
|
@llvm/pr-subscribers-platform-windows Author: Aaron Ballman (AaronBallman) Changes…660) 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 Full diff: https://github.com/llvm/llvm-project/pull/135798.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index f4befc242f28b..b8f26ec9a5447 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -694,6 +694,16 @@ Improvements to Clang's diagnostics
match a template template parameter, in terms of the C++17 relaxed matching rules
instead of the old ones.
+- No longer diagnosing idiomatic function pointer casts on Windows under
+ ``-Wcast-function-type-mismatch`` (which is enabled by ``-Wextra``). Clang
+ would previously warn on this construct, but will no longer do so on Windows:
+
+ .. code-block:: c
+
+ typedef void (WINAPI *PGNSI)(LPSYSTEM_INFO);
+ HMODULE Lib = LoadLibrary("kernel32");
+ PGNSI FnPtr = (PGNSI)GetProcAddress(Lib, "GetNativeSystemInfo");
+
- Don't emit duplicated dangling diagnostics. (#GH93386).
- Improved diagnostic when trying to befriend a concept. (#GH45182).
diff --git a/clang/lib/Sema/SemaCast.cpp b/clang/lib/Sema/SemaCast.cpp
index 54bc52fa2ac40..d0d44e8899133 100644
--- a/clang/lib/Sema/SemaCast.cpp
+++ b/clang/lib/Sema/SemaCast.cpp
@@ -1151,10 +1151,33 @@ static unsigned int checkCastFunctionType(Sema &Self, const ExprResult &SrcExpr,
return false;
};
+ auto IsFarProc = [](const FunctionType *T) {
+ // The definition of FARPROC depends on the platform in terms of its return
+ // type, which could be int, or long long, etc. We'll look for a source
+ // signature for: <integer type> (*)() and call that "close enough" to
+ // FARPROC to be sufficient to silence the diagnostic. This is similar to
+ // how we allow casts between function pointers and void * for supporting
+ // dlsym.
+ // Note: we could check for __stdcall on the function pointer as well, but
+ // that seems like splitting hairs.
+ if (!T->getReturnType()->isIntegerType())
+ return false;
+ if (const auto *PT = T->getAs<FunctionProtoType>())
+ return !PT->isVariadic() && PT->getNumParams() == 0;
+ return true;
+ };
+
// Skip if either function type is void(*)(void)
if (IsVoidVoid(SrcFTy) || IsVoidVoid(DstFTy))
return 0;
+ // On Windows, GetProcAddress() returns a FARPROC, which is a typedef for a
+ // function pointer type (with no prototype, in C). We don't want to diagnose
+ // this case so we don't diagnose idiomatic code on Windows.
+ if (Self.getASTContext().getTargetInfo().getTriple().isOSWindows() &&
+ IsFarProc(SrcFTy))
+ return 0;
+
// Check return type.
if (!argTypeIsABIEquivalent(SrcFTy->getReturnType(), DstFTy->getReturnType(),
Self.Context))
diff --git a/clang/test/Sema/warn-cast-function-type-win.c b/clang/test/Sema/warn-cast-function-type-win.c
new file mode 100644
index 0000000000000..4e7ba33b258d8
--- /dev/null
+++ b/clang/test/Sema/warn-cast-function-type-win.c
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 %s -triple x86_64-windows -fsyntax-only -Wcast-function-type -Wno-cast-function-type-strict -verify=windows
+// RUN: %clang_cc1 %s -triple x86_64-windows -fsyntax-only -Wcast-function-type -Wno-cast-function-type-strict -x c++ -verify=windows
+// RUN: %clang_cc1 %s -triple x86_64-pc-linux -fsyntax-only -Wcast-function-type -Wno-cast-function-type-strict -verify=linux
+// RUN: %clang_cc1 %s -triple x86_64-pc-linux -fsyntax-only -Wcast-function-type -Wno-cast-function-type-strict -x c++ -verify=linux,linux-cpp
+// RUN: %clang_cc1 %s -triple x86_64-windows -fsyntax-only -Wcast-function-type -Wcast-function-type-strict -x c++ -verify=strict
+// windows-no-diagnostics
+
+// On Windows targets, this is expected to compile fine, and on non-Windows
+// targets, this should diagnose the mismatch. This is to allow for idiomatic
+// use of GetProcAddress, similar to what we do for dlsym. On non-Windows
+// targets, this should be diagnosed.
+typedef int (*FARPROC1)();
+typedef unsigned long long (*FARPROC2)();
+
+FARPROC1 GetProcAddress1(void);
+FARPROC2 GetProcAddress2(void);
+
+typedef int (*test1_type)(int);
+typedef float(*test2_type)();
+
+void test(void) {
+ // This does not diagnose on Linux in C mode because FARPROC1 has a matching
+ // return type to test1_type, but FARPROC1 has no prototype and so checking
+ // is disabled for further compatibility issues. In C++ mode, all functions
+ // have a prototype and so the check happens.
+ test1_type t1 = (test1_type)GetProcAddress1();
+ // linux-cpp-warning@-1 {{cast from 'FARPROC1' (aka 'int (*)()') to 'test1_type' (aka 'int (*)(int)') converts to incompatible function type}}
+ // strict-warning@-2 {{cast from 'FARPROC1' (aka 'int (*)()') to 'test1_type' (aka 'int (*)(int)') converts to incompatible function type}}
+
+ // This case is diagnosed in both C and C++ modes on Linux because the return
+ // type of FARPROC2 does not match the return type of test2_type.
+ test2_type t2 = (test2_type)GetProcAddress2();
+ // linux-warning@-1 {{cast from 'FARPROC2' (aka 'unsigned long long (*)()') to 'test2_type' (aka 'float (*)()') converts to incompatible function type}}
+ // strict-warning@-2 {{cast from 'FARPROC2' (aka 'unsigned long long (*)()') to 'test2_type' (aka 'float (*)()') converts to incompatible function type}}
+}
+
|
|
@llvm/pr-subscribers-clang Author: Aaron Ballman (AaronBallman) Changes…660) 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 Full diff: https://github.com/llvm/llvm-project/pull/135798.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index f4befc242f28b..b8f26ec9a5447 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -694,6 +694,16 @@ Improvements to Clang's diagnostics
match a template template parameter, in terms of the C++17 relaxed matching rules
instead of the old ones.
+- No longer diagnosing idiomatic function pointer casts on Windows under
+ ``-Wcast-function-type-mismatch`` (which is enabled by ``-Wextra``). Clang
+ would previously warn on this construct, but will no longer do so on Windows:
+
+ .. code-block:: c
+
+ typedef void (WINAPI *PGNSI)(LPSYSTEM_INFO);
+ HMODULE Lib = LoadLibrary("kernel32");
+ PGNSI FnPtr = (PGNSI)GetProcAddress(Lib, "GetNativeSystemInfo");
+
- Don't emit duplicated dangling diagnostics. (#GH93386).
- Improved diagnostic when trying to befriend a concept. (#GH45182).
diff --git a/clang/lib/Sema/SemaCast.cpp b/clang/lib/Sema/SemaCast.cpp
index 54bc52fa2ac40..d0d44e8899133 100644
--- a/clang/lib/Sema/SemaCast.cpp
+++ b/clang/lib/Sema/SemaCast.cpp
@@ -1151,10 +1151,33 @@ static unsigned int checkCastFunctionType(Sema &Self, const ExprResult &SrcExpr,
return false;
};
+ auto IsFarProc = [](const FunctionType *T) {
+ // The definition of FARPROC depends on the platform in terms of its return
+ // type, which could be int, or long long, etc. We'll look for a source
+ // signature for: <integer type> (*)() and call that "close enough" to
+ // FARPROC to be sufficient to silence the diagnostic. This is similar to
+ // how we allow casts between function pointers and void * for supporting
+ // dlsym.
+ // Note: we could check for __stdcall on the function pointer as well, but
+ // that seems like splitting hairs.
+ if (!T->getReturnType()->isIntegerType())
+ return false;
+ if (const auto *PT = T->getAs<FunctionProtoType>())
+ return !PT->isVariadic() && PT->getNumParams() == 0;
+ return true;
+ };
+
// Skip if either function type is void(*)(void)
if (IsVoidVoid(SrcFTy) || IsVoidVoid(DstFTy))
return 0;
+ // On Windows, GetProcAddress() returns a FARPROC, which is a typedef for a
+ // function pointer type (with no prototype, in C). We don't want to diagnose
+ // this case so we don't diagnose idiomatic code on Windows.
+ if (Self.getASTContext().getTargetInfo().getTriple().isOSWindows() &&
+ IsFarProc(SrcFTy))
+ return 0;
+
// Check return type.
if (!argTypeIsABIEquivalent(SrcFTy->getReturnType(), DstFTy->getReturnType(),
Self.Context))
diff --git a/clang/test/Sema/warn-cast-function-type-win.c b/clang/test/Sema/warn-cast-function-type-win.c
new file mode 100644
index 0000000000000..4e7ba33b258d8
--- /dev/null
+++ b/clang/test/Sema/warn-cast-function-type-win.c
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 %s -triple x86_64-windows -fsyntax-only -Wcast-function-type -Wno-cast-function-type-strict -verify=windows
+// RUN: %clang_cc1 %s -triple x86_64-windows -fsyntax-only -Wcast-function-type -Wno-cast-function-type-strict -x c++ -verify=windows
+// RUN: %clang_cc1 %s -triple x86_64-pc-linux -fsyntax-only -Wcast-function-type -Wno-cast-function-type-strict -verify=linux
+// RUN: %clang_cc1 %s -triple x86_64-pc-linux -fsyntax-only -Wcast-function-type -Wno-cast-function-type-strict -x c++ -verify=linux,linux-cpp
+// RUN: %clang_cc1 %s -triple x86_64-windows -fsyntax-only -Wcast-function-type -Wcast-function-type-strict -x c++ -verify=strict
+// windows-no-diagnostics
+
+// On Windows targets, this is expected to compile fine, and on non-Windows
+// targets, this should diagnose the mismatch. This is to allow for idiomatic
+// use of GetProcAddress, similar to what we do for dlsym. On non-Windows
+// targets, this should be diagnosed.
+typedef int (*FARPROC1)();
+typedef unsigned long long (*FARPROC2)();
+
+FARPROC1 GetProcAddress1(void);
+FARPROC2 GetProcAddress2(void);
+
+typedef int (*test1_type)(int);
+typedef float(*test2_type)();
+
+void test(void) {
+ // This does not diagnose on Linux in C mode because FARPROC1 has a matching
+ // return type to test1_type, but FARPROC1 has no prototype and so checking
+ // is disabled for further compatibility issues. In C++ mode, all functions
+ // have a prototype and so the check happens.
+ test1_type t1 = (test1_type)GetProcAddress1();
+ // linux-cpp-warning@-1 {{cast from 'FARPROC1' (aka 'int (*)()') to 'test1_type' (aka 'int (*)(int)') converts to incompatible function type}}
+ // strict-warning@-2 {{cast from 'FARPROC1' (aka 'int (*)()') to 'test1_type' (aka 'int (*)(int)') converts to incompatible function type}}
+
+ // This case is diagnosed in both C and C++ modes on Linux because the return
+ // type of FARPROC2 does not match the return type of test2_type.
+ test2_type t2 = (test2_type)GetProcAddress2();
+ // linux-warning@-1 {{cast from 'FARPROC2' (aka 'unsigned long long (*)()') to 'test2_type' (aka 'float (*)()') converts to incompatible function type}}
+ // strict-warning@-2 {{cast from 'FARPROC2' (aka 'unsigned long long (*)()') to 'test2_type' (aka 'float (*)()') converts to incompatible function type}}
+}
+
|
github-project-automation
bot
moved this from Needs Triage
to Needs Merge
in LLVM Release Status
…#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
tstellar
force-pushed
the
aballman-20.x-func-cast
branch
from
754c032 to
4da7285
Compare
github-project-automation
bot
moved this from Needs Merge
to Done
in LLVM Release Status
|
@AaronBallman (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…660)
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