GCC warnings for memcpy

[Daniel Cheng]

Hi all,

I was working on fixing some MSan errors to unblock `-fsanitize-memory-param-retval`, and I landed what I had hoped was a simple Skia patch to fix one case of uninit: https://skia-review.googlesource.com/c/skia/+/835697

Unfortunately, this makes GCC rather unhappy, because we use memcpy with this type, and it's no longer considered a 'trivial type':

../SRC/skia/src/base/SkUtils.h:49:11: error: 'void* memcpy(void*, const void*, size_t)' copying an object of non-trivial type 'struct skvx::Vec<4, int>' from an array of 'const __vector(4) int' [-Werror=class-memaccess]
   49 |     memcpy(&val, ptr, sizeof(val));
      |     ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~

The GCC warning seems quite strict to me; the equivalent clang-tidy warning we use in Chrome only warns in case of non-trivially-copyable types (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/undefined-memory-manipulation.html).

1. Does anyone have *positive* experiences with this GCC warning? i.e. is there a reason it's useful to warn for *non-trivial* types instead of merely *non-trivially-copyable* types?

2. Does anyone have suggestions on a good way to suppress these types of warnings when it's "known" safe? Unfortunately, the warning is firing in templated code that uses memcpy, so the obvious way to disable it would be equivalent to disabling the warning everywhere.

Daniel

[Brian Osman]

After doing a quick search, it appears other projects have hit this (and also wonder about the fact that it doesn't check for trivially_copyable). Of note: The Skia templated code that's being triggered here already has static_asserts for is_trivially_copyable, so I'd be okay working around it within those routines. I saw one suggestion of casting the object pointer to void* prior to the memcpy call, which seems like it would thwart the warning. (My very quick attempt at creating a trivial repro in compiler explorer just failed, though).

[Peter Kasting]

On Wednesday, April 3, 2024 at 11:27:34 AM UTC-7 brian...@google.com wrote:

After doing a quick search, it appears other projects have hit this (and also wonder about the fact that it doesn't check for trivially_copyable).

Is there a gcc bug? If not, sounds like we should file one -- I agree that copying with memcpy() should not produce a warning for types marked trivially-copyable.

PK

[Daniel Cheng]

I think the officially-recommended way to mark this safe for gcc's warning is to use a cast, as Brian noted above (and that's what I did in the reland: https://skia-review.googlesource.com/c/skia/+/836616)

I didn't search for a pre-existing GCC bug (or try filing a new one). IMO, as far as GCC workarounds go, this one is fairly benign, and I didn't really have the time to advocate for changing this in gcc.

Daniel 

[Brian Osman]

Right. Also, the wording on the warning's documentation somewhat sidesteps the issue - it doesn't actually use the standard's terms for trivially-copyable. Rather:

Warn when the destination of a call to a raw memory function such as memset or memcpy is an object of class type, and when writing into such an object might bypass the class non-trivial or deleted constructor or copy assignment, violate const-correctness or encapsulation, or corrupt virtual table pointers.

That's fuzzy enough (to me), that I think it might be WAI? (Even if I disagree with the principle of warning in this case). 

[John Admanski]

On Mon, Apr 29, 2024 at 11:00 AM 'Brian Osman' via cxx <c...@chromium.org> wrote:

Right. Also, the wording on the warning's documentation somewhat sidesteps the issue - it doesn't actually use the standard's terms for trivially-copyable. Rather:

Warn when the destination of a call to a raw memory function such as memset or memcpy is an object of class type, and when writing into such an object might bypass the class non-trivial or deleted constructor or copy assignment, violate const-correctness or encapsulation, or corrupt virtual table pointers.

That's fuzzy enough (to me), that I think it might be WAI? (Even if I disagree with the principle of warning in this case). 

That just feels like it moves the issue to "the intention is wrong", though? Being something you can safely copy with memcpy is basically the point of "trivially copyable" so it seems incorrect for the compiler to invent a different definition of it.

The only bug I'm aware of for this is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107361.

 

On Mon, Apr 29, 2024 at 1:56 PM Daniel Cheng <dch...@chromium.org> wrote:

I think the officially-recommended way to mark this safe for gcc's warning is to use a cast, as Brian noted above (and that's what I did in the reland: https://skia-review.googlesource.com/c/skia/+/836616)

I didn't search for a pre-existing GCC bug (or try filing a new one). IMO, as far as GCC workarounds go, this one is fairly benign, and I didn't really have the time to advocate for changing this in gcc.

Daniel 

On Mon, 29 Apr 2024 at 08:31, Peter Kasting <pkas...@chromium.org> wrote:

On Wednesday, April 3, 2024 at 11:27:34 AM UTC-7 brian...@google.com wrote:

After doing a quick search, it appears other projects have hit this (and also wonder about the fact that it doesn't check for trivially_copyable).

Is there a gcc bug? If not, sounds like we should file one -- I agree that copying with memcpy() should not produce a warning for types marked trivially-copyable.

PK

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAMcBjo6tc2EHc4X%2BP1-Gx21en0LNDW1-2mosNOU_VmTH3y-kBw%40mail.gmail.com.

[Roland McGrath]

To be pedantic, memcpy is equivalent to copy-assignment when that's trivial.  It's not the same as copy-construction, in particular wrt to members declared const.  In both cases explicitly deleted constructor/operator= indicates some intent not to allow the usual copying even if trivial.  Those seem to be the distinctions the quoted manual wording intends to make.  For the latter, one might hope that it allows a constructor/operator= to be redeclared private and `= default` rather than deleted, and then permits memcpy uses only in places that would be allowed to use the private operator=.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAAE6AQs6fCZ1aaw--cEDhOV_279aRg%3D%2BC170yYyfo%3DUm4M_Yg%40mail.gmail.com.

[John Admanski]

I mean, that's kind of irrelevant in the context of this warning? If a type is trivially copyable, then you can copy the underlying bytes into another instance of the object, and that's safe to do. There might be other reasons why you should not do that sort of thing with a particular class (if it has const members, maybe you shouldn't be writing to them at all) but I don't think requiring trivial instead of trivially copyable is helpful in preventing that.

[Peter Kasting]

On Monday, April 29, 2024 at 11:21:16 AM UTC-7 John Admanski wrote:

On Mon, Apr 29, 2024 at 11:00 AM 'Brian Osman' via cxx <c...@chromium.org> wrote:

Right. Also, the wording on the warning's documentation somewhat sidesteps the issue - it doesn't actually use the standard's terms for trivially-copyable. Rather:

Warn when the destination of a call to a raw memory function such as memset or memcpy is an object of class type, and when writing into such an object might bypass the class non-trivial or deleted constructor or copy assignment, violate const-correctness or encapsulation, or corrupt virtual table pointers.

That's fuzzy enough (to me), that I think it might be WAI? (Even if I disagree with the principle of warning in this case). 

That just feels like it moves the issue to "the intention is wrong", though? Being something you can safely copy with memcpy is basically the point of "trivially copyable" so it seems incorrect for the compiler to invent a different definition of it.

The only bug I'm aware of for this is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107361.

From the linked bug, the original patch causing this behavior is likely https://gcc.gnu.org/legacy-ml/gcc-patches/2017-04/msg01571.html. I glanced at that briefly, and I think the original author was too quick to assume that memXXX() usage was "poor practice". I commented on this bug, which does seem like the right one.

(If anyone has gcc bugzilla edit permissions, feel free to confirm/triage that bug.)

PK