Compiler warnings from -Wcast-function-type in signal.h, slot.h

[db-src]

After an update on Debian unstable, I'm getting a bunch of warnings like the below.

This might be due to a compiler update more than libsicg++ since the packages in my apt archive seem quite old:

ls /var/cache/apt/archives/*sigc*

-rw-r--r-- 1 root root  61244 Mar 14 23:10 /var/cache/apt/archives/libsigc++-2.0-0v5_2.10.0-2_amd64.deb
-rw-r--r-- 1 root root 114968 Mar 14 23:10 /var/cache/apt/archives/libsigc++-2.0-dev_2.10.0-2_amd64.deb

the warnings (not necessarily all of them, but you get the idea):

/usr/include/sigc++-2.0/sigc++/signal.h: In static member function ‘static sigc::internal::signal_emit0<void, sigc::nil>::result_type sigc::internal::signal_emit0<void, sigc::nil>::emit(sigc::internal::signal_impl*)’:
/usr/include/sigc++-2.0/sigc++/signal.h:798:56: warning: cast between incompatible function types from ‘sigc::internal::hook’ {aka ‘void* (*)(void*)’} to ‘sigc::internal::signal_emit0<void, sigc::nil>::call_type’ {aka ‘void (*)(sigc::internal::slot_rep*)’} [-Wcast-function-type]
           (reinterpret_cast<call_type>(slot.rep_->call_))(slot.rep_);
                                                        ^
/usr/include/sigc++-2.0/sigc++/signal.h: In static member function ‘static sigc::internal::signal_emit0<void, sigc::nil>::result_type sigc::internal::signal_emit0<void, sigc::nil>::emit_reverse(sigc::internal::signal_impl*)’:
/usr/include/sigc++-2.0/sigc++/signal.h:825:55: warning: cast between incompatible function types from ‘sigc::internal::hook’ {aka ‘void* (*)(void*)’} to ‘sigc::internal::signal_emit0<void, sigc::nil>::call_type’ {aka ‘void (*)(sigc::internal::slot_rep*)’} [-Wcast-function-type]
           (reinterpret_cast<call_type>(it->rep_->call_))(it->rep_);

/usr/include/sigc++-2.0/sigc++/functors/slot.h:1752:26:   required from ‘sigc::slot<T_return, sigc::nil, sigc::nil, sigc::nil, sigc::nil, sigc::nil, sigc::nil, sigc::nil>::slot(const T_functor&) [with T_functor = djb::Control_Toggle_Two::Control_Toggle_Two(const Glib::ustring&, const Glib::ustring&, Gtk::Orientation)::<lambda()>; T_return = void]’
../../../home/daniel/src/hx1ed/modules/djb/gui/Control_Toggle_Two.cpp:34:4:   required from here
/usr/include/sigc++-2.0/sigc++/functors/slot.h:121:14: warning: cast between incompatible function types from ‘void (*)(sigc::internal::slot_rep*)’ to ‘sigc::internal::hook’ {aka ‘void* (*)(void*)’} [-Wcast-function-type]
     { return reinterpret_cast<hook>(&call_it); }

I don't know if these are solvable, but hopefully; I don't like having to just hide warnings when compiling my projects. However, I grimly suspect I might have to do so, since ultimately GLib and therefore perhaps glibmm as a consequence rely on doing naughty things with casts, which probably can't be avoided once we go deep enough. :P

[db-src]

Here's a discussion on a possible way to get around this, in a way that seems to satisfy gcc and the standard: https://bugs.python.org/issue33012#msg317823

and from the gcc docs:
https://gcc.gnu.org/onlinedocs/gcc-8.1.0/gcc/Warning-Options.html#index-Wcast-function-type

The function type void (*) (void) is special and matches everything, which can be used to suppress this warning. In a cast involving pointer to member types this warning warns whenever the type cast is changing the pointer to member type. This warning is enabled by -Wextra.

[kjellahl]

This is a duplicate of issue #1. It has been fixed in the master branch and in
the libsigc++-2-10 branch. Similar type casts have been fixed in the same way
in glibmm, master branch and glibmm-2-56 branch.

The fix is to call this template function instead of reinterpret_cast<>():

template <typename out_type, typename in_type>
inline out_type bitwise_equivalent_cast(in_type in)
{
  union {
    in_type in;
    out_type out;
  } u;
  u.in = in;
  return u.out;
}

There is not yet a tarball that includes this fix.

[db-src]

Yiiikes. How is that valid C++? Unions cannot be used for typecasting/reinterpretation in C++, only C. Doing that in C++ is undefined behaviour from everything I've read.

(I don't think compilers are capricious enough to blow up in this case; I have a vague recollection that this might even be a documented extension in g++, but that doesn't mean it's good code to write. :P)

[kjellahl]

I must confess that I didn't know that it's illegal. A union has been used for
casting from a function pointer to a data pointer (void*) in Glib::OptionGroup
since February 2011. As far as I know, no compiler has complained. (This is an ugly
cast that glib forces us to do. Previously that cast was done with reinterpret_cast,
but gcc 4 did not like that.)

[db-src]

UB doesn't require a diagnostic; that's the main problem with thinking that the lack of an error means things are OK. :) Pure speculation, but GCC might also eschew the warning because this is legal in C and/or (as I said, IIRC) it allows it as a documented extension in C++ mode too.

Won't the alternative method that I linked to solve this, while keeping the Standard happy too? (I didn't read too far into it yet though.) It may or may not be related to what you saw in GCC 4, but that's very old, so it might not reflect current compilers anyway.

[kjellahl]

In the master branch and the libsigc++-2-10 branch I have changed to

template <typename T_out, typename T_in>
inline T_out function_pointer_cast(T_in in)
{
  // The double reinterpret_cast suppresses a warning from gcc8 with the
  // -Wcast-function-type option.
  return reinterpret_cast<T_out>(reinterpret_cast<void (*)()>(in));
}

Do you care about glibmm? I'm not sure any combination of reinterpret_cast
will convert from a function pointer to a data pointer without compiler warnings.
Have you got an idea what to do there, if a union is unacceptable?

[db-src]

Thanks!

I care inasmuch as I would ideally like all libraries I use to be 100% standard-compliant, but I'm not holding out much hope for that where we start getting closer to GLib et al. :P I appreciate you considering these boring legalese points in any case.

As for how to do it, I have 2 thoughts right now, but these may not be comprehensive or optimal:

(1) An alternative to casts/reinterpretation, which as far as I know is allowed for all trivially copyable types, is to memcpy the bytes into a new instance of the destination type. That often gets optimised to the same thing as any cast while keeping the Standard happier. The guarantee here is pretty much what you get with allowed casts: you can 'cast' the Thing to Something Else and eventually back to a Thing that is still usable. I get the impression this is pretty much what to use when casts don't look legal. I haven't read about this with specific attention to data/function pointer conversion, but those should both be trivially copyable types, surely? But we would want to double-check whether existing discussions cover this (and, if it's viable, to assert that the intermediate type has sufficient sizeof).

(2) Secondarily, it seems that since C++11, converting between data and function pointers is "conditionally supported", and implementation-defined when it is: 1, 2 This may be better in some philosophical sense than the union 'trick', but it at least depends on the targeted compilers supporting this formally - if clearly they do in practice. But I don't feel implementation-defined behaviour is much better than UB, at least for cross-platform libraries (more for very specific/embedded systems).

[kjellahl]

(1) memcpy() is no solution here. It takes void* parameters, i.e. data pointers.
A call to memcpy() requires a conversion from function pointer to data pointer
outside memcpy().

(2) I've made a few tests with gcc 8. It accepts reinterpret_cast<void*>(a_function_pointer)
except when the -Wconditionally-supported option is used.

I'll probably keep using a union for that particular conversion. It has worked
for 7 years. Of course it may stop working with some future compiler, but I've
a feeling that in this case anything may.

Bjarne Stroustrup (The C++ Programming Language, 4th edition, section 8.3)
shows an example of using a union for data conversion. He says that it's bad use,
dangerous and nonportable, but he does not say that it's illegal.

[db-src]

But you'd be passing memcpy the addresses of pointers, not the original pointers themselves. Is a void* allowed to point to a function pointer? I honestly don't know. Maybe it's no better for that reason.

"illegal" isn't really a useful term when dissecting Standardese, IME. "undefined behaviour" is more relevant and inferable: the Standard doesn't state that union 'casting' should work, in fact quite the opposite since only one union member is theoretically alive at any one time, and merely reading does not change that (theoretical) fact. "implementation-defined behaviour" is better than that, at least. Anyway, the qualifiers Bjarne used, even if they exclude "undefined" or "illegal", should be hint enough!

Just because gcc 8 never complains about the union trick does not make it good style; it just means that it's UB and therefore does not require a compiler diagnostic, and gcc does not issue one. So saying that the alternative of reinterpret_cast doesn't work under -Wconditionally-supported is (A) obvious from what C++11 says about that cast and (B) no indication that this is the worse of 2 subpar options, only that it requires compiler documentation and diagnostic (good) when the other does not.

However, I won't harp on about this for much longer if my points aren't landing. To some extent, the ship of fully Standard-compliant code sailed when we started using GLib! Other infractions, especially widely relied-upon ones, aren't practically likely to make anything fail. It'd just be nice to avoid adding.

[kjellahl]

Back in 2011 I added the code that required a conversion from a function pointer
to a data pointer. In those days (before we started using C++11) it was not possible
to use reinterpret_cast<void*>(a_function_pointer) without a compiler warning
(treated as an error) from gcc with the compiler options we often use.
(See https://bugzilla.gnome.org/show_bug.cgi?id=589197) That's why I've been reluctant
to use it now. With C++11 and conditionally supported conversions, the situation
has changed. So, after all, I've pushed code that uses reinterpret_cast instead of
union in Glib::OptionGroup. But if this will lead to new problems with compiler
warnings, it's not impossible that I change back to union.

I find it a bit strange that gcc with the -pedantic -Wall -Wextra -Werror options
silently accepts a conversion from a function pointer to a void*, but flags
a conversion between two types of function pointers as an error (with one exception).

[db-src]

Yeah, that does sound odd and perhaps worth a gcc bug if one does not already exist (though I know none of us like filing those :) )