warn_cstruct_memaccess should not trigger inside constructors for otherwise trivially initializable types

[ojhunt]

We warn on this code, and variations of it, when gcc does not

  struct Object {
   int some_field;
   Object() { memset(this, 0, sizeof(*this); }
   Object(const Object&);
  };

I put together a general set of the cases at https://godbolt.org/z/5YYj1oxf9

Basically performing these operations within constructors of a type, as long as the reason for the type not being trivially_{whatever} is just its own constructors, not due to the object being polymorphic or some sub- or baseobject being non-trivial to copy or initialize.

[ojhunt]

These could be related to #111434 @serge-sans-paille, but @ahatanak has also written some of these checks.

[ojhunt]

This makes me wonder if we should warn on the use of the normal memset/bzero functions in destructors on the basis that they're likely being used to try an "securely" clear an object, but it would be UB for that to be observable so can be (is?) removed by the compiler. similarly memset/bzero(ptr, ...); delete ptr/free(ptr);

[ojhunt]

Someone left a comment that has seems to not be displaying: "The class is not trivially-copyable in the sense of the language standard, so that memset should be considered UB."

This is the base class - if we are to tailor this to constructors we should consider (compare to gcc warnings) only being silent in base class constructors (i.e constructors would warn if bot the base class). Also we'd need to require that the destination be this - not just any memset in the constructor.

"The class is also not trivial for the purpose of layout in the ABI sense, meaning that tail padding (if there was any) would be reused by derived classes, causing the memset to wreak havoc if the derived class is ever copied."

That problem would be a subclass constructor doing this after the baseclass - which would be bad for many reasons.

[keinflue]

Someone left a comment that has seems to not be displaying: "The class is not trivially-copyable in the sense of the language standard, so that memset should be considered UB."

Sorry, that was me. After posting it I immediately realized that my argument wasn't exactly good, so I deleted it again. I was trying to come up with a concrete example where using the memset in the default constructor would go wrong due to tail padding reuse, but couldn't. So I didn't reply again.

[ojhunt]

Someone left a comment that has seems to not be displaying: "The class is not trivially-copyable in the sense of the language standard, so that memset should be considered UB."

Sorry, that was me. After posting it I immediately realized that my argument wasn't exactly good, so I deleted it again. I was trying to come up with a concrete example where using the memset in the default constructor would go wrong due to tail padding reuse, but couldn't. So I didn't reply again.

ah no worries, I assumed GH screwing up, because I generally assume that of GH :D

It's a fairly common - and somewhat idiomatic in some places, certainly memset_s/bzero_s are the only way to ensure padding is actually zeroed, the non _s versions are the most clean and non-fragile mechanisms to ensure all fields are zeroed.

[shafik]

Are you arguing that #111434 got it wrong or that technically it is correct from a wording perspective but is practically safe? I am trying to sus out if there is a wording question here or not.

I did not find any core issues in this are but did find P2782.

[ojhunt]

Are you arguing that #111434 got it wrong or that technically it is correct from a wording perspective but is practically safe?

@shafik This is a warning, and it was motivated by a similar warning in GCC, but GCC does not warn for this operation being performed with this as the target inside a constructor.

Separate from appealing to authority (the semantics of the gcc warning) memset/bzero are idiomatic ways of zeroing out a struct during construction, so not issuing a warning (again, in the root class of a hierarchy) seems reasonable.

I was pondering what @keinflue said though, consider:

struct Foo {
   // ...
   Foo() { bzero(this, sizeof(*this)); }
};
struct Bar {
  // ...
};
struct Wibble : Foo, Bar {};

[edit to add the intended consideration]

Consider: is it possible to construct a type (via alignment, packed, etc or similar) where sizeof() could result in the size extending beyond end of the Foo base class? I think that even it that were possible it would not impact anything as initialization order would simply make such a tail padding write precede the the subsequent base init.

I am trying to sus out if there is a wording question here or not.

I think there might be a wording issue in the warning regardless of the above -- while it's technically correct in that clang is warning about the operation because the type is not trivially copyable, it's producing that warning in response to memset(expr, 0, size) so it seems likely to be confusing to the developer that we are talking about copying? On the other hand is it really reasonable to warn on memset(0)/bzero due to non-trivial copy in the first place? it's clearly an initialization step, and is clearly not intended to perform part of a copy operation? (I'm not sure what gcc does here)

I did not find any core issues in this are but did find P2782.

That does seem reasonable I think, but is independent of this warning - the code I've found that breaks on this is fairly idiomatic (due to -Werror, because of course), but the core issue seems to be the inclusion of trivial copyability in the list of reasons to warning on memset and bzero.