On 05/16/2017 04:48 PM, Pedro Alves wrote:
On 05/16/2017 08:41 PM, Jason Merrill wrote:
I agree that it makes sense to
check for a trivial assignment operator specifically. I guess we want
a slightly stronger "trivially copyable" that also requires a
non-deleted assignment operator.
It seems to me that the relevant tests are:
bcopy/memcpy/memmove want trivally copyable + non-deleted assignment.
bzero/memset want trivial + non-deleted assignment.
I'm still not convinced we need to consider standard-layout at all.
What do you think of warning for memset of types with references? Since NULL
references are undefined behavior (N4659, [dcl.ref]/5), IMHO such code is quite
smelly and most likely a sign of incomplete refactoring, not design.
(My original intention for poisoning memset of standard-layout type in gdb was
mainly as proxy/approximation for "type with references". Other properties
that make a type non-standard-layout are not as interesting to me.)
While at it, maybe the same reasoning would justify warn of memcpy/memset
of types with const data members? memcpy of such types kind of sounds like a
recipe for subtle breakage that could only be salvaged with std::launder.
And if you know about that, you'll probably be copying objects of type T
to/from raw byte/char storage, not to/from another T.
Actually, now that I look at Martin's new patch, I see he's already warning
about const data members and references, both memcpy and memset. I'm not
super convinced on warning about memcpy of references (unlike memset), but
I don't feel strongly against it either. I'd be fine (FWIW) with giving it a
try and see what breaks out there.
I did this because objects with references cannot be assigned
to (the default copy assignment is deleted). So as a baseline
rule, I made the warning trigger whenever a native assignment
or copy isn't valid. In the IMO unlikely event that a memcpy
over a reference is intended, the warning is easy to suppress.
I expect calling memset on an object with a reference to almost
certainly be a bug since there's no way to make such a reference
usable. The only time it might be intentional is when someone
tries to wipe out the storage before deleting the object in
the storage (e.g., in a dtor). But that's a misuse as well
because such calls are typically eliminated, much to many
a security analyst's shock and horror. I'd like to eventually
diagnose that too (though possibly under a different warning).
I used a similar (though not exactly the same) rationale for
warning for const members. They too cannot be assigned to,
and letting memset or memcpy silently change them violates
const-correctnes. It's also undefined and the immutability
of such members an optimization opportunity waiting to be
exploited.
+Wnon-trivial-memaccess
+C++ ObjC++ Var(warn_nontrival_memaccess) Warning LangEnabledBy(C++ ObjC++,
Wall)
+Warn for raw memory writes to objects of non-trivial types.
May I suggest renaming the warning (and the description above) from
-Wnon-trivial-memaccess to something else that avoids "trivial" in
its name? Because it's no longer strictly about "trivial" in the
standard C++ sense. The documentation talks about "The safe way",
and "does not warn on safe calls", so maybe call it -Wunsafe-memaccess?
I debated whether to rename things (not just the warning but
also the function that implements it in GCC). Ultimately I
decided it wasn't necessary because the rules seem close enough
to be based on some notion of triviality and because no better
name came to mind. -Wunsafe-memaccess might work. The one mild
concern I have with it is that it could suggest it might do more
than simple type checking (e.g., buffer overflow and what not).
Let's see what Jason thinks.
(I spotted a couple typos in the new patch: "otherwse", "becase", btw.)
I'm a horrible typist. I'll proofread the patch again and fix
them up before committing it.
Martin
