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

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

rsmith requested changes to this revision.
rsmith added a comment.
This revision now requires changes to proceed.

I think the documentation isn't quite right yet, but otherwise I think I'm happy. (With a couple of code change suggestions.)

In D79279#2240487 <https://reviews.llvm.org/D79279#2240487>, @jfb wrote:

> In D79279#2235085 <https://reviews.llvm.org/D79279#2235085>, @rsmith wrote:
>
>> 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?
>
> Kinda, that's the motivating from Hans' paper which I'm following. One other use case (and the reason I assume Azul folks want it too) is when there's a GC that looks at objects. With this it knows it won't see torn objects when the GC is concurrent. It's similar, but generally `atomic` also implies an ordering, and here it's explicitly unordered (i.e. Bring Your Own Fences).
>
> So I don't have a strong opinion on the name, but `atomic` seem slightly wrong.

I mean, I see your point, but I think not mentioning `atomic` at all also seems a little surprising, given how tied this feature is to atomic access (eg, rejecting access sizes larger than the inline atomic width). But this is not a hill I have any desire to die on :) If you'd prefer to keep the current name, I can live with it.



================
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.
----------------
jfb wrote:
> rsmith wrote:
> > 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`?
> This is just moving documentation that was below. Phab is confused with the diff.
The documentation that was moved applied to `memcpy`, `memmove`, `wmemcpy`, and `wmemmove`. In the new location, it doesn't apply to `wmemcpy` nor `wmemmove` but does now apply to `memchr`, `memcmp`, ..., for which it is incorrect.

We used to distinguish between "string builtins", which had the restriction to character types, and "memory builtins", which had the restriction to trivially-copyable types. Can you put that distinction back, or otherwise restore the wording to the old / correct state?


================
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)``.
+
----------------
jfb wrote:
> rsmith wrote:
> > 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.)
> [w]memset are indeed the odd ones, update says so.
This feature test macro doesn't cover `memcpy` / `memmove`; this documentation is incorrect for older versions of Clang. Clang 4-7 define this feature test macro but do not support constant evaluation of `memcpy` / `memmove`.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:636-643
+  if (ArgTy->isObjCObjectPointerType())
+    return ArgTy->castAs<clang::ObjCObjectPointerType>()
+        ->getPointeeType()
+        .isVolatileQualified();
+  if (ArgTy->isPointerType())
+    return ArgTy->castAs<clang::PointerType>()
+        ->getPointeeType()
----------------
(Just a simplification, NFC.)


================
Comment at: clang/lib/Sema/SemaChecking.cpp:5609
+    return ExprError(Diag(TheCall->getBeginLoc(),
+                          PDiag(diag::err_atomic_op_needs_trivial_copy))
+                     << DstValTy << DstOp->getSourceRange());
----------------
jfb wrote:
> rsmith wrote:
> > 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.
> That rationale makes sense, but it's pre-existing behavior for atomic. I can change all of them in a follow-up if that's OK?
> 
> We don't have such a check for other builtins. I can do a second follow-up to then adopt these warnings for them too?
The pre-existing behavior for atomic builtins is to reject if the type is incomplete. Eg;
```
<stdin>:1:33: error: address argument to atomic operation must be a pointer to a trivially-copyable type ('struct A *' invalid)
struct A; void f(struct A *p) { __atomic_store(p, p, 0); }
                                ^              ~
```
We should do the same here. (Though I'd suggest calling `RequireCompleteType` instead to get a more meaningful diagnostic.) These days I think we should check for unsized types too, eg:

```
if (RequireCompleteSizedType(ScrOp->getBeginLoc(), SrcValTy))
  return true;
if (!SrcValTy.isTriviallyCopyableType(Context) && !SrcValTy->isVoidType())
  return ExprError(...);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279