[PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

On 05/21/2017 10:50 PM, Jason Merrill wrote:
> On Sun, May 21, 2017 at 7:59 PM, Martin Sebor <msebor@gmail.com> wrote:
>> On 05/19/2017 03:42 PM, Jason Merrill wrote:
>>> On Fri, May 19, 2017 at 4:07 PM, Martin Sebor <msebor@gmail.com> wrote:
>>>> On 05/19/2017 01:07 PM, Jason Merrill wrote:
>>>>> On Tue, May 16, 2017 at 5:39 PM, Martin Sebor <msebor@gmail.com> wrote:
>>>>>> On 05/16/2017 01:41 PM, Jason Merrill wrote:
>>>>>>
>>>>>>> I'm still not convinced we need to consider standard-layout at all.
>>>>>>
>>>>>> I agree.  The patch doesn't make use of is_standard_layout_p().
>>>>>> It defines its own helper called almost_std_layout_p() that
>>>>>> combines trivial_type_p() with tests for non-public members,
>>>>>
>>>>>
>>>>>
>>>>> That's the part that seems unnecessary.  Why do we care about
>>>>> non-public members?
>>>>
>>>>
>>>> Because modifying them breaks encapsulation.
>>>
>>>
>>> Not if you're clearing/copying the object as a whole.
>>>
>>>> If I take a legacy struct, make some of its members private,
>>>> and define accessors and modifiers to manipulate those members
>>>> and maintain invariants between them, I will want to check and
>>>> adjust all code that changes objects of the struct in ways that
>>>> might violate the invariants.
>>>
>>> For a trivial type, worrying about invariants doesn't make sense to
>>> me, since default-initialization won't establish any invariants.  And
>>> bzero or memset(0) will have the same effect as value-initialization
>>> (if zero_init_p (type); we probably want to check that).  If you're
>>> going to establish invariants, I would expect you to write a default
>>> constructor, which would make the class non-trivial.
>>
>> Thanks for the zero_init_p pointer!  Let me add that to the patch
>> along with Pedro's suggestion to use the current C++ terminology,
>> retest and resubmit.
>>
>> In most cases you're right that defining the default constructor
>> is the way to go.  There is are a couple of use cases where a ctor
>> tends to be avoided: when objects the class need to be initialized
>> statically, and where they need to be PODs.  GCC itself relies on
>> the latter (e.g., some of the vec templates), apparently because
>> it stores them in unions.  It doesn't look tome like these vec
>> templates maintain much of an invariant of any kind, but they
>> easily could.
>>
>> An example of the static initialization case is an atomic class
>> (not necessarily std::atomic though I think this would apply to
>> it as well).  Its objects usually need to be statically default-
>> initializable (without running any ctors) but then they must be
>> explicitly initialized by some sort of an init call to be made
>> valid, and their state can only be accessed and modified via
>> member functions (and so their state is private).  Modifying
>> them by any other means, including by memset or memcpy, is
>> undefined.
>>
>>>
>>>> What common use case are you concerned about that isn't more
>>>> appropriately expressed using the generated default or copy
>>>> constructor or assignment operator?
>>>
>>>
>>> None; I am concerned about focusing the warning on code that is
>>> actually likely to be problematic.
>>
>>
>> Hopefully the above explanation resolves that concern.  If not,
>> please let me know.
>
> I still think that we shouldn't warn about zeroing out a
> non-standard-layout object when value-initialization would produce the
> exact same result.  If you want to keep the almost_std_layout_p check
> for the non-zero memset case, that's fine.

Attached is an updated patch with the requested changes.  I've
also renamed the option -Wclass-memaccess to avoid suggesting
that the warning focuses solely on non-trivial types, and
updated its wording and comments throughout to refer to value
initialization rather than default initialization.  Retesting
exposed another problem recently introduced into dumplfile.c
so this revision of the patch also contains a fix for that.

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcc-80560.diff
Type: text/x-patch
Size: 55371 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20170524/c12d51c5/attachment.bin>