[Bug c++/104855] New: -Wclass-memaccess is too broad with valid code

[Bug c++/104855] New: -Wclass-memaccess is too broad with valid code

[soap at gentoo dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104855

            Bug ID: 104855
           Summary: -Wclass-memaccess is too broad with valid code
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: soap at gentoo dot org
  Target Milestone: ---

Given the following snippet:

#include <cassert>
#include <cstring>
#include <type_traits>

struct A
{
    A() = default;
    A(unsigned a, unsigned b) : data_(a + b) {}

private:
    unsigned data_;
};

static_assert(std::is_trivial_v<A>, "");
static_assert(std::is_trivially_copyable_v<A>, "");

A foo(unsigned x)
{
    A result;
    std::memcpy(&result, &x, sizeof(x));
    return result;
}

with GCC 11 and -Wall, I get the following warning

example.cpp: In function ‘A foo(unsigned int)’:
example.cpp:20:16: warning: ‘void* memcpy(void*, const void*, size_t)’ copying
an object of type ‘struct A’ with ‘private’ member ‘A::data_’ from an array of
‘unsigned int’; use assignment or copy-initialization instead
[-Wclass-memaccess]
   20 |     std::memcpy(&result, &x, sizeof(x));
      |     ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
example.cpp:5:8: note: ‘struct A’ declared here
    5 | struct A
      |        ^

which seems overbroad given that the code is 100% valid. I have asked Martin
Sebor, and his reason for this warning is

> about it breaking encapsulation by modifying a private data member.
> In the test case the modification might violate the invariant that
> the class represent the sum of the arguments it's constructed with.

which I understand, but I still consider this warning too broad for valid code,
especially because the suggested workaround is to cast the &result to a void*,
which involves reinterpret_cast and will raise eyebrows. Clang doesn't do this,
and I think it's right on this one.

[Bug c++/104855] -Wclass-memaccess is too broad with valid code

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104855

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|unknown                     |12.0
           Keywords|                            |diagnostic

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
I think the code is definitely "bad style", it seems A lacks a CTOR from
'unsigned' and the code tries to workaround this issue.

Not sure whether these kind of "style" diagnostics should be in -Wall

[Bug c++/104855] -Wclass-memaccess is too broad with valid code

[soap at gentoo dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104855

--- Comment #2 from David Seifert <soap at gentoo dot org> ---
(In reply to Richard Biener from comment #1)
> I think the code is definitely "bad style", it seems A lacks a CTOR from
> 'unsigned' and the code tries to workaround this issue.
> 
> Not sure whether these kind of "style" diagnostics should be in -Wall

Even if I add a single argument ctor, the warning remains.

[Bug c++/104855] -Wclass-memaccess is too broad with valid code

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104855

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-03-10
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Confirmed. I think we should separate "this is undefined because your type
isn't trivially copyable" from "this seems like poor style".

[Bug c++/104855] -Wclass-memaccess is too broad with valid code

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104855

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
And the former category *should* be in -Wall, but IMHO the latter shouldn't be.

[Bug c++/104855] -Wclass-memaccess is too broad with valid code

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104855

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |INVALID
                 CC|                            |msebor at gcc dot gnu.org

--- Comment #5 from Martin Sebor <msebor at gcc dot gnu.org> ---
The documented purpose of the warning is to detect not just code that's
undefined (such as overwriting const or reference members) but also code that
violates encapsulation (and with it potentially also class invariants).  Such
code may be valid in the strict language sense but it's almost certainly not
valid under the design of the class, as in the test case in comment #0.

In the rare instances when such code is intentional and safe or where it cannot
be changed, an explicit cast along with a suitable comment certainly seems like
a preferable solution to avoid the warning than would be disabling it for the
majority of uses where it is not intentional.

Since any object pointer can be converted to void*, either implicitly (by
passing it to a memcpy argument) or by a static_cast, using reinterpret_cast is
not necessary to suppress the warning (nor would it be appropriate).

If -Wclass-memaccess had the ability to analyze the class invariants splitting
up -Wclass-memaccess into multiple levels as Jonathan suggests in comment #3
might perhaps be doable and useful, with level 1 triggering for the subset of
code that's strictly undefined, and higher levels for the rest.  But because
the warning runs in the C++ front end where the actual implementation of the
class is not readily analyzable, the separation would result in a bias heavily
skewed toward the latter (i.e., lots of false negatives at level 1).

The guidance for deciding whether or not a subset of warnings should be in
-Wall is in the GCC manual:

  [-Wall] enables all the warnings about constructions that some users consider
questionable, and that are easy to avoid (or modify to prevent the warning),
even in conjunction with macros. 

As Richard notes in comment #1, this instance of -Wclass-memacess (and in my
experience the overwhelming majority of others) with the easy suppression fit
this description.