Inconsistent (missing) incompatible function pointer type warnings #41465

brunodefraine · 2019-06-04T08:33:12Z


Bugzilla Link	42120
Version	trunk
OS	All
CC	@dwblaikie,@DougGregor,@zygoloid

Extended Description

Clang marks pointer conversions that add noreturn or noescape information to function types as incompatible (while removing such information is fine), but this warning is not consistent:

$ cat test.c

void fun00(int *);
void fun01(int *) __attribute__((noreturn));
void fun10(__attribute__((noescape)) int *);
void fun11(__attribute__((noescape)) int *) __attribute__((noreturn));

// OK: safe, no warning
void (*fptr01)(int *) = &fun01;
void (*fptr02)(__attribute__((noescape)) int *) = &fun11;
void (*fptr03)(int *) __attribute__((noreturn)) = &fun11;

// OK: unsafe, warning
void (*fptr11)(int *) __attribute__((noreturn)) = &fun00;
void (*fptr12)(__attribute__((noescape)) int *) = &fun00;
void (*fptr13)(__attribute__((noescape)) int *) __attribute__((noreturn)) = &fun00;

// NOT OK: also unsafe, no warning?
void (*fptr14)(__attribute__((noescape)) int *) = &fun01;
void (*fptr15)(int *) __attribute__((noreturn)) = &fun10;

$ clang -c -x c test.c
(only cases fptr11, fptr12, fptr13 are warnings; fptr14 and fptr15 pass without warning/error)

Godbolt: https://godbolt.org/z/XjFsFB

That no warnings are issued for fptr14 and fptr15 cases seems wrong. The conversions are at least as dangerous as the fptr11 and fptr12 cases, which do get warnings.

Note that C++ behavior:

$ clang -c -x c++ test.c
(cases fptr11, fptr12, fptr12, fptr13, fptr14, fptr15 are flagged as errors)

Godbolt: https://godbolt.org/z/cVjcCQ

The text was updated successfully, but these errors were encountered:

brunodefraine · 2019-06-04T08:54:58Z

This behavior is implemented in file clang/lib/Sema/SemaExpr.cpp near the end of function checkPointerTypesForAssignment:

  if (!S.getLangOpts().CPlusPlus &&
      S.IsFunctionConversion(ltrans, rtrans, ltrans))
     return Sema::IncompatiblePointer;
  return ConvTy;

This is very subtle. I think the intention is to check that the conversion from RHS to LHS is an invalid function conversion and then degrade the conversion type to IncompatiblePointer. But this is implementated by checking that the inverse conversion from LHS to RHS is valid (and LHS and RHS are not identical). This is an incomplete condition: my test case demonstrates that the inverse conversion is not necessarily valid for the conversion to be invalid.

I include a patch that replaces the call to IsFunctionConversion by a more direct check that the functions are incompatible. With this patch, also cases fptr14 and fptr15 of my test case are flagged with a warning.

brunodefraine · 2019-06-04T08:57:05Z

incompatible-func-conv.patch

dwblaikie · 2019-06-04T15:25:35Z

Usually best to send patches to llvm-commits (contributing patches discussed here: https://llvm.org/docs/Contributing.html ) - they can go a bit lost when attached to bugs.

All of our thread entry functions have this signature: void (*)(void*) __attribute__((noreturn)) The low-level __thread_create() function accepts a thread_func_t as the entry point, which is defined more simply as: void (*)(void *) And then the thread_create() and thread_create_named() macros cast the passed-in function point down to thread_func_t, that is, casting away the `noreturn` attribute. Clang considers casting between these two types to be invalid because both the caller and the callee may have elided parts of the stack frame save and restore, knowing that they won't be needed. Recent Linux appears to be setting -Wcast-function-type-strict, which causes this invalid cast to emit a warning, which with -Werror is converted to an error, breaking the build. This commit fixes this in the simplest possible way: adding `noreturn` to the `thread_func_t` attribute. Since all our thread entry functions already have this attribute, it's arguably a just a consistency fix anyway. I considered removing the casts in the macros, which silences the warnings, but it turns out that Clang has a bug that won't emit this error for implicit conversions, only explicit casts. So leaving them there seems like a reasonable belt-and-suspenders approach. Also, frankly, this whole mechanism seems a little undercooked inside LLVM, so I'm content go with my intuition about the smallest, least invaisve change. Further reading: llvm/llvm-project@1aad641 llvm/llvm-project#7325 llvm/llvm-project#41465 Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <robn@despairlabs.com>

All of our thread entry functions have this signature: void (*)(void*) __attribute__((noreturn)) The low-level __thread_create() function accepts a thread_func_t as the entry point, which is defined more simply as: void (*)(void *) And then the thread_create() and thread_create_named() macros cast the passed-in function point down to thread_func_t, that is, casting away the `noreturn` attribute. Clang considers casting between these two types to be invalid because both the caller and the callee may have elided parts of the stack frame save and restore, knowing that they won't be needed. Recent Linux appears to be setting -Wcast-function-type-strict, which causes this invalid cast to emit a warning, which with -Werror is converted to an error, breaking the build. This commit fixes this in the simplest possible way: adding `noreturn` to the `thread_func_t` attribute. Since all our thread entry functions already have this attribute, it's arguably a just a consistency fix anyway. I considered removing the casts in the macros, which silences the warnings, but it turns out that Clang has a bug that won't emit this error for implicit conversions, only explicit casts. So leaving them there seems like a reasonable belt-and-suspenders approach. Also, frankly, this whole mechanism seems a little undercooked inside LLVM, so I'm content go with my intuition about the smallest, least invaisve change. Further reading: - llvm/llvm-project@1aad641 - llvm/llvm-project#7325 - llvm/llvm-project#41465 Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <robn@despairlabs.com>

All of our thread entry functions have this signature: void (*)(void*) __attribute__((noreturn)) The low-level __thread_create() function accepts a thread_func_t as the entry point, which is defined more simply as: void (*)(void *) And then the `thread_create()` and `thread_create_named()` macros cast the passed-in function point down to `thread_func_t`, that is, casting away the `noreturn` attribute. Clang considers casting between these two types to be invalid because both the caller and the callee may have elided parts of the stack frame save and restore, knowing that they won't be needed. Recent Linux appears to be setting `-Wcast-function-type-strict`, which causes this invalid cast to emit a warning, which with `-Werror` is converted to an error, breaking the build. This commit fixes this in the simplest possible way: adding `noreturn` to the `thread_func_t` attribute. Since all our thread entry functions already have this attribute, it's arguably a just a consistency fix anyway. I considered removing the casts in the macros, which silences the warnings, but it turns out that Clang has a bug that won't emit this error for implicit conversions, only explicit casts. So leaving them there seems like a reasonable belt-and-suspenders approach. Also, frankly, this whole mechanism seems a little undercooked inside LLVM, so I'm content go with my intuition about the smallest, least invaisve change. Further reading: - llvm/llvm-project@1aad641 - llvm/llvm-project#7325 - llvm/llvm-project#41465 Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <robn@despairlabs.com>

All of our thread entry functions have this signature: void (*)(void*) __attribute__((noreturn)) The low-level `__thread_create()` function accepts a `thread_func_t` as the entry point, which is defined more simply as: void (*)(void *) And then the `thread_create()` and `thread_create_named()` macros cast the passed-in function point down to `thread_func_t`, that is, casting away the `noreturn` attribute. Clang considers casting between these two types to be invalid because both the caller and the callee may have elided parts of the stack frame save and restore, knowing that they won't be needed. Recent Linux appears to be setting `-Wcast-function-type-strict`, which causes this invalid cast to emit a warning, which with `-Werror` is converted to an error, breaking the build. This commit fixes this in the simplest possible way: adding `noreturn` to the `thread_func_t` attribute. Since all our thread entry functions already have this attribute, it's arguably a just a consistency fix anyway. I considered removing the casts in the macros, which silences the warnings, but it turns out that Clang has a bug that won't emit this error for implicit conversions, only explicit casts. So leaving them there seems like a reasonable belt-and-suspenders approach. Also, frankly, this whole mechanism seems a little undercooked inside LLVM, so I'm content go with my intuition about the smallest, least invaisve change. **NOTE**: `__thread_create` is exported by `spl.ko` and has a `thread_func_t` arg, so this is an ABI break. Whether that matters in practice, I have no idea. Further reading: - llvm/llvm-project@1aad641 - llvm/llvm-project#7325 - llvm/llvm-project#41465 Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <robn@despairlabs.com>

All of our thread entry functions have this signature: void (*)(void*) __attribute__((noreturn)) The low-level `__thread_create()` function accepts a `thread_func_t` as the entry point, which is defined more simply as: void (*)(void *) And then the `thread_create()` and `thread_create_named()` macros cast the passed-in function point down to `thread_func_t`, that is, casting away the `noreturn` attribute. Clang considers casting between these two types to be invalid because both the caller and the callee may have elided parts of the stack frame save and restore, knowing that they won't be needed. Recent Linux appears to be setting `-Wcast-function-type-strict`, which causes this invalid cast to emit a warning, which with `-Werror` is converted to an error, breaking the build. This commit fixes this in the simplest possible way: adding `noreturn` to the `thread_func_t` attribute. Since all our thread entry functions already have this attribute, it's arguably a just a consistency fix anyway. I considered removing the casts in the macros, which silences the warnings, but it turns out that Clang has a bug that won't emit this error for implicit conversions, only explicit casts. So leaving them there seems like a reasonable belt-and-suspenders approach. Also, frankly, this whole mechanism seems a little undercooked inside LLVM, so I'm content go with my intuition about the smallest, least invaisve change. **NOTE**: `__thread_create` is exported by `spl.ko` and has a `thread_func_t` arg, so this is an ABI break. Whether that matters in practice, I have no idea. Further reading: - llvm/llvm-project@1aad641 - llvm/llvm-project#7325 - llvm/llvm-project#41465 Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Rob Norris <robn@despairlabs.com> Closes #16672 Closes #16673

All of our thread entry functions have this signature: void (*)(void*) __attribute__((noreturn)) The low-level `__thread_create()` function accepts a `thread_func_t` as the entry point, which is defined more simply as: void (*)(void *) And then the `thread_create()` and `thread_create_named()` macros cast the passed-in function point down to `thread_func_t`, that is, casting away the `noreturn` attribute. Clang considers casting between these two types to be invalid because both the caller and the callee may have elided parts of the stack frame save and restore, knowing that they won't be needed. Recent Linux appears to be setting `-Wcast-function-type-strict`, which causes this invalid cast to emit a warning, which with `-Werror` is converted to an error, breaking the build. This commit fixes this in the simplest possible way: adding `noreturn` to the `thread_func_t` attribute. Since all our thread entry functions already have this attribute, it's arguably a just a consistency fix anyway. I considered removing the casts in the macros, which silences the warnings, but it turns out that Clang has a bug that won't emit this error for implicit conversions, only explicit casts. So leaving them there seems like a reasonable belt-and-suspenders approach. Also, frankly, this whole mechanism seems a little undercooked inside LLVM, so I'm content go with my intuition about the smallest, least invaisve change. **NOTE**: `__thread_create` is exported by `spl.ko` and has a `thread_func_t` arg, so this is an ABI break. Whether that matters in practice, I have no idea. Further reading: - llvm/llvm-project@1aad641 - llvm/llvm-project#7325 - llvm/llvm-project#41465 Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Rob Norris <robn@despairlabs.com> Closes openzfs#16672 Closes openzfs#16673

All of our thread entry functions have this signature: void (*)(void*) __attribute__((noreturn)) The low-level `__thread_create()` function accepts a `thread_func_t` as the entry point, which is defined more simply as: void (*)(void *) And then the `thread_create()` and `thread_create_named()` macros cast the passed-in function point down to `thread_func_t`, that is, casting away the `noreturn` attribute. Clang considers casting between these two types to be invalid because both the caller and the callee may have elided parts of the stack frame save and restore, knowing that they won't be needed. Recent Linux appears to be setting `-Wcast-function-type-strict`, which causes this invalid cast to emit a warning, which with `-Werror` is converted to an error, breaking the build. This commit fixes this in the simplest possible way: adding `noreturn` to the `thread_func_t` attribute. Since all our thread entry functions already have this attribute, it's arguably a just a consistency fix anyway. I considered removing the casts in the macros, which silences the warnings, but it turns out that Clang has a bug that won't emit this error for implicit conversions, only explicit casts. So leaving them there seems like a reasonable belt-and-suspenders approach. Also, frankly, this whole mechanism seems a little undercooked inside LLVM, so I'm content go with my intuition about the smallest, least invaisve change. **NOTE**: `__thread_create` is exported by `spl.ko` and has a `thread_func_t` arg, so this is an ABI break. Whether that matters in practice, I have no idea. Further reading: - llvm/llvm-project@1aad641 - llvm/llvm-project#7325 - llvm/llvm-project#41465 Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <robn@despairlabs.com>

All of our thread entry functions have this signature: void (*)(void*) __attribute__((noreturn)) The low-level `__thread_create()` function accepts a `thread_func_t` as the entry point, which is defined more simply as: void (*)(void *) And then the `thread_create()` and `thread_create_named()` macros cast the passed-in function point down to `thread_func_t`, that is, casting away the `noreturn` attribute. Clang considers casting between these two types to be invalid because both the caller and the callee may have elided parts of the stack frame save and restore, knowing that they won't be needed. Recent Linux appears to be setting `-Wcast-function-type-strict`, which causes this invalid cast to emit a warning, which with `-Werror` is converted to an error, breaking the build. This commit fixes this in the simplest possible way: adding `noreturn` to the `thread_func_t` attribute. Since all our thread entry functions already have this attribute, it's arguably a just a consistency fix anyway. I considered removing the casts in the macros, which silences the warnings, but it turns out that Clang has a bug that won't emit this error for implicit conversions, only explicit casts. So leaving them there seems like a reasonable belt-and-suspenders approach. Also, frankly, this whole mechanism seems a little undercooked inside LLVM, so I'm content go with my intuition about the smallest, least invaisve change. **NOTE**: `__thread_create` is exported by `spl.ko` and has a `thread_func_t` arg, so this is an ABI break. Whether that matters in practice, I have no idea. Further reading: - llvm/llvm-project@1aad641 - llvm/llvm-project#7325 - llvm/llvm-project#41465 Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Rob Norris <robn@despairlabs.com> Closes openzfs#16672 Closes openzfs#16673

All of our thread entry functions have this signature: void (*)(void*) __attribute__((noreturn)) The low-level `__thread_create()` function accepts a `thread_func_t` as the entry point, which is defined more simply as: void (*)(void *) And then the `thread_create()` and `thread_create_named()` macros cast the passed-in function point down to `thread_func_t`, that is, casting away the `noreturn` attribute. Clang considers casting between these two types to be invalid because both the caller and the callee may have elided parts of the stack frame save and restore, knowing that they won't be needed. Recent Linux appears to be setting `-Wcast-function-type-strict`, which causes this invalid cast to emit a warning, which with `-Werror` is converted to an error, breaking the build. This commit fixes this in the simplest possible way: adding `noreturn` to the `thread_func_t` attribute. Since all our thread entry functions already have this attribute, it's arguably a just a consistency fix anyway. I considered removing the casts in the macros, which silences the warnings, but it turns out that Clang has a bug that won't emit this error for implicit conversions, only explicit casts. So leaving them there seems like a reasonable belt-and-suspenders approach. Also, frankly, this whole mechanism seems a little undercooked inside LLVM, so I'm content go with my intuition about the smallest, least invaisve change. **NOTE**: `__thread_create` is exported by `spl.ko` and has a `thread_func_t` arg, so this is an ABI break. Whether that matters in practice, I have no idea. Further reading: - llvm/llvm-project@1aad641 - llvm/llvm-project#7325 - llvm/llvm-project#41465 Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <robn@despairlabs.com>

llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021

Endilll added the clang:to-be-triaged Should not be used for new issues label Jul 19, 2024

robn mentioned this issue Oct 21, 2024

spl/thread: explicitly define thread_func_t as noreturn openzfs/zfs#16673

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inconsistent (missing) incompatible function pointer type warnings #41465

Inconsistent (missing) incompatible function pointer type warnings #41465

brunodefraine commented Jun 4, 2019 •

edited by Endilll

Loading

brunodefraine commented Jun 4, 2019 •

edited by Endilll

Loading

brunodefraine commented Jun 4, 2019

dwblaikie commented Jun 4, 2019

Inconsistent (missing) incompatible function pointer type warnings #41465

Inconsistent (missing) incompatible function pointer type warnings #41465

Comments

brunodefraine commented Jun 4, 2019 • edited by Endilll Loading

Extended Description

brunodefraine commented Jun 4, 2019 • edited by Endilll Loading

brunodefraine commented Jun 4, 2019

dwblaikie commented Jun 4, 2019

brunodefraine commented Jun 4, 2019 •

edited by Endilll

Loading

brunodefraine commented Jun 4, 2019 •

edited by Endilll

Loading