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. Now, this gdb code is using the old VEC (originated from gcc's C days, it's not the current C++fied VEC implementation), but the point is that any other random vector-like container out there is free to optimize copy of a range of non-trivial but trivially copyable types using memcpy/memmove. Note that libstdc++ does not actually do that optimization, but that's just a missed optimization, see PR libstdc++/68350 [1] "std::uninitialized_copy overly restrictive for trivially_copyable types". (libstdc++'s std::vector defers copy to std::unitialized_copy.) [1] - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68350 > These aren't undefined and the patch could be tweaked to allow > them. I think they're undefined because the types are not trivially copyable (non-trivial destructor with side effects). > I decided not to invest effort into it because, although > not strictly erroneous, I think they represent poor practice. I think that for my suggestion, all you really need is use call trivially_copyable_p instead of trivial_type_p, for memcpy/memmove/mempcpy at least. For memset, I'd suggest to go the other direction actually, and instead of relaxing the trivial check, to tighten it, by warning about memset'ting objects of non-standard-layout type too. I.e., warn for memset of all non-POD (non-trivial + non-standard-layout) types. That's what I did in the gdb patch. That would also produce warnings for memset of trivial types that via refactoring end up with references, like: struct Trivial { Trivial () = default; Trivial (int &i) : m_i (i) {} void add (int howmuch) { m_i += howmuch; } private: int &m_i; }; void reset (Trivial *triv) { memset (triv, 0, sizeof (Trivial)); } void recompute (Trivial *triv) { reset (triv); // start over triv->add (10); // whoops, null reference. } It's also warn for memset of trivial types that aren't standard layout due to having a mix of public/protected/private fields, which is likely not a real problem in practice, but I'd call those a code smell that warrants a warning too: struct S { private: int a; public: int b; }; S s; memset (&s, 0, sizeof (S)); Playing with the patch, I noticed that you can't silence it by casting the pointer to void, but you can with casting to char, like: void copy (B *dst, B *src) { memcpy (dst, src, sizeof (*src)); // warns memcpy ((void*) dst, (void *) src, sizeof (*src)); // still warns memcpy ((char*) dst, (char *) src, sizeof (*src)); // doesn't warn memcpy ((unsigned char*) dst, (unsigned char *) src, sizeof (*src)); // doesn't warn } I can understand how we end up like that, given char's magic properties, but still I think many will reach for "void" first if they (really really) need to add a cast to silence the warning. In any case, I think it'd be very nice to add cases with such casts to the new tests, and maybe mention it in the documentation too. Thanks, Pedro Alves
