#349 Bug with QEVT_DYN_CTOR and QEvt(DYNAMIC) constructor

[Anonymous]

With QEVT_DYN_CTOR being defined, use of the QEvt(DynEvt dummy) via Q_NEW invokes undefined behavior, which breaks optimized builds.

I have created a reduced example of the issue: https://godbolt.org/z/rdxE4vYGz

QEvt(DynEvt dummy) does not initialize any member variables, which leaves them uninitialized.
Reading of uninitialized data is undefined behavior in C++, thus the intended behavior, as stated in the comment is broken.

// dynamic event already initialized in QP::QF::newX_()

g++ seems to zero-initialize the members on placement new, overriding the values from QP::QF::newX_().

I have also implemented a proposed fix for the reduced example.
The idea is simply to set the member variables after doing placement new.
This implementation also has the benefit of allowing to use the QEvt(QSignal) constructor (from subclasses) for dynamically allocated events. In the current implementation this leads to leaked events filling the event pool, because the constructor clears the poolId stored in evtTag_, which is interpreted by gc as the event not being dynamically allocated.

[Quantum Leaps]

This behavior of placement new is unfortunate.

At this point, I don't know how to fix the problem such behavior causes. (The fix you propose doesn't work because it simplistically hard-codes event pool #1. But in general case, the information about the event pool is stored in the event by the function QF::newX_()). So the root cause here is wiping out the vital information by the placement new. The fact that it happens only at higher optimization level only adds insult to injury.

--MMS

[Quantum Leaps]

I checked this behavior in ARM compiler 6 (clang), and placement new does not seem to clear memory, even with higher optimization levels. Again, this is embedded target (ARM Cortex-M).
--MMS

[Quantum Leaps]

OK, I've checked in a potential fix for this bizarre problem (see qpcpp GitHub repo) The (potential) fix has two parts:

1. the placement new is overridden just for the QEvt and subclasses
2. the optimizations are suppressed around the overridden placement new.

Here is the relevant fragment from the modified qp.hpp header file:

#ifdef QEVT_DYN_CTOR

#include <cstddef> // for std::size_t

// placement new specialized for QP::QEvt and subclasses
//
// NOTE:
// It is reported that some GCC and Clang versions clear the memory passed
// to the placement new, but only at higher levels of optimization.
// Such clearing of memory wipes out the important pool-ID information
// left in the event by the QP::QF::newX_() allocator. As a workaround for
// this problem, the optimization is turned off the placement new operator.
#if defined __clang__
__attribute__((optnone))
#elif defined(__GNUC__) || defined(__GNUG__)
__attribute__((optimize("O0")))
#endif
inline void* operator new (std::size_t, QP::QEvt *where) {
    return where;
}

#endif // QEVT_DYN_CTOR

It would be great if you could test it in your project and compiler and post to this discussion thread what you find.

In my tests with MinGW (gcc/g++ 9.2.0) the code works for all optimization levels and even for the LTO.

--MMS

[Anonymous]

Thank you for looking into it.
In my reduced example your fix seems to be working: https://godbolt.org/z/6hqd4a4xc
Although I don’t like that it still relies on undefined behavior and also turns off optimizations.

The problem occurs with various versions of arm-gcc-none-eabi, including the toolchain version 13.2 from ARM for Linux.
Quickly changing the GCC version on my example revealed, that coincidentally g++ 9.3.0 is the first version to exhibit this problem.

My previously proposed fix is only for a simplified version, although I see now, that there is the necessity to share additional data (i.e. poolId) between q_newX and q_newX_init.

I adjusted the example to cover this need and added some comments for further clarification: https://godbolt.org/z/b5Wh9bhW6

[Quantum Leaps]

Yes, you are right that my proposed "fix" still relies on the UB, and I don't quite like that either.

But regarding turning off optimizations, it is only for the particular specialized placement new, and does not affect any surrounding code (which can be still optimized any whay you wish). Actually, avoiding the costly clearing of the whole memory block in placement new should be more efficient, especially for bigger memory blocks.

(On a side note, it would be very interesting to get to the bottom of it and find out why the compilers clear the whole memory block in placement new. Do they perhaps cover up some potential problems caused by optimizations?)

Now, regarding your adjusted fix, I understand that the complete solution would require an internal QP function that provides the "pool-ID" of a dynamic event, which will then be consisently used inside the framework (and inside the Q_NEW() and Q_NEW_X() macros).

Another potential solution would be to sidestep the whole messy and expensive placement new and C++ constructors altogether. Instead, dynamic QEvt subclasses could simply provide member function(s) (e.g. ctor() or init()) for initialization with possibly multiple overloaded variants, just like the overloaded ctors. This would be perhaps less "nerdy", but straightforward, free from UB, and significantly more efficient. This is how dynamic event ctors are implemented in QP/C. The use of the Q_NEW() and Q_NEW_X() would NOT need to change at all. I tend to prefer such simple, efficient soultions (KISS principle!)

What do you think?

--MMS

[Anonymous]

Maybe the compiler is not actively clearing the memory, but just optimizing away everything that is happening to the memory before the placement new.

I would prefer constructors over special methods, but it might be a reasonable solution and would be okay for me.

However, I doubt that your proposed non-constructor solution really is considerably more efficient in optimized builds.
At least, my solution with the helper object is actually very similar to the original implementation in terms of complexity or “mess”.

[Anonymous]

I have now implemented my solution for v7.3.0, however without caring about the case where QEVT_DYN_CTOR is not defined.
See attachment.

[Quantum Leaps]

Thank you for the contribution.

I agree that the Q_NEW()and Q_NEW_X() facilities should be templates rather than the old-fashion macros. Also, your solution is definitely much more modern ( "c-plus-plusy").

Maybe the compiler is not actively clearing the memory, but just optimizing away everything that is happening to the memory before the placement new.

I don't think that there is any doubt that the whole memory block is cleared in the placement new (but only at higher optimization levels, which is precisely suboptimal). I realize that this is UB, so the compiler is allowed to do such "optimizations." But frankly, such behavior is really concerning to me. (Isn't it to you?) This is a ticking bomb for a real-time system, where you need to know the CPU utilization. Yet, some compiler at some optimization level will, out of the blue, insert such a CPU hog. I'm also sure that such behavior is unacceptable for functional safety certification.

For these reasons, the recently released QP/C++ 7.3.3 stays away from the problematic placement new and uses a simple initialization function for dynamic events (when QEVT_DYN_CTOR is defined). Also, Q_NEW() is still implemented as old-fashioned macro. This might change to a template in the future, but it should be transparent to the applications.

I hope that this design decision makes sense to you, particularly if you consier the functional safety focus.

--MMS

[Anonymous]

Yes, from that point of view your solution makes more sense.
Thank you. You can close this ticket, then.

[Quantum Leaps]

Fixed in QP/C++ 7.3.3.
--MMS