This is an archive of the discontinued LLVM Phabricator instance.

[ADT] Fix Optional ABI mismatch between clang & gcc
AbandonedPublic

Authored by kbenzie on Aug 14 2018, 8:39 AM.

Details

Summary

This patch addresses discussion in
https://bugs.llvm.org/show_bug.cgi?id=35978
and the email thread
https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20180122/519188.html

Compiling LLVM with either clang or gcc and a dependant project which
uses the opposite compiler results in an ABI incompatibility in Optional
due to the trivially copyable optimisation in the OptionalStorage type
being enabled when compiling with clang and disabled when GCC.

Additionally, this patch removes undefined behavior in the copy assignment
operator of OptionalStorage. When OptionalStorage does not hold a
valid object and the copy assignment operator is invoked, the object
being copied must be copy constructed in the uninitialized storage.buffer.

In the scenario of installing LLVM from the apt packages and a
downstream project choosing to use a different compiler the resulting
application will exhibit runtime failures such as segmentation faults
when calling LLVM functions with an Optional parameter crossing which
cross the ABI boundary.

I believe this is a serious bug and if this patch is accepted should
also be backported to the 7.0 release branch as this has been a latent
issue since late January 2018.

I did not have enough information to be able to reproduce the
"miscompiles" mentioned on the bug tracker so am unable to verify if
those have been fixed this with patch, it is my hope that fixing the
undefined behavior should resolve these issues.

Diff Detail

Event Timeline

kbenzie created this revision.Aug 14 2018, 8:39 AM
timshen added inline comments.Aug 20 2018, 10:01 AM
include/llvm/ADT/Optional.h
120

Will this suffice?
if (hasVal) { memcpy(&storage.buffer, &y.storage.buffer, sizeof(T)); }

timshen added inline comments.Aug 20 2018, 10:03 AM
include/llvm/ADT/Optional.h
120

Sorry, I meant

hasVal = y.hasVal;
if (hasVal) { memcpy(...); }
kbenzie added inline comments.Aug 21 2018, 2:58 AM
include/llvm/ADT/Optional.h
120

This assignment operator does not take a const OptionalStorage& so y.hasVal is not a valid expression since T is the type of the object to be copied into storage.buffer e.g. for OptionalStorage<int>
then T is int.

A previous attempt to fix the GCC miscompiles used memmove which was only reported to fix GCC 5.4 release builds.

The expression *reinterpret_cast<T *>(storage.buffer) = y is undefined behaviour in the case where a valid object is not held in storage.buffer according to C++ rules which lead to this implementation.

rsmith added a subscriber: rsmith.Oct 25 2018, 11:50 AM
rsmith added inline comments.
include/llvm/ADT/Optional.h
113

When the type is *actually* trivially-copyable (and not just isPodLike, which lies), we could just store it directly: union { T storage; };

122

This would be more correct as:

#ifdef __cpp_lib_launder
*std::launder(reinterpret_cast<T *>(&storage.buffer)) = y;
#else
*reinterpret_cast<T *>(std::addressof(storage.buffer)) = y;
#endif

(I'd expect the std::launder to also shut up the TBAA warnings.)

kbenzie abandoned this revision.Jan 15 2019, 2:51 AM