This is the mail archive of the libstdc++@gcc.gnu.org mailing list for the libstdc++ project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Use __builtin_memmove for trivially copy assignable types


On 07/19/2018 08:39 AM, Jonathan Wakely wrote:
> On 19/07/18 10:01 -0400, Glen Fernandes wrote:
>> On Thu, Jul 19, 2018 at 9:25 AM Jonathan Wakely <jwakely@redhat.com>
>> wrote:
>>> Sorry for the delay in reviewing this properly, as I've only just
>>> realised that this introduces undefined behaviour, doesn't it?
>>>
>>> It's undefined to use memmove for a type that is not trivially
>>> copyable. All trivial types are trivially copyable, so __is_trivial
>>> was too conservative, but safe (IIRC we used it because there was no
>>> __is_trivially_copyable trait at the time, so __is_trivial was the
>>> best we had).
>>>
>>> There are types which are trivially assignable but not trivially
>>> copyable, and it's undefined to use memmove for such types.
>>
>> I was still unclear about that, but I forwarded you an e-mail from
>> Marshall with his answer when I asked whether libc++'s use of
>> TriviallyCopyAssignable here was incorrect. Let me know if it applies
>> here, and if not (and that interpretation of the standard is
>> incorrect), I'll update the patch to do as you suggest and run the
>> tests again.
> 
> While I sympathise with Marshall's position (that std::copy only cares
> about assignment not copying) that doesn't make it OK to use memmove
> here.
> 
> Using memmove for a non-trivially copyable type is undefined. Period.
> 
> The fact GCC warns that it's undefined also means GCC might start
> optimising based on the assumption that undefined behaviour isn't
> reached at runtime. So it could (for example) assume that the input
> range must be empty and remove the entire call to std::copy.
Right.  In fact we have a pass which searches for a very small subset of
undefined behavior (null pointer dereferences, division by zero) and
when it finds them it replaces the offending operation with a trap and
does the obvious CFG cleanups.

While neither of those would affect this specific issue and we're pretty
conservative about adding more cases to this pass, certainly the right
thing to do is avoid undefined behavior :-)  So I'm in total agreement
with Jon here.

jeff


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]