[Bug tree-optimization/104475] [12/13 Regression] Wstringop-overflow + atomics incorrect warning on dynamic object
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104475 --- Comment #25 from Jason Merrill --- This was clarified for C++ last year by http://wg21.link/cwg2535 I notice that C warn_for_null_address also warns about >mem; I don't know where that is justified in the C standard.
[Bug tree-optimization/104475] [12/13 Regression] Wstringop-overflow + atomics incorrect warning on dynamic object
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104475 Richard Biener changed: What|Removed |Added URL||https://gcc.gnu.org/piperma ||il/gcc-patches/2022-Decembe ||r/608077.html --- Comment #24 from Richard Biener --- Here's a patch doing that (ADDR_NONZERO) which fixes the issue.
[Bug tree-optimization/104475] [12/13 Regression] Wstringop-overflow + atomics incorrect warning on dynamic object
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104475
--- Comment #23 from Richard Biener ---
Interestingly doing
void QFutureInterfaceBase::setThrottled(bool enable)
{
if (>m_mutex == nullptr)
__builtin_unreachable ();
QMutexLocker lock(>m_mutex);
will avoid the diagnostic but instead complains
qfutureinterface.cpp:67:21:warning: the address of
'QFutureInterfaceBasePrivate::m_mutex' will never be NULL [-Waddress]
(that's without the re-ordering). So the C++ frontend already knows this
but doesn't transfer it to the middle-end. Maybe we want something
like ADDR_EXPR_NONZERO on the built ADDR_EXPR?
[Bug tree-optimization/104475] [12/13 Regression] Wstringop-overflow + atomics incorrect warning on dynamic object
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104475 --- Comment #22 from Richard Biener --- Re-ordering m_mutex and continuationMutex avoids the bogus diagnostic.
[Bug tree-optimization/104475] [12/13 Regression] Wstringop-overflow + atomics incorrect warning on dynamic object
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104475 --- Comment #21 from Richard Biener --- When the C++ frontend sees QMutexLocker lock(>m_mutex); it could hint the middle-end via emitting [[assume (>m_mutex != nullptr)]] QMutexLocker lock(>m_mutex); instead.
[Bug tree-optimization/104475] [12/13 Regression] Wstringop-overflow + atomics incorrect warning on dynamic object
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104475
Richard Biener changed:
What|Removed |Added
CC||jason at gcc dot gnu.org
--- Comment #20 from Richard Biener ---
(In reply to Thiago Macieira from comment #19)
> (In reply to Richard Biener from comment #15)
> > Thanks, it's still the same reason - we isolate a nullptr case and end up
> > with
> >
> > __atomic_or_fetch_4 (184B, 64, 0); [tail call]
> >
> > The path we isolate is d->m_mutex == nullptr && !enable in
> >
> > void QFutureInterfaceBase::setThrottled(bool enable)
> > {
> > QMutexLocker lock(>m_mutex);
>
> Thank you for the analysis, Richard. But do note that it's >m_mutex, not
> d->m_mutex that is passed to the locker. C++ says that if you do d-> then d
> != nullptr, so >m_mutex can't be nullptr either.
Hmm, I see
[local count: 1073741824]:
_1 = this_10(D)->d;
_2 = &_1->m_mutex;
MEM[(struct __as_base &)] ={v} {CLOBBER};
if (_2 != 0B)
so we load the pointer 'this_10(D)->d' and then indeed check >m_mutex
for being NULL.
The middle-end long allowed >x for the sake of legacy code
implementing offsetof by pointer arithmetic on an object at NULL.
Given
struct X { int a; int b ; };
int foo (struct X *x)
{
return >a != (int *)0;
}
we do indeed not simplify this. Doing >b != (int *)0 shows even that
is only simplified by recent code in Ranger (via DOM at -O1 or VRP at -O2).
That's also true for the
struct X { int a; int b ; };
int foo (struct X *x)
{
int *p = (int *)x + 1;
return p != (int *)0;
}
variant. Special-casing this in the language frontends probably won't
fix the testcase since the use in a nullptr comparison is hidden
via a function call of the CTOR:
QMutexLocker lock(>m_mutex);
and as you say m_mutex is at offset zero.
Since consider pointer overflow undefined we should be able to optimize
>b != nullptr in the middle-end earlier and more consistently. The
offset zero case is harder since we consider >b as just pointer
arithmetic and x + 0 != nullptr cannot be optimized I think.
[Bug tree-optimization/104475] [12/13 Regression] Wstringop-overflow + atomics incorrect warning on dynamic object
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104475
--- Comment #19 from Thiago Macieira ---
(In reply to Richard Biener from comment #15)
> Thanks, it's still the same reason - we isolate a nullptr case and end up
> with
>
> __atomic_or_fetch_4 (184B, 64, 0); [tail call]
>
> The path we isolate is d->m_mutex == nullptr && !enable in
>
> void QFutureInterfaceBase::setThrottled(bool enable)
> {
> QMutexLocker lock(>m_mutex);
Thank you for the analysis, Richard. But do note that it's >m_mutex, not
d->m_mutex that is passed to the locker. C++ says that if you do d-> then d !=
nullptr, so >m_mutex can't be nullptr either.
However, I guess GCC thinks it can be because the offset of m_mutex in QFIBP is
zero. pahole says:
public:
void QFutureInterfaceBasePrivate(class QFutureInterfaceBasePrivate *,
enum State);
void ~QFutureInterfaceBasePrivate(class QFutureInterfaceBasePrivate *,
int);
class QMutex m_mutex; /* 0 8 */
class QBasicMutex continuationMutex; /* 8 8 */
So there's a missed optimisation here. But it doesn't look like GCC is the only
one to miss it, see https://gcc.godbolt.org/z/WW5hbW6sW. Maybe it's an
intentional choice?
> we predict the path to be unlikely but the adjustment to the threader
> covered probably never executed paths (with probability zero). The
> threading opportunity arises because the DTOR calls
>
> inline void unlock() noexcept
> {
> if (!isLocked)
> return;
> m->unlock();
> isLocked = false;
> }
>
> and we know isLocked on the nullptr path.
We know it can't be true.
> I thought we could maybe enhance prediction to look for nullptr based
> accesses but at the time we estimate probabilities the QMutexLocker
> CTOR isn't yet inlined (the DTOR is partially inlined, exposing the
> isLocked check).
>
> Note the "impossible" path is actually in the sources - so there might
> be a missing conditional somewhere.
I don't see it, but that's probably because I'm looking at it from the C++
side. If the mutex pointer that was passed is null, then isLocked is never set
to true. What you're saying is that the unlock() function above was inlined and
that GCC knew m to be nullptr, but didn't know isLocked's value... which makes
no sense to me. If the constructor wasn't inlined, it couldn't know the value
of m either. If the constructor was inlined, then it should know the value of
both.
Anyway, this discussion made me realise there's a series of changes to
QMutexLocker ending in "QMutexLocker: strenghten the locking operations"
(https://code.qt.io/cgit/qt/qtbase.git/commit/?id=1b1456975347b044c11169458b53c9f6083dbc59).
This probably did change how the optimiser works, explaining why the warnings
went away.
But it shouldn't have. We went from
inline ~QMutexLocker() {
unlock();
}
inline void unlock() noexcept
{
if (!isLocked)
return;
m->unlock();
isLocked = false;
}
to
inline ~QMutexLocker()
{
if (m_isLocked)
unlock();
}
inline void unlock() noexcept
{
Q_ASSERT(m_isLocked);
m_mutex->unlock();
m_isLocked = false;
}
with the Q_ASSERT expanding to nothing in release builds, it should be
effectively identical code.
[Bug tree-optimization/104475] [12/13 Regression] Wstringop-overflow + atomics incorrect warning on dynamic object
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104475 --- Comment #18 from Richard Biener --- The change improved the wording of the diagnostic by appending the note indicating an object at zero address. It didn't mitigate the diagnostic which as far as I analyzed is technically correct (but not very helpful). An improvement for these diagnostics would be analyzer-style reporting of the guarding conditions. Another possible improvement would be to somehow keep a pointer to the symbolic base we equality-propagated from the conditional so that we, for if (!d) if (enabled) *d = 0; can say the object pointed-to by 'd' when 'd' is nullptr is accessed here. The IL currently just has a pointer constant and doesn't know that was originally derived from 'd'.
[Bug tree-optimization/104475] [12/13 Regression] Wstringop-overflow + atomics incorrect warning on dynamic object
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104475 --- Comment #17 from CVS Commits --- The master branch has been updated by Richard Biener : https://gcc.gnu.org/g:926f5059bb8d295e2b68cea7c9af53606946eb1a commit r13-4503-g926f5059bb8d295e2b68cea7c9af53606946eb1a Author: Richard Biener Date: Tue Dec 6 10:12:01 2022 +0100 tree-optimization/104475 - improve access diagnostics When we end up isolating a nullptr path it happens we diagnose accesses to offsetted nullptr objects. The current diagnostics have no good indication that this happens so the following records the fact that our heuristic detected a nullptr based access in the access_ref structure and sets up diagnostics to inform of that detail. The diagnostic itself could probably be improved here but its API is twisted and the necessary object isn't passed around. Instead of just ...bits/atomic_base.h:655:34: warning: 'unsigned int __atomic_fetch_and_4(volatile void*, unsigned int, int)' writing 4 bytes into a region of size 0 overflows the destination [-Wstringop-overflow=] we now add In member function 'void QFutureInterfaceBase::setThrottled(bool)': cc1plus: note: destination object is likely at address zero PR tree-optimization/104475 * pointer-query.h (access_ref::ref_nullptr_p): New flag. * pointer-query.cc (access_ref::access_ref): Initialize ref_nullptr_p. (compute_objsize_r): Set ref_nullptr_p if we treat it that way. (access_ref::inform_access): If ref was treated as nullptr based, indicate that.
[Bug tree-optimization/104475] [12/13 Regression] Wstringop-overflow + atomics incorrect warning on dynamic object
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104475 --- Comment #16 from Richard Biener --- The odd thing is that we do /* Pointer constants other than null smaller than param_min_pagesize might be the result of erroneous null pointer addition/subtraction. Unless zero is a valid address set size to zero. For null pointers, set size to the maximum for now since those may be the result of jump threading. Similarly, for values >= param_min_pagesize in order to support (type *) 0x7cdeab00. */ if (integer_zerop (ptr) || wi::to_widest (ptr) >= param_min_pagesize) pref->set_max_size_range (); so if we plain dereference nullptr we will not diagnose the access but if we dereference at an address between zero and param_min_pagesize we will. The machinery unfortunately doesn't propagate this decision so the diagnostic itself is quite unhelpful (or would have to replicate the above). The code also doesn't catch upcasting of nullptr which would result in small "negative" pointers. I have a patch improving the diagnostic by means of printing a note like /home/tjmaciei/dev/gcc/include/c++/13.0.0/bits/atomic_base.h:655:34: warning: 'unsigned int __atomic_fetch_and_4(volatile void*, unsigned int, int)' writing 4 bytes into a region of size 0 overflows the destination [-Wstringop-overflow=] In member function 'void QFutureInterfaceBase::setThrottled(bool)': cc1plus: note: destination object is likely at address zero amending each and every "into a region of size 0" case would be tedious since the API used there doesn't pass down the object I amended.
[Bug tree-optimization/104475] [12/13 Regression] Wstringop-overflow + atomics incorrect warning on dynamic object
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104475
--- Comment #15 from Richard Biener ---
Thanks, it's still the same reason - we isolate a nullptr case and end up with
__atomic_or_fetch_4 (184B, 64, 0); [tail call]
The path we isolate is d->m_mutex == nullptr && !enable in
void QFutureInterfaceBase::setThrottled(bool enable)
{
QMutexLocker lock(>m_mutex);
if (enable) {
switch_on(d->state, Throttled);
} else {
switch_off(d->state, Throttled);
if (!(d->state.loadRelaxed() & suspendingOrSuspended))
d->pausedWaitCondition.wakeAll();
}
}
where for unknown reason QMutexLocker happily allows a nullptr mutex
(it's documented that way)
inline explicit QMutexLocker(Mutex *mutex) noexcept
{
m = mutex;
if (__builtin_expect(!!(mutex), true)) {
mutex->lock();
isLocked = true;
}
}
we predict the path to be unlikely but the adjustment to the threader
covered probably never executed paths (with probability zero). The
threading opportunity arises because the DTOR calls
inline void unlock() noexcept
{
if (!isLocked)
return;
m->unlock();
isLocked = false;
}
and we know isLocked on the nullptr path.
I thought we could maybe enhance prediction to look for nullptr based
accesses but at the time we estimate probabilities the QMutexLocker
CTOR isn't yet inlined (the DTOR is partially inlined, exposing the
isLocked check).
Note the "impossible" path is actually in the sources - so there might
be a missing conditional somewhere.
[Bug tree-optimization/104475] [12/13 Regression] Wstringop-overflow + atomics incorrect warning on dynamic object
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104475 --- Comment #14 from Thiago Macieira --- Created attachment 54015 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54015=edit qfutureinterface.cpp preprocessed [gcc trunk-20221205] (In reply to Richard Biener from comment #13) > There's been some changes on trunk but the preprocessed source doesn't work > there. Uploaded the updated preprocessed source with current trunk, from roughly the same Qt commit (I chose a date just before this bug report was opened). I can still reproduce this issue with the minimal original sources and these preprocessed, but somehow not with the actual qfutureinterface.cpp, either then or now.
[Bug tree-optimization/104475] [12/13 Regression] Wstringop-overflow + atomics incorrect warning on dynamic object
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104475 --- Comment #13 from Richard Biener --- There's been some changes on trunk but the preprocessed source doesn't work there.
[Bug tree-optimization/104475] [12/13 Regression] Wstringop-overflow + atomics incorrect warning on dynamic object
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104475 Richard Biener changed: What|Removed |Added Target Milestone|12.2|12.3 --- Comment #12 from Richard Biener --- GCC 12.2 is being released, retargeting bugs to GCC 12.3.
[Bug tree-optimization/104475] [12/13 Regression] Wstringop-overflow + atomics incorrect warning on dynamic object
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104475 Richard Biener changed: What|Removed |Added Priority|P3 |P2
[Bug tree-optimization/104475] [12/13 Regression] Wstringop-overflow + atomics incorrect warning on dynamic object
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104475 Jakub Jelinek changed: What|Removed |Added Target Milestone|12.0|12.2 --- Comment #11 from Jakub Jelinek --- GCC 12.1 is being released, retargeting bugs to GCC 12.2.
