Bug 108374 - [12/13/14 Regression] unexpected -Wstringop-overflow when using std::atomic and std::shared_ptr
Summary: [12/13/14 Regression] unexpected -Wstringop-overflow when using std::atomic a...
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 12.2.0
: P2 normal
Target Milestone: 12.4
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks: Wstringop-overflow
  Show dependency treegraph
 
Reported: 2023-01-11 18:18 UTC by Romain Geissler
Modified: 2023-05-08 12:26 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2023-01-12 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Romain Geissler 2023-01-11 18:18:04 UTC
Hi,

The following snippet produces an unexpected -Wstringop-overflow with gcc 12 and current trunk:

#include <atomic>
#include <memory>

struct A: public std::enable_shared_from_this<A>
{
    std::atomic<size_t> _attr;
};

void f(std::shared_ptr<A> pointer)
{
    std::weak_ptr<A> weakPointer(pointer);

    [[maybe_unused]] const unsigned int aAttr = weakPointer.lock()->_attr;
}


Compiler Explorer output:
In file included from /opt/compiler-explorer/gcc-trunk-20230111/include/c++/13.0.0/atomic:41,
                 from <source>:1:
In member function 'std::__atomic_base<_IntTp>::__int_type std::__atomic_base<_IntTp>::load(std::memory_order) const [with _ITp = long unsigned int]',
    inlined from 'std::__atomic_base<_IntTp>::operator __int_type() const [with _ITp = long unsigned int]' at /opt/compiler-explorer/gcc-trunk-20230111/include/c++/13.0.0/bits/atomic_base.h:365:20,
    inlined from 'void f(std::shared_ptr<A>)' at <source>:13:69:
/opt/compiler-explorer/gcc-trunk-20230111/include/c++/13.0.0/bits/atomic_base.h:505:31: error: 'long unsigned int __atomic_load_8(const volatile void*, int)' writing 8 bytes into a region of size 0 overflows the destination [-Werror=stringop-overflow=]
  505 |         return __atomic_load_n(&_M_i, int(__m));
      |                ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
In function 'void f(std::shared_ptr<A>)':
cc1plus: note: destination object is likely at address zero
cc1plus: some warnings being treated as errors
Compiler returned: 1


I have found bug #104475 which seems to also deal with atomics and -Wstringop-overflow however I can't judge if it looks like a duplicate or a different issue.

Cheers,
Romain
Comment 1 Romain Geissler 2023-01-11 18:19:57 UTC
I forgot to mention: this happens on x86-64 with -O1.
Comment 2 Richard Biener 2023-01-12 09:43:57 UTC
We now say

cc1plus: note: destination object is likely at address zero

<bb 2> [local count: 1073741824]:
_9 = MEM[(const struct __shared_ptr &)pointer_2(D)]._M_ptr;
_10 = MEM[(const struct __shared_count &)pointer_2(D) + 8]._M_pi;
if (_10 != 0B)
  goto <bb 4>; [53.47%]
else
  goto <bb 3>; [46.53%]

<bb 3> [local count: 499612072]:
__atomic_load_8 (16B, 5); [tail call]
D.37576 ={v} {CLOBBER};
D.37576 ={v} {CLOBBER(eol)};
goto <bb 19>; [100.00%] (leads straight to return)

so we have __shared_count of pointer _M_pi == 0 here, whatever that means
and not sure why we atomically load here.

Confirmed.
Comment 3 Jonathan Wakely 2023-01-12 09:57:34 UTC
(In reply to Romain Geissler from comment #0)
>     std::weak_ptr<A> weakPointer(pointer);
> 
>     [[maybe_unused]] const unsigned int aAttr = weakPointer.lock()->_attr;

If pointer == nullptr then weakPointer.lock() is also null, and so dereferencing it to access the attr member is undefined, and does indeed perform an atomic load at address 0.

Instead of complaining about it, I would expect GCC to treat that undefined condition as unreachable and optimize it away.
Comment 4 Richard Biener 2023-01-12 10:00:06 UTC
(In reply to Jonathan Wakely from comment #3)
> (In reply to Romain Geissler from comment #0)
> >     std::weak_ptr<A> weakPointer(pointer);
> > 
> >     [[maybe_unused]] const unsigned int aAttr = weakPointer.lock()->_attr;
> 
> If pointer == nullptr then weakPointer.lock() is also null, and so
> dereferencing it to access the attr member is undefined, and does indeed
> perform an atomic load at address 0.
> 
> Instead of complaining about it, I would expect GCC to treat that undefined
> condition as unreachable and optimize it away.

Hmm, but then the program is bogus, no?  And a diagnostic warranted.

At least if it is well-defined to have a nullptr == pointer.

So I'd be inclined to close as INVALID?
Comment 5 Jonathan Wakely 2023-01-12 10:10:34 UTC
(In reply to Richard Biener from comment #4)
> Hmm, but then the program is bogus, no?  And a diagnostic warranted.

No.

> At least if it is well-defined to have a nullptr == pointer.

It's well defined, but that doesn't mean the program is bogus. It just means you can't call that function with such a value.

> So I'd be inclined to close as INVALID?

No, I don't think so. It would only be invalid if you called f(nullptr) or similar.

The code is basically doing something like:

int f(const A* p, bool is_valid)
{
  const A* q = is_valid ? p : nullptr;
  return *q;
}

Instead of complaining that q might be null, we can optimize that to return *p.

It might be nicer to optimize it to:

  if (!p)
    __builtin_trap();
  return *p;

but either way, we can't just declare the whole program to be invalid because it's possible to call the function incorrectly.
Comment 6 Jakub Jelinek 2023-01-12 11:34:09 UTC
The warning is simply badly designed like most of the middle-end warnings.

As has been discussed, we should have some switch to decide what to do when we have such provable UB, and the possibilities should be turn it into __builtin_unreachable (), turn it into __builtin_trap () or for users who don't mind seeing lots of false positives keep the warning.  Though perhaps now that we have -funreachable-traps the option could be just 2 possibilities, unreachable vs. (useless) warnings, with the default of the former.
Comment 7 Richard Biener 2023-05-08 12:26:28 UTC
GCC 12.3 is being released, retargeting bugs to GCC 12.4.