Hi Martin,
Thanks much for doing this. A few comments below, in light of my
experience doing the equivalent checks in the gdb patch linked below,
using standard C++11.
On 04/29/2017 09:09 PM, Martin Sebor wrote:
Calling memset, memcpy, or similar to write to an object of
a non-trivial type (such as one that defines a ctor or dtor,
or has such a member) can break the invariants otherwise
maintained by the class and cause undefined behavior.
The motivating example that prompted this work was a review of
a change that added to a plain old struct a new member with a ctor
and dtor (in this instance the member was of type std::vector).
To help catch problems of this sort some projects (such as GDB)
have apparently even devised their own clever solutions to detect
them: https://sourceware.org/ml/gdb-patches/2017-04/msg00378.html.
The attached patch adds a new warning, -Wnon-trivial-memaccess,
that has GCC detect these mistakes. The patch also fixes up
a handful of instances of the problem in GCC. These instances
correspond to the two patterns below:
struct A
{
void *p;
void foo (int n) { p = malloc (n); }
~A () { free (p); }
};
void init (A *a)
{
memset (a, 0, sizeof *a);
}
and
struct B
{
int i;
~A ();
};
(typo: "~B ();")
void copy (B *p, const B *q)
{
memcpy (p, q, sizeof *p);
...
}
IMO the check should be relaxed from "type is trivial" to "type is
trivially copyable" (which is what the gdb detection at
https://sourceware.org/ml/gdb-patches/2017-04/msg00378.html
uses for memcpy/memmove). Checking that the destination is trivial is
going to generate false positives -- specifically, [basic-types]/3
specifies that it's fine to memcpy trivially _copyable_ types, not
trivial types. A type can be both non-trivial and trivially copyable
at the same time. For example, this compiles, but triggers
your new warning:
#include <stdlib.h>
#include <string.h>
#include <type_traits>
struct NonTrivialButTriviallyCopyable
{
NonTrivialButTriviallyCopyable () : i (0) {}
int i;
};
static_assert (!std::is_trivial<NonTrivialButTriviallyCopyable>::value, "");
static_assert (std::is_trivially_copyable<NonTrivialButTriviallyCopyable>::value, "");
void copy (NonTrivialButTriviallyCopyable *dst, NonTrivialButTriviallyCopyable *src)
{
memcpy (dst, src, sizeof (*src));
}
$ /opt/gcc/bin/g++ -std=gnu++11 trivial-warn.cc -o trivial-warn -g3 -O0 -Wall -Wextra -c
trivial-warn.cc: In function ‘void copy(NonTrivialButTriviallyCopyable*, NonTrivialButTriviallyCopyable*)’:
trivial-warn.cc:16:34: warning: calling ‘void* memcpy(void*, const void*, size_t)’ with a pointer to a non-trivial type ‘struct NonTrivialButTriviallyCopyable’ [-Wnon-trivial-memaccess]
memcpy (dst, src, sizeof (*src));
^
$
Implementations of vector-like classes can very well (and are
encouraged) to make use of std::is_trivially_copyable to know whether
they can copy a range of elements to new storage
using memcpy/memmove/mempcpy.
Running your patch against GDB trips on such a case:
src/gdb/btrace.h: In function ‘btrace_insn_s* VEC_btrace_insn_s_quick_insert(VEC_btrace_insn_s*, unsigned int, const btrace_insn_s*, const char*, unsigned int)’:
src/gdb/common/vec.h:948:62: error: calling ‘void* memmove(void*, const void*, size_t)’ with a pointer to a non-trivial type ‘btrace_insn_s {aka struct btrace_insn}’ [-Werror=non-trivial-memaccess]
memmove (slot_ + 1, slot_, (vec_->num++ - ix_) * sizeof (T)); \
^
There is nothing wrong with the code being warned here.
While "struct btrace_insn" is trivial (has a user-provided default
ctor), it is still trivially copyable.