[PATCH] D79279: Add overloaded versions of builtin mem* functions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thanks, I'm happy with this approach.

If I understand correctly, the primary (perhaps sole) purpose of `__builtin_memcpy_sized` is to provide a primitive from which an atomic operation can be produced. That being the case, I wonder if the name is emphasizing the wrong thing, and a name that contains `atomic` would be more obvious. As a concrete alternative, `__atomic_unordered_memcpy` is not much longer and seems to have the right kinds of implications. WDYT?



================
Comment at: clang/docs/LanguageExtensions.rst:2395-2397
+Beyond C's specification for these functions, the above builtins can also be
+overloaded on non-default address spaces. Further, the following builtins can
+also be overloaded on ``volatile``:
----------------
"can also be overloaded" -> "are also overloaded" would be clearer I think. ("Can also be overloaded" would suggest to me that it's the user of the builtin who overloads them, perhaps by declaring overloads.)


================
Comment at: clang/docs/LanguageExtensions.rst:2403-2406
+Constant evaluation support is only provided when the source and destination are
+pointers to arrays with the same trivially copyable element type, and the given
+size is an exact multiple of the element size that is no greater than the number
+of elements accessible through the source and destination operands.
----------------
Is this true in general, or only for `[w]mem{cpy,move}`? I thought for the other cases, we required an array of `char` / `wchar_t`?


================
Comment at: clang/docs/LanguageExtensions.rst:2409
+Support for constant expression evaluation for the above builtins can be detected
+with ``__has_feature(cxx_constexpr_string_builtins)``.
+
----------------
I think this only applies to the above list minus the five functions you added to it. Given this and the previous comment, I'm not sure that merging the documentation on string builtins and memory builtins is working out well -- they seem to have more differences than similarities.

(`memset` is an outlier here that should be called out -- we don't seem to provide any constant evaluation support for it whatsoever.)


================
Comment at: clang/docs/LanguageExtensions.rst:2451
+lock-free size for the target architecture. It is undefined behavior to provide
+a memory locations which is aligned to less than the access size. It is
+undefined behavior to provide a size which is not evenly divided by the
----------------



================
Comment at: clang/docs/LanguageExtensions.rst:2452
+a memory locations which is aligned to less than the access size. It is
+undefined behavior to provide a size which is not evenly divided by the
+specified access size.
----------------



================
Comment at: clang/lib/AST/ExprConstant.cpp:8870
+        return false;
+      if (N.urem(ElSz.getLimitedValue()) != 0) {
+        Info.FFDiag(E, diag::note_constexpr_mem_sized_bad_size)
----------------
`getLimitedValue()` here seems unnecessary; `urem` can take an `APInt`.


================
Comment at: clang/lib/AST/ExprConstant.cpp:8872
+        Info.FFDiag(E, diag::note_constexpr_mem_sized_bad_size)
+            << (int)N.getLimitedValue() << (int)ElSz.getLimitedValue();
+        return false;
----------------
Consider using `toString` instead of truncating each `APSInt` to `uint64_t` then to `int`. The size might reliably fit into `uint64_t`, but I don't think we can assume that `int` is large enough.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:635
+    return ArgTy->castAs<clang::ObjCObjectPointerType>()->getPointeeType();
+  return ArgTy->castAs<clang::PointerType>()->getPointeeType();
+}
----------------
I'm not sure this `castAs` is necessarily correct. If the operand is C++11 `nullptr`, we could perform a null-to-pointer implicit conversion, and `ArgTy` could be `NullPtrTy` after stripping that back off here.

It seems like maybe what we want to do is strip off implicit conversions until we hit a non-pointer type, and take the pointee type we found immediately before that?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:5609
+    return ExprError(Diag(TheCall->getBeginLoc(),
+                          PDiag(diag::err_atomic_op_needs_trivial_copy))
+                     << DstValTy << DstOp->getSourceRange());
----------------
Generally, I'm a little uncomfortable about producing an error if a type is complete but allowing the construct if the type is incomplete -- that seems like a situation where a warning would be more appropriate to me. It's surprising and largely unprecedented that providing more information about a type would change the program from valid to invalid.

Do we really need the protection of an error here rather than an enabled-by-default warning? Moreover, don't we already have a warning for `memcpy` of a non-trivially-copyable object somewhere? If not, then I think we should add such a thing that also applies to the real `memcpy`, rather than only warning on the builtin.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:5732
+
+    if (!DstValTy.isTriviallyCopyableType(Context) && !DstValTy->isVoidType())
+      return ExprError(Diag(TheCall->getBeginLoc(),
----------------
jfb wrote:
> rsmith wrote:
> > You need to call `Sema::isCompleteType` first before asking this question, in order to trigger class instantiation when necessary in C++. (Likewise for the checks in the previous function.)
> Before the condition, right? LMK if I added the right thing!
It would be more correct from a modules perspective to use

```
if (isCompleteType(Loc, T) && !T.isTriviallyCopyableType(Context))
```

That way, if the definition of the type is in some loaded-but-not-imported module file, we'll treat it the same as if the definition of the type is entirely unknown. (That also removes the need to check for the `void` case.) But given that this only allows us to accept code that is wrong in some sense, I'm not sure it really matters much.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79279/new/

https://reviews.llvm.org/D79279