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

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Thanks for the detailed comments, I think I've addressed all of them! I also added UBSan support to check the builtin invocation. I think this patch is pretty much ready to go. A follow-up will need to add the support functions to compiler-rt (they're currently optional, as per https://reviews.llvm.org/D33240), and in cases where size is known we can inline them (as we do for memcpy and friends).



================
Comment at: clang/docs/LanguageExtensions.rst:2454
+and might be non-uniform throughout the operation.
+
+The builtins can be used as building blocks for different facilities:
----------------
rsmith wrote:
> From the above description, I think the documentation is unclear what the types `T` and `U` are used for. I think the answer is something like:
> 
> """
> The types `T` and `U` are required to be trivially-copyable types, and `byte_element_size` (if specified) must be a multiple of the size of both types. `dst` and `src` are expected to be suitably aligned for `T` and `U` objects, respectively.
> """
> 
> But... we actually get the alignment information by analyzing pointer argument rather than from the types, just like we do for `memcpy` and `memmove`, so maybe the latter part is not right. (What did you intend regarding alignment for the non-atomic case?) The trivial-copyability and divisibility checks don't seem fundamentally important to the goal of the builtin, so I wonder if we could actually just use `void` here and remove the extra checks. (I don't really have strong views one way or the other on this, except that we should either document what `T` and `U` are used for or make the builtins not care about the pointee type beyond its qualifiers.)
You're right. I've removed most treatment of `T` / `U`, and updated the documentation.

I left the trivial copy check, but `void*` is a usual escape hatch.

Divisibility is now only checked for `size` / `element_size`.


================
Comment at: clang/lib/AST/ExprConstant.cpp:8851
+      // about atomicity, but needs to check runtime constraints on size. We
+      // can't check the alignment runtime constraints.
+      APSInt ElSz;
----------------
rsmith wrote:
> We could use the same logic we use in `__builtin_is_aligned` here. For any object whose value the constant evaluator can reason about, we should be able to compute at least a minimal alignment (though the actual runtime alignment might of course be greater).
I think the runtime alignment is really the only thing that matters here. I played with constexpr checking based on what `__builtin_is_aligned` does, and it's not particularly useful IMO.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2639-2640
   case Builtin::BI__builtin_mempcpy: {
+    QualType DestTy = getPtrArgType(CGM, E, 0);
+    QualType SrcTy = getPtrArgType(CGM, E, 1);
     Address Dest = EmitPointerWithAlignment(E->getArg(0));
----------------
rsmith wrote:
> Looking through implicit conversions in `getPtrArgType` here will change the code we generate for cases like:
> 
> ```
> void f(volatile void *p, volatile void *q) {
>   memcpy(p, q, 4);
> }
> ```
> 
> ... (in C, where we permit such implicit conversions) to use a volatile memcpy intrinsic. Is that an intentional change?
I'm confused... what's the difference that this makes for the pre-existing builtins? My intent was to get the `QualType` unconditionally, but I can conditionalize it if needed... However this ought to make no difference:
```
static QualType getPtrArgType(CodeGenModule &CGM, const CallExpr *E,
                              unsigned ArgNo) {
  QualType ArgTy = E->getArg(ArgNo)->IgnoreImpCasts()->getType();
  if (ArgTy->isArrayType())
    return CGM.getContext().getAsArrayType(ArgTy)->getElementType();
  if (ArgTy->isObjCObjectPointerType())
    return ArgTy->castAs<clang::ObjCObjectPointerType>()->getPointeeType();
  return ArgTy->castAs<clang::PointerType>()->getPointeeType();
}
```
and indeed I can't see the example you provided change in IR from one to the other. The issue I'm working around is that getting it unconditionally would make ObjC code sad when `id` is passed in as I outlined above.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:5567
+          << (int)ElSz->getLimitedValue() << DstElSz << DstValTy
+          << DstOp->getSourceRange() << Arg->getSourceRange());
+
----------------
jfb wrote:
> 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.
They're gone, we now only check that size and element size match up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279