Bug 374568

I would guess that the HackedMenu hack has the same intention as what you are doing. Have you tried this patch extensively, run it through valgrind etc?

I must say it surprises me a bit that you are catching bugs no one has ever run into (I for one have been using QtCurve for years now).

I was using QtCurve for many years too, but now I check my apps using GCC's sanitizer. This is the reason, I believe. The first problem (memcmp()) was manifesting itself when client code was compiled with the address sanitizer, but then I tried to compile QtCurve itself with it, and this one appeared. I'm using this change for a day only; without it, every touch of any app menu lead to a crash, while now it does not. Did not try Valgrind yet.

As you noted, the backtrace is strange and almost suggests that the sanitiser itself is causing side-effects. It seems highly unlikely that a Qt event would cause a jump directly into config_file.cpp's loadImage()!
Wouldn't be the first time that a measurement instrument changes the measured observables (or that a bug jumps elsewhere when you try to look at it).

I don't see how the original hack can be bad, either. It just subclasses QMenuBar with an additional wrapper to be able to call QMenuBar::event(). Maybe it's related to using a C-style case, or a struct instead of a class HackedMenu but the main difference there should just be that all "struct members" are public, no? But you could try with `class HackedMenu` too, and with dynamic_cast<HackedMenu*>(menu)->send(event).

Not that I'm against simplifying the code as you propose, but I'd like to be able to understand why the sanitiser trips over it. What does that sanitiser do anyway,

That patch looks correct.

IIRC that hack was probably needed since the method was protected. (maybe on qt4? My internet connection is too terrible before Jan. 10 for me to do a detail check)

Just curious, why would the old version not work? Is it accidentally defining a virtual method?

The method is still protected in Qt5 from what I saw. 

The old version doesn't work with GCC 6's address sanitiser active. If it just overrides an existing virtual method there should probably be side-effects even without the sanitiser. Maybe run it through cppcheck and clang's analyser?

Update: even when QtCurve is compiled without sanitizer, it crashes. QObject::event() is public in Qt 4.
The following does not crash:

struct HackedMenu: public QMenuBar {
     using QMenuBar::event;
};
 
if (((HackEvent*)event)->adjust()) {
   ((HackedMenu*)menu)->event(event);
   return true;
}
return false;

And Qt 4 version does not crash.

(In reply to Eugene Shalygin from comment #6)
> Update: even when QtCurve is compiled without sanitizer, it crashes.

You must have hit a border condition. Please try to debug and obtain a proper backtrace. Ideally set a breakpoint in HackedMenu::send() or just before calling it, and then step through the code.

(In reply to RJVB from comment #7)

> You must have hit a border condition.

I disagree. AFAIK, the code shall lead to UB, because we call a virtual function from an object, whose constuctor was not called (HackedMenu). This is what I meant earlier when said that I can't understand how this could work. In support of this version, if I call the method explicitly specifying the class (QMenuBar::event(ev)) it does not crash and under debugger I see that the function is actually called. In the original code, the execution jumps to random location, which I can change by moving HackedMenu declaration around (e.g. moving it out of the function to an anonymous namespace or making it public class). 

> Please try to debug and obtain a proper backtrace.

As such I think the backtrace is correct. It does differ from our expectations, but this is because of UB.

I just can't understand why does it work in Qt 4 version...

(In reply to Eugene Shalygin from comment #8)
> if I call the method explicitly specifying the class (QMenuBar::event(ev)) it
> does not crash and under debugger I see

Specifying either by using directive or writing QMenuBar::event(ev) in HackedMenu::send(). I.e. when I call non-virtual function, the code works.

I agree the code is likely UB. Still curious why it only crashes on your setup and what code the compiler actually generate. When the method is protected, would the following work?

```
struct HackedMenu: public QMenuBar {
static void send(QMenuBar *menubar, QMouseEvent *event)
{
    menubar->event(event);
}
};
 
if (((HackEvent*)event)->adjust()) {
   HackedMenu::send(menu, event);
   return true;
}
```

This should not be UB (I hope?) and does not have any of the casts.

(In reply to Yichao Yu from comment #10)

> I agree the code is likely UB. Still curious why it only crashes on your
> setup and what code the compiler actually generate. When the method is

If UB = undefined behaviour in this context then it wouldn't be the first time a change in GCC made things go weird that always worked. Then again, there's only 1 way to be certain about it (and that it's not a regression in the compiler): ask on the gcc ML or bugzilla.
But isn't cppcheck supposed to catch known cases of undefined behaviour and doesn't gcc itself have an option to warn about them?

It still is surprising though that this code has been in use for years on many different platforms and with different compilers (Mac and Linux, gcc and clang on both in my case), without ever causing trouble. Usually that kind of wide-spread application tends to bring out the very subtle bugs.

> protected, would the following work?

> struct HackedMenu: public QMenuBar {
> static void send(QMenuBar *menubar, QMouseEvent *event)

That's no longer really a HackedMenu, just a WrappedMenu ;)

Eugene : do you use any other unusual compiler options, a different assembler or linker? Do you see the issue with -O0?

FWIW, on Linux I use -Ofast -flto with gcc "6.2.0-3ubuntu11~14.04".

I've been running qtcurve_utils.cpp through a few analysers. Cppcheck doesn't complain about anything, nor does the clang 3.9 static and coverage analyser (but I never used the latter before so may have misused it).

Clazy however finds this, with level3 (all relevant and related messages):

qtcq5-lnx-work/QtCurve-1.8.18/qt5/style/qtcurve.h:62:1: warning: 
      QtCurve::Style should take QObject parent argument in CTOR [-Wclazy-ctor-missing-parent-argument]
class Style: public QCommonStyle {
^
>>> should it? <<<

qtcq5-lnx-work/QtCurve-1.8.18/qt5/style/qtcurve_utils.cpp:226:5: warning: Polymorphic class is copyable. Potential
      slicing. [-Wclazy-copyable-polymorphic]
    struct HackEvent: public QMouseEvent {
    ^
qtcq5-lnx-work/QtCurve-1.8.18/qt5/style/qtcurve_utils.cpp:241:5: warning: HackedMenu should take QWidget parent
      argument in CTOR [-Wclazy-ctor-missing-parent-argument]
    struct HackedMenu: public QMenuBar {
    ^
qtcq5-lnx-work/QtCurve-1.8.18/qt5/style/qtcurve_utils.cpp:241:5: warning: HackedMenu is missing a Q_OBJECT macro
      [-Wclazy-missing-qobject-macro]


The missing Q_OBJECT macro might explain why casts to HackedMenu can go subtly wrong.

(In reply to Yichao Yu from comment #10)

> setup and what code the compiler actually generate. When the method is
> protected, would the following work?

Nice try, but no:

qtcq5-lnx-work/QtCurve-1.8.18/qt5/style/qtcurve_utils.cpp:244:22: error: 'event' is a protected member of 'QMenuBar'
            menubar->event(event);
                     ^
/opt/local/libexec/qt5/Library/Frameworks/QtWidgets.framework/Headers/qmenubar.h:133:10: note: can only access this member on an object of type 'HackedMenu'

I was wrong about Qt 4 case. There was a mistake in my setup and Qt4 style.so was from release build. Then I found that the problem shows up in Qt4 as well and seems to be disappearing when inlining is active, so it present in debug builds only. That is why, probably, regular users (-02 and similar) were not affected.

https://godbolt.org/g/tq82MX shows that with inlining GCC generates direct call to QMenuBar::event().

So that's why we don't see it!
And so in optimised builds there is in fact no bug in the compiled code.

If you have the time and opportunity, could you check if you get the crash with other compilers and -O0 (GCC versions and clang)? It strikes me as really odd that a compiler won't warn about a situation for which it can generate correct code when optimising but not (intentionally?) without optimisation.

FWIW, Gcc has `-fisolate-erroneous-path-*`, -fsanitize-undefined-trap-on-error (maybe you already used that?) and -Wsequence-point that are all related to catching undefined behaviour.

And FWIW2: rebuilding just qtcurve_utils.cpp with -O0 and clang on Mac doesn't cause any crashing.

Output with -fsanitize=address -fsanitize=undefined -fsanitize=vptr

/usr/bin> /usr/bin/pictorialist
.cmake_utils_base/cmake_c_macros/include_fix/qtcurve-utils/utils.h:110:37: runtime error: load of value 268435456, which is not a valid value for type 'StateFlag'
/home/eugene/develop/KDE/live/qtcurve/qt5/style/qtcurve_utils.cpp:249:22: runtime error: downcast of address 0x7ffc16b265d0 which does not point to an object of type 'HackEvent'
0x7ffc16b265d0: note: object is of type 'QMouseEvent'
 00 00 36 40  78 2e 46 95 80 7f 00 00  00 00 00 00 00 00 00 00  05 00 06 00 00 00 00 00  ae 1a 79 08
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'QMouseEvent'
/home/eugene/develop/KDE/live/qtcurve/qt5/style/qtcurve_utils.cpp:249:36: runtime error: member call on address 0x7ffc16b265d0 which does not point to an object of type 'HackEvent'
0x7ffc16b265d0: note: object is of type 'QMouseEvent'
 00 00 36 40  78 2e 46 95 80 7f 00 00  00 00 00 00 00 00 00 00  05 00 06 00 00 00 00 00  ae 1a 79 08
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'QMouseEvent'
/home/eugene/develop/KDE/live/qtcurve/qt5/style/qtcurve_utils.cpp:250:23: runtime error: downcast of address 0x604000099410 which does not point to an object of type 'HackedMenu'
0x604000099410: note: object is of type 'QMenuBar'
 1a 00 80 04  e0 44 a9 95 80 7f 00 00  80 51 00 00 60 61 00 00  90 46 a9 95 80 7f 00 00  00 00 be be
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'QMenuBar'
/home/eugene/develop/KDE/live/qtcurve/qt5/style/qtcurve_utils.cpp:250:34: runtime error: member call on address 0x604000099410 which does not point to an object of type 'HackedMenu'
0x604000099410: note: object is of type 'QMenuBar'
 1a 00 80 04  e0 44 a9 95 80 7f 00 00  80 51 00 00 60 61 00 00  90 46 a9 95 80 7f 00 00  00 00 be be
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'QMenuBar'
/home/eugene/develop/KDE/live/qtcurve/qt5/style/qtcurve_utils.cpp:245:22: runtime error: execution reached a __builtin_unreachable() call
*** Failure: Exit code 1 ***

clang builds do not crash here too.

GCC 5.4.0 does not crash too, although -fsanitize=vptr prints out the same errors as the version 6.3.0 does.

Anyway, since the hack is not needed, let's replace it with a proper cast and QObject::event() call?

I have nothing against your initial explicit cast to a QObject*, what about you Yichao?

I do think this has to be reported to GCC though. I think the vptr warning is more a bad-semantics-practice warning than one about something that could cause a crash like you're seeing.

Although: I don't know about C-style casting to a different class, but casting to a *non packed* struct might let you run into issues caused by different alignment. 
And *that* could explain the jump into the unknown.

Anyway, bug or feature, in the end this is probably something that only people intimately familiar with GCC's internals might be able to explain.

Re: alignment: you could compare the function addresses, i.e.

    event in HackedMenu::send() (printing that will also teach us which function call takes the hyperjump)
vs.
    static_cast<QObject*>(menu)->event

> I have nothing against your initial explicit cast to a QObject*, what about you Yichao?

Sounds good to me too.

Git commit 79f9e0a81a180b820abe2e4a2fbb21f8e1ff5592 by R.J.V. Bertin.
Committed on 08/01/2017 at 16:09.
Pushed by rjvbb into branch 'master'.

Use a cast-to-QObject rather than a "HackedMenu hack"

Call QMenuBar::event() by casting to a QObject rather
than to a wrapper class.

M  +1    -9    qt4/style/qtcurve.cpp
M  +1    -9    qt5/style/qtcurve_utils.cpp

https://commits.kde.org/qtcurve/79f9e0a81a180b820abe2e4a2fbb21f8e1ff5592

Thanks. I will prepare a MWE and if an answer will not be found by other means, will ask in GCC mailing list.