minimal compile time check for wrong signal handler return type

[dterrahe]

EDIT: This is now a compile time check (so compilation will intentionally break if a wrong callback signature is used).
Uses of g_signal_connect that cannot be checked (for example because they get passed the callback in a variable) or that need to return the connected signal id are changed to use g_signal_connect_data instead. g_signal_connect_after and g_signal_connect_swapped are not covered by this check.

All issues that caused this check to fail have been fixed in the second commit.

---

A minimal version of #19818 which just checks the return type when connecting signal handlers. There's a compile-time part (matching the actual function against void/gboolean(*)() ) and a run-time part (looking up what the signal actually wants). So the overhead is an extra indirection before calling g_signal_connect_data and looking up the signal id and querying it. Which should be fairly minimal but could be made debug build only.

The only source changes required are to consistently wrap the c_handler in G_CALLBACK rather than casting it with (GCallback) (because the macro disables the cast by attaching a prefix). So unlike #19818 here it is fine to keep the instance wrapped in G_OBJECT or whatever. And the check in g_signal_connect is skipped for helper functions (like dt_iop_button_new) where we don't know what the real handler signature is.

@jenshannoschwalm Some of the issues this catches are already addressed in the first commit of the other PR but I didn't track the fixes down or added them here because I wanted to keep this minimal. However, they are all bugs (unless someone can argue when this would generate false positives?) so might still qualify for 5.4 @TurboGit ? If not, and if everybody is OK with the style changes in #19818/1 then merging them first and then fixing the remainder highlighted by this would be less work.

[dterrahe]

I'm getting these btw

     1.5892 connecting signal value-changed to handler with wrong return type /home/diederik/darktable/src/gui/gtk.c:1464
     1.5893 connecting signal value-changed to handler with wrong return type /home/diederik/darktable/src/gui/gtk.c:1468
     2.2737 connecting signal button-press-event to handler with wrong return type /home/diederik/darktable/src/develop/blend_gui.c:3480
     2.3084 connecting signal button-release-event to handler with wrong return type /home/diederik/darktable/src/iop/temperature.c:2149
     2.8698 connecting signal size_allocate to handler with wrong return type /home/diederik/darktable/src/iop/colorequal.c:3037
     3.1415 connecting signal draw to handler with wrong return type /home/diederik/darktable/src/iop/overlay.c:1085
     3.4851 connecting signal button-press-event to handler with wrong return type /home/diederik/darktable/src/iop/channelmixerrgb.c:4697
     3.4852 connecting signal button-press-event to handler with wrong return type /home/diederik/darktable/src/iop/channelmixerrgb.c:4705
     3.4852 connecting signal button-press-event to handler with wrong return type /home/diederik/darktable/src/iop/channelmixerrgb.c:4712
     3.6198 connecting signal button-release-event to handler with wrong return type /home/diederik/darktable/src/iop/colorbalance.c:1901
     3.7706 connecting signal button-press-event to handler with wrong return type /home/diederik/darktable/src/libs/masks.c:1798
     3.7707 connecting signal button-press-event to handler with wrong return type /home/diederik/darktable/src/libs/masks.c:1806
     3.7707 connecting signal button-press-event to handler with wrong return type /home/diederik/darktable/src/libs/masks.c:1814
     3.7708 connecting signal button-press-event to handler with wrong return type /home/diederik/darktable/src/libs/masks.c:1822
     3.7708 connecting signal button-press-event to handler with wrong return type /home/diederik/darktable/src/libs/masks.c:1830
     3.7787 connecting signal enter-notify-event to handler with wrong return type /home/diederik/darktable/src/libs/tools/viewswitcher.c:266
     3.7787 connecting signal leave-notify-event to handler with wrong return type /home/diederik/darktable/src/libs/tools/viewswitcher.c:267
     3.7788 connecting signal enter-notify-event to handler with wrong return type /home/diederik/darktable/src/libs/tools/viewswitcher.c:266
     3.7788 connecting signal leave-notify-event to handler with wrong return type /home/diederik/darktable/src/libs/tools/viewswitcher.c:267
     3.7989 connecting signal button-press-event to handler with wrong return type /home/diederik/darktable/src/libs/tools/colorlabels.c:141
     3.7991 connecting signal button-press-event to handler with wrong return type /home/diederik/darktable/src/libs/tools/colorlabels.c:141
     3.7991 connecting signal button-press-event to handler with wrong return type /home/diederik/darktable/src/libs/tools/colorlabels.c:141
     3.7992 connecting signal button-press-event to handler with wrong return type /home/diederik/darktable/src/libs/tools/colorlabels.c:141
     3.7992 connecting signal button-press-event to handler with wrong return type /home/diederik/darktable/src/libs/tools/colorlabels.c:141
     3.7993 connecting signal button-press-event to handler with wrong return type /home/diederik/darktable/src/libs/tools/colorlabels.c:141
     4.1984 connecting signal changed to handler with wrong return type /home/diederik/darktable/src/libs/styles.c:861
     4.1985 connecting signal activate to handler with wrong return type /home/diederik/darktable/src/libs/styles.c:862
     4.1988 connecting signal toggled to handler with wrong return type /home/diederik/darktable/src/libs/styles.c:869
     4.2182 connecting signal button-press-event to handler with wrong return type /home/diederik/darktable/src/libs/export.c:1733
     4.2183 connecting signal button-press-event to handler with wrong return type /home/diederik/darktable/src/libs/export.c:1735
     4.2183 connecting signal button-press-event to handler with wrong return type /home/diederik/darktable/src/libs/export.c:1737
     4.2183 connecting signal button-press-event to handler with wrong return type /home/diederik/darktable/src/libs/export.c:1739```

[TurboGit]

I'll stay on the safe side... And will register this for 5.6.

[dterrahe]

Sure. This is a developer diagnostic tool. It can be used locally without needing to be merged upstream.

What I meant is that you might still consider applying fixes for the cases of wrong return types in 5.4 (i.e. for the list I pasted above). We've seen this kind of thing causing crashes on intel mac. I can't remember if that was when bool expected and void returned or vice versa. @zisoft ? The above list seems to contain both instances. Would you be able to do some hammer testing on the functionality involved and see if you can trigger disaster? Remember that this is just the list generated at startup. Opening dialogs or menus could expose more.

BTW I admit when "fixing" the issues (in the other PR) I may not have been very careful. The general rule is that for button-press/release/scroll/key events, you return TRUE if you've handled it. But for motion/draw/enter/leave you always return FALSE because if there are further handlers set up, you generally don't want to block them. There might also be cases where a "button-press" signal is used when a "pressed" would have been simpler, i.e. not needing modifiers or double clicks and not combined with release or propagating to higher level; if not returning bool correctly all of this might have been working correctly by accident. This needs careful examination though; mindlessly replacing can cause breakage.

I now found a compile time check that works even on gcc. The only thing it doesn't do is catch situations where the signal name is "misspelled" (i.e. button_press_event instead of button-press-event) and the return type is wrong at the same time.

Once all positives are fixed, I suggest this is the one to be merged. There is no run-time cost at all and it should catch everything without having to open dialogs/menus etc.

#if !defined(__cplusplus)
#undef G_CALLBACK
static inline GCallback G_CALLBACK(void *f) { return (GCallback)f; } // as a macro it gets expanded before reaching here
#define DISABLINGPREFIXG_CALLBACK
#define BOOLSIGNAL(s, signal) || !strcmp(s, #signal)
#undef _Static_assert
#undef  g_signal_connect
#define g_signal_connect(instance, signal, c_handler, user_data) do { \
  _Static_assert((FALSE \
    BOOLSIGNAL(signal, button-press-event) \
    BOOLSIGNAL(signal, button-release-event) \
    BOOLSIGNAL(signal, motion-notify-event) \
    BOOLSIGNAL(signal, enter-notify-event) \
    BOOLSIGNAL(signal, leave-notify-event) \
    BOOLSIGNAL(signal, key-press-event) \
    BOOLSIGNAL(signal, configure-event) \
    BOOLSIGNAL(signal, focus-out-event) \
    BOOLSIGNAL(signal, focus-in-event) \
    BOOLSIGNAL(signal, scroll-event) \
    BOOLSIGNAL(signal, delete-event) \
    BOOLSIGNAL(signal, drag-motion) \
    BOOLSIGNAL(signal, drag-failed) \
    BOOLSIGNAL(signal, drag-drop) \
    BOOLSIGNAL(signal, event) \
    BOOLSIGNAL(signal, focus) \
    BOOLSIGNAL(signal, draw) \
    BOOLSIGNAL(signal, popup-menu) \
    BOOLSIGNAL(signal, query-tooltip) \
    BOOLSIGNAL(signal, match-selected) \
    ) == _Generic((DISABLINGPREFIX##c_handler), gboolean(*)(): TRUE, default: FALSE), \
    "signal " signal " return type does not match specified handler " #c_handler); \
  g_signal_connect_data((instance), (signal), (GCallback)(c_handler), (user_data), NULL, (GConnectFlags) 0); } while(0)
#endif // __cplusplus

[zisoft]

We've seen this kind of thing causing crashes on intel mac. I can't remember if that was when bool expected and void returned or vice versa. @zisoft ?

bool expected, void returned: #19338 (comment)

No crash on Intel Mac, just a misbehavior.