Usage of memset with non-trivial types that results in -Wclass-memaccess

[am11]

Starting with gcc 8, compiler is warning about several usages of memset in case of types that are
non-trivially copyable. This includes structs inheriting struct ZeroInit, usage of FillMemory and ZeroMemory macros (defined in pal.h) and other places which are directly calling the function (although some of those places can make use of the defined macro as it stands).

From release notes (https://www.gnu.org/software/gcc/gcc-8/changes.html):

-Wclass-memaccess warns when objects of non-trivial class types are manipulated in potentially unsafe ways by raw memory functions such as memcpy, or realloc. The warning helps detect calls that bypass user-defined constructors or copy-assignment operators, corrupt virtual table pointers, data members of const-qualified types or references, or member pointers. The warning also detects calls that would bypass access controls to data members.

LLVM has included -Wnontrivial-memaccess for C structs and there are also considerations to add -Wclass-memaccess in future: https://reviews.llvm.org/D45310

Reading the community discussions about practices pertaining to usage of memset with non-POD types and how others have responded to the gcc warning, I think there are three ways to fix this (in descending order of preference):

1. Call proper ctor, use std:fill and friends for non-trivial data structures.
2. Cast pointer to void* to circumvent the type check.
3. Suppress warning in cmake -Wno-class-memaccess.

[mikedn]

Call proper ctor

That would be ideal but I'm afraid that at least in the JIT code you'll find some cases that might be difficult to fix like that. So 2. will probably be needed as well, if it doesn't cause other problems.

[jkotas]

How many places do hit this warning - could you please share the list (as gist if it is too long)?

[am11]

cd coreclr
./build.sh -skiprestore -skipmanaged -gcc &> build-output.txt

# count unique lines
cat build-output.txt | grep class-memaccess | uniq -uc | wc -l
# => 1646

# filter output file with afterLines=2 and replace newline with literal '\n'
CONTENTS=`cat build-output.txt | grep -A 2 class-memaccess | sed -E ':a;N;$!ba;s/\r{0,1}\n/\\n/g'`

# write gist API request body to a temp file
echo "{\"public\":false,\"files\":{\"class-memaccess.txt\":{\"content\":\"$CONTENTS\"}}}" > class-memaccess-gist.txt

# send to github server using access token
curl -X POST -d @class-memaccess-gist.txt -u <personal-access-token-with-'gist'-scope>:x-oauth-basic https://api.github.com/gists 

gist | raw

[am11]

@jkotas, I have found few places while fixing some of these warnings in cee_wks target, where 204 is being memset() to complex types under #ifdef _DEBUG, e.g.

runtime/src/coreclr/src/vm/class.h

Line 2157 in e7ce09d

#ifdef _DEBUG

:

1. are they (all) in use?
2. what is the significance of number 204; can we make it 0?

[jkotas]

what is the significance of number 204;

It is easy to notice that it is a uninitialized value when debugging. It is why it is done only under _DEBUG.

[jkotas]

The specific one you have linked is getting major refactoring in dotnet/coreclr#1828.

[RobertHenry6bev]

There are numerous places gcc-12 complains about memset being called to zero the entire object when the object is not Plain Old Data (POD). (The C++ standard haas a more encompassing definition than POD.)

Consider

runtime/src/coreclr/vm/syncblk.h

Line 631 in 3e0a5ad

ZeroMemory(this, sizeof(InteropSyncBlockInfo));

InteropSyncBlockInfo has a lot of non-trivial data, some of which will (likely?) have constructors fired on them, and the memset is a blunt hammer to zero out everything, perhaps leaving embedded objects in an inconsistent and buggy state.

[RobertHenry6bev]

I went through and added gcc diagnostic suppressions throughout the code base as needed to get to compile with gcc12.

There were 41 files changed with ~50 changes. See attached file
robhenry-memaccess-pragma.txt

It is conceivable that the CONTRACT semantics of some of the callers to memset tell some compiler that memset is safe, but gcc certainly doesn't know about that.

[RobertHenry6bev]

/cc @AaronRobinsonMSFT