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

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

This is almost ready I think!
There are a few things still open, I'd love feedback on them.



================
Comment at: clang/docs/LanguageExtensions.rst:2435-2437
+* ``__builtin_memcpy_overloaded(QUAL void *dst, QUAL const void *src, size_t byte_size, size_t byte_element_size = <unspecified>)``
+* ``__builtin_memmove_overloaded(QUAL void *dst, QUAL const void *src, size_t byte_size, size_t byte_element_size = <unspecified>)``
+* ``__builtin_memset_overloaded(QUAL void *dst, unsigned char val, size_t byte_size, size_t byte_element_size = <unspecified>)``
----------------
rsmith wrote:
> rsmith wrote:
> > What happens if `byte_element_size` does not divide `byte_size`?
> Did you really mean `void*` here? I've been pretty confused by some of the stuff happening below that seems to depend on the actual type of the passed pointer, which would make more sense if you meant `QUAL T *` here rather than `QUAL void*`. Do the builtins do different things for different argument pointee types or not?
Runtime constraint violation. constexpr needs to catch this too, added. Though IIUC we can't actually check alignment in constexpr, which makes sense since there's no actual address.

Similarly, I think we ought to add UBSan builtin check for this. I think it makes sense to add as an option to `CreateElementUnorderedAtomicMemCpy`: either assert-check at compile-time (the current default, which triggers assertions as I've annotated in the tests' FIXME), or at runtime if the sanitizer is enabled. WDYT?

I've added these two to the documentation.


================
Comment at: clang/docs/LanguageExtensions.rst:2435-2437
+* ``__builtin_memcpy_overloaded(QUAL void *dst, QUAL const void *src, size_t byte_size, size_t byte_element_size = <unspecified>)``
+* ``__builtin_memmove_overloaded(QUAL void *dst, QUAL const void *src, size_t byte_size, size_t byte_element_size = <unspecified>)``
+* ``__builtin_memset_overloaded(QUAL void *dst, unsigned char val, size_t byte_size, size_t byte_element_size = <unspecified>)``
----------------
jfb wrote:
> rsmith wrote:
> > rsmith wrote:
> > > What happens if `byte_element_size` does not divide `byte_size`?
> > Did you really mean `void*` here? I've been pretty confused by some of the stuff happening below that seems to depend on the actual type of the passed pointer, which would make more sense if you meant `QUAL T *` here rather than `QUAL void*`. Do the builtins do different things for different argument pointee types or not?
> Runtime constraint violation. constexpr needs to catch this too, added. Though IIUC we can't actually check alignment in constexpr, which makes sense since there's no actual address.
> 
> Similarly, I think we ought to add UBSan builtin check for this. I think it makes sense to add as an option to `CreateElementUnorderedAtomicMemCpy`: either assert-check at compile-time (the current default, which triggers assertions as I've annotated in the tests' FIXME), or at runtime if the sanitizer is enabled. WDYT?
> 
> I've added these two to the documentation.
Oh yeah, this should be `T*` and `U*`. Fixed.

They used to key atomicity off of element size, but now that we have the extra parameter we only look at `T` and `U` for correctness (not behavior).


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8941-8943
+def err_atomic_builtin_ext_size_mismatches_el : Error<
+  "number of bytes to copy must be a multiple of pointer element size, "
+  "got %0 bytes to copy with element size %1 for %2">;
----------------
rsmith wrote:
> Presumably the number of bytes need not be a compile-time constant? It's a bit weird to produce an error rather than a warning on a case that would be valid but (perhaps?) UB if the argument were non-constant.
I commented below, indeed it seems like some of this ought to be relaxed.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:5567
+          << (int)ElSz->getLimitedValue() << DstElSz << DstValTy
+          << DstOp->getSourceRange() << Arg->getSourceRange());
+
----------------
I'm re-thinking these checks:
```
    if (ElSz->urem(DstElSz))
      return ExprError(
          Diag(TheCall->getBeginLoc(),
               PDiag(diag::err_atomic_builtin_ext_size_mismatches_el))
          << (int)ElSz->getLimitedValue() << DstElSz << DstValTy
          << DstOp->getSourceRange() << Arg->getSourceRange());
```
I'm not sure we ought to have them anymore. We know that the types are trivially copyable, it therefore doesn't really matter if you're copying with operations smaller than the type itself. For example:
```
struct Data {
  int a, b, c, d;
};
```
It ought to be fine to do 4-byte copies of `Data`, if whatever your algorithm is is happy with that. I therefore think I'll remove these checks based on the dst / src element types. The only thing that seems to make sense is making sure that you don't straddle object boundaries with element size.

I removed sizeless types: we'll codegen whatever you ask for.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279