[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 , @jfb wrote:

> In 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()
+->getPointeeType()
+.isVolatileQualified();
+  if (ArgTy->isPointerType())
+return ArgTy->castAs()
+->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 
> > 

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

I went through all the comments here, plus looked at the code myself.  I 
believe all of the comments by other reviewers have been fixed/answered 
acceptably. I don't have any additional comments to add, therefore I think it 
is appropriate to accept this revision.

@jfb: Please give this a day or two before committing to give the other 
reviewers a chance to speak up!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D79279#2235085 , @rsmith wrote:

> 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?

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.

Follow-ups I suggest in comments:

- Make "must be trivially copyable" a warning throughout (not just here, but 
for atomics too).
- Implement that diagnostic for `mem*` functions.




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.

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.



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)``.
+

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.



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)

rsmith wrote:
> `getLimitedValue()` here seems unnecessary; `urem` can take an `APInt`.
Their bitwidth doesn't always match, and that asserts out.



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;

rsmith wrote:
> 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.
OK I updated 2 other places as well.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:635
+return ArgTy->castAs()->getPointeeType();
+  return ArgTy->castAs()->getPointeeType();
+}

rsmith wrote:
> 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?
Ah good catch! The new functions I'm adding just disallow nullptr, but the 
older ones allow it. I've modified the code accordingly and added a test in 
CodeGen for nullptr.



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:
> jfb wrote:
> > rsmith wrote:
> > > jfb wrote:
> > > > 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 

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

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 288131.
jfb marked 12 inline comments as done.
jfb added a comment.

Address Richard's comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/CodeGen/CGBuilder.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-memfns-nullptr.cpp
  clang/test/CodeGen/builtin-memfns.c
  clang/test/CodeGen/builtin-sized-memfns.c
  clang/test/CodeGen/ubsan-builtin-checks.c
  clang/test/CodeGen/ubsan-builtin-ctz-clz.c
  clang/test/CodeGen/ubsan-builtin-mem_sized.c
  clang/test/CodeGenObjC/builtin-memfns.m
  clang/test/Sema/builtin-sized-memfns.cpp
  clang/test/SemaCXX/constexpr-string.cpp
  compiler-rt/lib/ubsan/ubsan_handlers.cpp
  compiler-rt/lib/ubsan/ubsan_handlers.h
  compiler-rt/test/ubsan/TestCases/Misc/builtins-ctz-clz.cpp
  compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_sized.cpp
  compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp

Index: compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
===
--- compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
+++ /dev/null
@@ -1,35 +0,0 @@
-// REQUIRES: arch=x86_64
-//
-// RUN: %clangxx -fsanitize=builtin -w %s -O3 -o %t
-// RUN: %run %t 2>&1 | FileCheck %s --check-prefix=RECOVER
-// RUN: %clangxx -fsanitize=builtin -fno-sanitize-recover=builtin -w %s -O3 -o %t.abort
-// RUN: not %run %t.abort 2>&1 | FileCheck %s --check-prefix=ABORT
-
-void check_ctz(int n) {
-  // ABORT: builtins.cpp:[[@LINE+2]]:17: runtime error: passing zero to ctz(), which is not a valid argument
-  // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctz(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctzl(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctzll(n);
-}
-
-void check_clz(int n) {
-  // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clz(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clzl(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clzll(n);
-}
-
-int main() {
-  check_ctz(0);
-  check_clz(0);
-  return 0;
-}
Index: compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_sized.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_sized.cpp
@@ -0,0 +1,93 @@
+// RUN: %clangxx -fsanitize=builtin -w %s -O3 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+
+using uintptr_t = __UINTPTR_TYPE__;
+using size_t = __SIZE_TYPE__;
+
+void check_memcpy_align(char *dst_aligned, char *dst_misaligned, const char *src_aligned, const char *src_misaligned, size_t sz) {
+  // OK.
+  __builtin_memcpy_sized(dst_aligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_sized.cpp:[[@LINE+1]]:26: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_sized, element size 2
+  __builtin_memcpy_sized(dst_misaligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_sized.cpp:[[@LINE+1]]:39: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_sized, element size 2
+  __builtin_memcpy_sized(dst_aligned, src_misaligned, sz, 2);
+}
+
+void check_memmove_align(char *dst_aligned, char *dst_misaligned, const char *src_aligned, const char *src_misaligned, size_t sz) {
+  // OK.
+  __builtin_memmove_sized(dst_aligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_sized.cpp:[[@LINE+1]]:27: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_sized, element size 2
+  __builtin_memmove_sized(dst_misaligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_sized.cpp:[[@LINE+1]]:40: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_sized, element size 2
+  __builtin_memmove_sized(dst_aligned, src_misaligned, sz, 2);
+}
+
+void check_memset_align(char *dst_aligned, char *dst_misaligned, size_t sz) {
+  // OK.
+  __builtin_memset_sized(dst_aligned, 0, sz, 2);
+  // CHECK: builtins-mem_sized.cpp:[[@LINE+1]]:26: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_sized, element size 2
+  __builtin_memset_sized(dst_misaligned, 0, sz, 2);
+}
+
+void check_memcpy_size(char 

[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()->getPointeeType();
+  return ArgTy->castAs()->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 

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

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Ping, I think I've addressed all comments here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Actually I think any subsequent updates to tests can be done in a follow-up 
patch, since I'm not changing the status-quo on address space here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 285479.
jfb added a comment.

Fix a test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/CodeGen/CGBuilder.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-memfns.c
  clang/test/CodeGen/builtin-sized-memfns.c
  clang/test/CodeGen/ubsan-builtin-checks.c
  clang/test/CodeGen/ubsan-builtin-ctz-clz.c
  clang/test/CodeGen/ubsan-builtin-mem_sized.c
  clang/test/CodeGenObjC/builtin-memfns.m
  clang/test/Sema/builtin-sized-memfns.cpp
  clang/test/SemaCXX/constexpr-string.cpp
  compiler-rt/lib/ubsan/ubsan_handlers.cpp
  compiler-rt/lib/ubsan/ubsan_handlers.h
  compiler-rt/test/ubsan/TestCases/Misc/builtins-ctz-clz.cpp
  compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_sized.cpp
  compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp

Index: compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
===
--- compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
+++ /dev/null
@@ -1,35 +0,0 @@
-// REQUIRES: arch=x86_64
-//
-// RUN: %clangxx -fsanitize=builtin -w %s -O3 -o %t
-// RUN: %run %t 2>&1 | FileCheck %s --check-prefix=RECOVER
-// RUN: %clangxx -fsanitize=builtin -fno-sanitize-recover=builtin -w %s -O3 -o %t.abort
-// RUN: not %run %t.abort 2>&1 | FileCheck %s --check-prefix=ABORT
-
-void check_ctz(int n) {
-  // ABORT: builtins.cpp:[[@LINE+2]]:17: runtime error: passing zero to ctz(), which is not a valid argument
-  // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctz(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctzl(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctzll(n);
-}
-
-void check_clz(int n) {
-  // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clz(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clzl(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clzll(n);
-}
-
-int main() {
-  check_ctz(0);
-  check_clz(0);
-  return 0;
-}
Index: compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_sized.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_sized.cpp
@@ -0,0 +1,93 @@
+// RUN: %clangxx -fsanitize=builtin -w %s -O3 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+
+using uintptr_t = __UINTPTR_TYPE__;
+using size_t = __SIZE_TYPE__;
+
+void check_memcpy_align(char *dst_aligned, char *dst_misaligned, const char *src_aligned, const char *src_misaligned, size_t sz) {
+  // OK.
+  __builtin_memcpy_sized(dst_aligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_sized.cpp:[[@LINE+1]]:26: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_sized, element size 2
+  __builtin_memcpy_sized(dst_misaligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_sized.cpp:[[@LINE+1]]:39: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_sized, element size 2
+  __builtin_memcpy_sized(dst_aligned, src_misaligned, sz, 2);
+}
+
+void check_memmove_align(char *dst_aligned, char *dst_misaligned, const char *src_aligned, const char *src_misaligned, size_t sz) {
+  // OK.
+  __builtin_memmove_sized(dst_aligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_sized.cpp:[[@LINE+1]]:27: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_sized, element size 2
+  __builtin_memmove_sized(dst_misaligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_sized.cpp:[[@LINE+1]]:40: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_sized, element size 2
+  __builtin_memmove_sized(dst_aligned, src_misaligned, sz, 2);
+}
+
+void check_memset_align(char *dst_aligned, char *dst_misaligned, size_t sz) {
+  // OK.
+  __builtin_memset_sized(dst_aligned, 0, sz, 2);
+  // CHECK: builtins-mem_sized.cpp:[[@LINE+1]]:26: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_sized, element size 2
+  __builtin_memset_sized(dst_misaligned, 0, sz, 2);
+}
+
+void check_memcpy_size(char *dst, char *src) {
+  // OK.
+  __builtin_memcpy_sized(dst, src, 32, 2);
+  __builtin_memcpy_sized(dst, 

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

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 285414.
jfb added a comment.

Update overloading as discussed: on the original builtins, and separate the 
_sized variant, making its 4th parameter non-optional. If this looks good, I'll 
need to update codege for a few builtins (to handle volatile), as well as add 
tests for their codegen and address space (which should already work, but isn't 
tested).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/CodeGen/CGBuilder.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-memfns.c
  clang/test/CodeGen/builtin-sized-memfns.c
  clang/test/CodeGen/ubsan-builtin-checks.c
  clang/test/CodeGen/ubsan-builtin-ctz-clz.c
  clang/test/CodeGen/ubsan-builtin-mem_sized.c
  clang/test/CodeGenObjC/builtin-memfns.m
  clang/test/Sema/builtin-sized-memfns.cpp
  clang/test/SemaCXX/constexpr-string.cpp
  compiler-rt/lib/ubsan/ubsan_handlers.cpp
  compiler-rt/lib/ubsan/ubsan_handlers.h
  compiler-rt/test/ubsan/TestCases/Misc/builtins-ctz-clz.cpp
  compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_sized.cpp
  compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp

Index: compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
===
--- compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
+++ /dev/null
@@ -1,35 +0,0 @@
-// REQUIRES: arch=x86_64
-//
-// RUN: %clangxx -fsanitize=builtin -w %s -O3 -o %t
-// RUN: %run %t 2>&1 | FileCheck %s --check-prefix=RECOVER
-// RUN: %clangxx -fsanitize=builtin -fno-sanitize-recover=builtin -w %s -O3 -o %t.abort
-// RUN: not %run %t.abort 2>&1 | FileCheck %s --check-prefix=ABORT
-
-void check_ctz(int n) {
-  // ABORT: builtins.cpp:[[@LINE+2]]:17: runtime error: passing zero to ctz(), which is not a valid argument
-  // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctz(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctzl(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctzll(n);
-}
-
-void check_clz(int n) {
-  // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clz(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clzl(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clzll(n);
-}
-
-int main() {
-  check_ctz(0);
-  check_clz(0);
-  return 0;
-}
Index: compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_sized.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_sized.cpp
@@ -0,0 +1,93 @@
+// RUN: %clangxx -fsanitize=builtin -w %s -O3 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+
+using uintptr_t = __UINTPTR_TYPE__;
+using size_t = __SIZE_TYPE__;
+
+void check_memcpy_align(char *dst_aligned, char *dst_misaligned, const char *src_aligned, const char *src_misaligned, size_t sz) {
+  // OK.
+  __builtin_memcpy_sized(dst_aligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_sized.cpp:[[@LINE+1]]:26: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_sized, element size 2
+  __builtin_memcpy_sized(dst_misaligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_sized.cpp:[[@LINE+1]]:39: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_sized, element size 2
+  __builtin_memcpy_sized(dst_aligned, src_misaligned, sz, 2);
+}
+
+void check_memmove_align(char *dst_aligned, char *dst_misaligned, const char *src_aligned, const char *src_misaligned, size_t sz) {
+  // OK.
+  __builtin_memmove_sized(dst_aligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_sized.cpp:[[@LINE+1]]:27: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_sized, element size 2
+  __builtin_memmove_sized(dst_misaligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_sized.cpp:[[@LINE+1]]:40: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_sized, element size 2
+  __builtin_memmove_sized(dst_aligned, src_misaligned, sz, 2);
+}
+
+void check_memset_align(char *dst_aligned, char *dst_misaligned, size_t sz) {
+  // OK.
+  __builtin_memset_sized(dst_aligned, 0, sz, 2);
+  // CHECK: 

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

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D79279#2201484 , @rsmith wrote:

> I think it would be reasonable in general to guarantee that our `__builtin_` 
> functions have contracts at least as wide as the underlying C function, but 
> allow them to have extensions, and to keep the plain C functions unextended. 
> I had actually thought we already did that in more cases than we do (but 
> perhaps I was thinking about the LLVM math intrinsics that guarantee to not 
> set `errno`). That would mean that a C standard library implementation is 
> still free to `#define foo(x,y,z) __builtin_foo(x,y,z)`, but if they do, they 
> may pick up extensions.

Alright, how about I go with what you say here, and instead of adding 
`__builtin_*_overloaded` versions I just overload the `__builtin_*` variants? 
This includes having an optional 4th parameter for access size.
Alternatively, I could overload  `__builtin_*`, but have a separate set of 
functions (say  `__builtin_*_sized`) for the atomic access size variants.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

rsmith added a comment.

In D79279#2201136 , @rjmccall wrote:

> In D79279#2200916 , @rsmith wrote:
>
>> In D79279#2197176 , @rjmccall wrote:
>>
>>> I thought part of the point of `__builtin_memcpy` was so that C library 
>>> headers could do `#define memcpy(x, y, z) __builtin_memcpy(x, y, z)`.  If 
>>> so, the conformance issue touches `__builtin_memcpy` as well, not just 
>>> calls to the library builtin.

For what it's worth, giving `__builtin_memcpy` broader semantics than `memcpy` 
wouldn't be unprecedented: it's already the case that `__builtin_memcpy` is 
usable in constant expressions where plain `memcpy` is not (but there's no 
macro risk in that case at least since C++ `memcpy` can't be a macro, and a 
call to memcpy is never an ICE in C).

>> They would have to declare it as well (because C code can `#undef memcpy` 
>> and expect to then be able to call a real function), so the `#define` would 
>> be pointless.
>
> It wouldn't be pointless; it would enable optimization of direct calls to 
> memcpy (the 99% case) without the compiler having to special-case a function 
> by name.

I mean, yes, in an environment where the compiler didn't special-case the 
function by name anyway it wouldn't be pointless. I'm not aware of any such 
environment in popular usage, but that could just be ignorance on my part.

> If we just want memcpy to do the right thing when called directly, that's not 
> ridiculous.  I don't think it would actually have any conformance problems: a 
> volatile memcpy is just a less optimizable memcpy, and to the extent that an 
> address-space-aware memcpy is different from the standard definition, it's 
> probably UB to use the standard definition to copy memory in non-generic 
> address spaces.

I think this is a constraint violation in C; C11 6.5.2.2/2 (a "Constraints" 
paragraph) requires that "Each argument shall have a type such that its value 
may be assigned to an object with the unqualified version of the type of its 
corresponding parameter." So this would be an extension, not just defining UB, 
and seems like the kind of extension we'd normally diagnose under 
`-pedantic-errors`, but we could make an exception. I also think it'd be mildly 
surprising for an explicitly-declared function to allow different argument 
conversions depending on whether its name is `memcpy`.

It seems to me that you're not entirely comfortable with making `memcpy` and 
`__builtin_memcpy` differ in this way, and I'm not entirely comfortable with 
making `memcpy`'s behavior differ from its declared type. Meanwhile, 
@tstellar's patch wants `__builtin_memcpy` to be overloaded on address space. I 
don't see a way to address all three concerns at once.

I think it would be reasonable in general to guarantee that our `__builtin_` 
functions have contracts at least as wide as the underlying C function, but 
allow them to have extensions, and to keep the plain C functions unextended. I 
had actually thought we already did that in more cases than we do (but perhaps 
I was thinking about the LLVM math intrinsics that guarantee to not set 
`errno`). That would mean that a C standard library implementation is still 
free to `#define foo(x,y,z) __builtin_foo(x,y,z)`, but if they do, they may 
pick up extensions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D79279#2200916 , @rsmith wrote:

> In D79279#2197176 , @rjmccall wrote:
>
>> I thought part of the point of `__builtin_memcpy` was so that C library 
>> headers could do `#define memcpy(x, y, z) __builtin_memcpy(x, y, z)`.  If 
>> so, the conformance issue touches `__builtin_memcpy` as well, not just calls 
>> to the library builtin.
>
> They would have to declare it as well (because C code can `#undef memcpy` and 
> expect to then be able to call a real function), so the `#define` would be 
> pointless.

It wouldn't be pointless; it would enable optimization of direct calls to 
memcpy (the 99% case) without the compiler having to special-case a function by 
name.  And you don't need an `#undef`, since `` doesn't trigger the 
preprocessor when `memcpy` is a function-like macro.  I seem to remember this 
being widely used in some math libraries, but it's entirely possible that it's 
never been used for `memcpy` and the like.  It's also entirely possible that 
I'm passing around folklore.

If we just want memcpy to do the right thing when called directly, that's not 
ridiculous.  I don't think it would actually have any conformance problems: a 
volatile memcpy is just a less optimizable memcpy, and to the extent that an 
address-space-aware memcpy is different from the standard definition, it's 
probably UB to use the standard definition to copy memory in non-generic 
address spaces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.

In D79279#2200916 , @rsmith wrote:

> In D79279#2197176 , @rjmccall wrote:
>
>> I thought part of the point of `__builtin_memcpy` was so that C library 
>> headers could do `#define memcpy(x, y, z) __builtin_memcpy(x, y, z)`.  If 
>> so, the conformance issue touches `__builtin_memcpy` as well, not just calls 
>> to the library builtin.
>
> They would have to declare it as well (because C code can `#undef memcpy` and 
> expect to then be able to call a real function), so the `#define` would be 
> pointless.  It doesn't look like glibc does anything like this; do you know 
> of a C standard library implementation that does?
>
> If we want to follow that path, then we'll presumably (eventually) want 
> address-space-`_overloaded` versions of all lib builtins that take pointers 
> -- looks like that's around 60 functions total. That said, I do wonder how 
> many of the functions in question that we're implicitly overloading on 
> address space actually support such overloading -- certainly any of them that 
> we lower to a call to a library function is going to go wrong at runtime.
>
> +@tstellar, who added this functionality in r233706 -- what was the intent 
> here?

The goal of this patch was to avoid having to overload all the builtin with 
address spaces, which would be a lot of new builtins, but this functionality 
was added for targets that do not have a memcpy lib call, so I didn't consider 
the case where a libcall would be emitted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

rsmith added a comment.

In D79279#2197176 , @rjmccall wrote:

> I thought part of the point of `__builtin_memcpy` was so that C library 
> headers could do `#define memcpy(x, y, z) __builtin_memcpy(x, y, z)`.  If so, 
> the conformance issue touches `__builtin_memcpy` as well, not just calls to 
> the library builtin.

They would have to declare it as well (because C code can `#undef memcpy` and 
expect to then be able to call a real function), so the `#define` would be 
pointless.  It doesn't look like glibc does anything like this; do you know of 
a C standard library implementation that does?

If we want to follow that path, then we'll presumably (eventually) want 
address-space-`_overloaded` versions of all lib builtins that take pointers -- 
looks like that's around 60 functions total. That said, I do wonder how many of 
the functions in question that we're implicitly overloading on address space 
actually support such overloading -- certainly any of them that we lower to a 
call to a library function is going to go wrong at runtime.

+@tstellar, who added this functionality in r233706 -- what was the intent here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 283406.
jfb marked 5 inline comments as done.
jfb added a comment.

Remove restrict, update docs, call isCompleteType


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/CodeGen/CGBuilder.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-overloaded-memfns.c
  clang/test/CodeGen/ubsan-builtin-checks.c
  clang/test/CodeGen/ubsan-builtin-ctz-clz.c
  clang/test/CodeGen/ubsan-builtin-mem_overloaded.c
  clang/test/CodeGenObjC/builtin-memfns.m
  clang/test/Sema/builtin-overloaded-memfns.cpp
  clang/test/SemaCXX/constexpr-string.cpp
  compiler-rt/lib/ubsan/ubsan_handlers.cpp
  compiler-rt/lib/ubsan/ubsan_handlers.h
  compiler-rt/test/ubsan/TestCases/Misc/builtins-ctz-clz.cpp
  compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_overloaded.cpp
  compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp

Index: compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
===
--- compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
+++ /dev/null
@@ -1,35 +0,0 @@
-// REQUIRES: arch=x86_64
-//
-// RUN: %clangxx -fsanitize=builtin -w %s -O3 -o %t
-// RUN: %run %t 2>&1 | FileCheck %s --check-prefix=RECOVER
-// RUN: %clangxx -fsanitize=builtin -fno-sanitize-recover=builtin -w %s -O3 -o %t.abort
-// RUN: not %run %t.abort 2>&1 | FileCheck %s --check-prefix=ABORT
-
-void check_ctz(int n) {
-  // ABORT: builtins.cpp:[[@LINE+2]]:17: runtime error: passing zero to ctz(), which is not a valid argument
-  // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctz(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctzl(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctzll(n);
-}
-
-void check_clz(int n) {
-  // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clz(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clzl(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clzll(n);
-}
-
-int main() {
-  check_ctz(0);
-  check_clz(0);
-  return 0;
-}
Index: compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_overloaded.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_overloaded.cpp
@@ -0,0 +1,93 @@
+// RUN: %clangxx -fsanitize=builtin -w %s -O3 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+
+using uintptr_t = __UINTPTR_TYPE__;
+using size_t = __SIZE_TYPE__;
+
+void check_memcpy_align(char *dst_aligned, char *dst_misaligned, const char *src_aligned, const char *src_misaligned, size_t sz) {
+  // OK.
+  __builtin_memcpy_overloaded(dst_aligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:31: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memcpy_overloaded(dst_misaligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:44: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memcpy_overloaded(dst_aligned, src_misaligned, sz, 2);
+}
+
+void check_memmove_align(char *dst_aligned, char *dst_misaligned, const char *src_aligned, const char *src_misaligned, size_t sz) {
+  // OK.
+  __builtin_memmove_overloaded(dst_aligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:32: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memmove_overloaded(dst_misaligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:45: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memmove_overloaded(dst_aligned, src_misaligned, sz, 2);
+}
+
+void check_memset_align(char *dst_aligned, char *dst_misaligned, size_t sz) {
+  // OK.
+  __builtin_memset_overloaded(dst_aligned, 0, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:31: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  

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

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D79279#2197118 , @rsmith wrote:

> Two observations that are new to me in this review:
>
> 1. We already treat all builtins as being overloaded on address space.
> 2. The revised patch treats `__builtin_mem*_overloaded` as being overloaded 
> *only* on address space, volatility, and atomicity. (We've tuned the design 
> to a point where the other qualifiers don't matter any more.)
>
> So, we're adding three features here: overloading on (a) address space, (b) 
> volatility, and (c) atomicity. (a) is already available in the 
> non-`_overloaded` form, and we seem to be approaching agreement that (b) 
> should be available in the non-`_overloaded` form too. So that only leaves 
> (c), which is really not `_overloaded` but `_atomic`.
>
> Based on those observations I'm now thinking that we might prefer a somewhat 
> different approach (but one that should require only minimal changes to the 
> patch in front of us). Specifically:
>
> 1. Stop treating lib builtins (eg, plain `memcpy`) as overloaded on address 
> space. That's a (pre-existing) conformance bug, at least for the Embedded C 
> TR.
> 2. Keep `__builtin_` forms of lib builtins overloaded on address space. (No 
> change.)
> 3. Also overload `__builtin_` forms of lib builtins on volatility where it 
> makes sense, instead of adding new builtin names `__builtin_mem*_overloaded`.
> 4. Add a new name for the builtin for the atomic forms of `memcpy` and 
> `memset` (`__builtin_memcpy_unordered_atomic` maybe?).
> 5. Convert the "trivial types" check from an error to a warning and apply it 
> to all the mem* overloads. (Though I think we might actually already have 
> such a check, so this might only require extending it to cover the atomic 
> builtin.)
>
> What do you think?

That's fine with me, but as John noted that's inconsistent with what he thought 
builtins allowed (i.e. `#define memcpy(dst, src, sz) __builtin_memcpy(dst, src, 
sz)`. If that's Not A Thing then your plan works. If it is then we need to tune 
it a bit.

Also note that the `_atomic` builtin also needs to support some overloading, at 
least for address spaces (and maybe `volatile` in the future).

So, let me know what you'd both rather see, so I don't ping-pong code too much.




Comment at: clang/docs/LanguageExtensions.rst:2434
+* ``volatile``
+* ``restrict``
+* ``__unaligned``

rsmith wrote:
> Does `restrict` really make sense here? It seems like the only difference it 
> could possibly make would be to treat `memcpy` as `memmove` if either operand 
> is marked restrict, but
> (a) if the caller wants that, they can just use `memcpy` directly, and
> (b) it's not correct to propagate restrict-ness from the caller to the callee 
> anyway, because restrict-ness is really a property of the declaration of the 
> identifier in its scope, not a property of its type:
> ```
> void f(void *restrict p) {
>   __builtin_memmove(p, p + 1, 4);
> }
> ```
> (c) a restrict qualifier on the pointee type is irrelevant to memcpy and a 
> restrict qualifier on the pointer type isn't part of QUAL.
I dropped `restrict`.



Comment at: clang/docs/LanguageExtensions.rst:2435
+* ``restrict``
+* ``__unaligned``
+* non-default address spaces

rsmith wrote:
> I don't think ``__unaligned`` matters any more. We always take the actual 
> alignment inferred from the pointer arguments, just like we do for 
> non-overloaded `memcpy`.
It's still allowed as a qualifier, though.



Comment at: clang/lib/Sema/SemaChecking.cpp:5732
+
+if (!DstValTy.isTriviallyCopyableType(Context) && !DstValTy->isVoidType())
+  return ExprError(Diag(TheCall->getBeginLoc(),

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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: clang/docs/LanguageExtensions.rst:2446
+order in which they occur (and in which they are observable) can only be
+guaranteed using appropriate fences around the function call. Element size must
+therefore be a lock-free size for the target architecture. It is a runtime

rjmccall wrote:
> jfb wrote:
> > rjmccall wrote:
> > > "*The* element size must..."  But I would suggest using "access size" 
> > > consistently rather than "element size".
> > I'm being consistent with the naming for IR, which uses "element" as well. 
> > I'm not enamored with the naming, but wanted to point out the purposeful 
> > consistency to make sure you preferred "access size". Without precedent I 
> > would indeed prefer "access size", but have a slight preference for 
> > consistency here. This is extremely weakly held preference.
> > 
> > (I fixed "the").
> IR naming is generally random fluff plucked from the mind of an inspired 
> compiler engineer.  User documentation is the point where we're supposed to 
> put our bad choices behind us and do something that makes sense to users. :)
"access size" it is :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 283402.
jfb marked an inline comment as done.
jfb added a comment.

Use 'access size' instead of 'element size'.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/CodeGen/CGBuilder.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-overloaded-memfns.c
  clang/test/CodeGen/ubsan-builtin-checks.c
  clang/test/CodeGen/ubsan-builtin-ctz-clz.c
  clang/test/CodeGen/ubsan-builtin-mem_overloaded.c
  clang/test/CodeGenObjC/builtin-memfns.m
  clang/test/Sema/builtin-overloaded-memfns.cpp
  clang/test/SemaCXX/constexpr-string.cpp
  compiler-rt/lib/ubsan/ubsan_handlers.cpp
  compiler-rt/lib/ubsan/ubsan_handlers.h
  compiler-rt/test/ubsan/TestCases/Misc/builtins-ctz-clz.cpp
  compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_overloaded.cpp
  compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp

Index: compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
===
--- compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
+++ /dev/null
@@ -1,35 +0,0 @@
-// REQUIRES: arch=x86_64
-//
-// RUN: %clangxx -fsanitize=builtin -w %s -O3 -o %t
-// RUN: %run %t 2>&1 | FileCheck %s --check-prefix=RECOVER
-// RUN: %clangxx -fsanitize=builtin -fno-sanitize-recover=builtin -w %s -O3 -o %t.abort
-// RUN: not %run %t.abort 2>&1 | FileCheck %s --check-prefix=ABORT
-
-void check_ctz(int n) {
-  // ABORT: builtins.cpp:[[@LINE+2]]:17: runtime error: passing zero to ctz(), which is not a valid argument
-  // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctz(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctzl(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctzll(n);
-}
-
-void check_clz(int n) {
-  // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clz(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clzl(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clzll(n);
-}
-
-int main() {
-  check_ctz(0);
-  check_clz(0);
-  return 0;
-}
Index: compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_overloaded.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_overloaded.cpp
@@ -0,0 +1,93 @@
+// RUN: %clangxx -fsanitize=builtin -w %s -O3 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+
+using uintptr_t = __UINTPTR_TYPE__;
+using size_t = __SIZE_TYPE__;
+
+void check_memcpy_align(char *dst_aligned, char *dst_misaligned, const char *src_aligned, const char *src_misaligned, size_t sz) {
+  // OK.
+  __builtin_memcpy_overloaded(dst_aligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:31: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memcpy_overloaded(dst_misaligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:44: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memcpy_overloaded(dst_aligned, src_misaligned, sz, 2);
+}
+
+void check_memmove_align(char *dst_aligned, char *dst_misaligned, const char *src_aligned, const char *src_misaligned, size_t sz) {
+  // OK.
+  __builtin_memmove_overloaded(dst_aligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:32: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memmove_overloaded(dst_misaligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:45: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memmove_overloaded(dst_aligned, src_misaligned, sz, 2);
+}
+
+void check_memset_align(char *dst_aligned, char *dst_misaligned, size_t sz) {
+  // OK.
+  __builtin_memset_overloaded(dst_aligned, 0, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:31: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  

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

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: compiler-rt/lib/ubsan/ubsan_handlers.cpp:659
+ uptr PtrOrSize) {
   GET_REPORT_OPTIONS(true);
+  handleInvalidBuiltin(Data, Opts, PtrOrSize);

vsk wrote:
> It looks like `__ubsan_handle_invalid_builtin` is meant to be recoverable, so 
> I think this should be `GET_REPORT_OPTIONS(false)`. Marking this 
> unrecoverable makes it impossible to suppress redundant diagnostics at the 
> same source location. It looks this isn't code you've added: feel free to 
> punt this to me. If you don't mind folding in a fix, adding a test would be 
> simple (perform UB in a loop and verify only one diagnostic is printed).
I folded this into the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 283400.
jfb marked an inline comment as done.
jfb added a comment.

Add loop test requested by Vedant


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/CodeGen/CGBuilder.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-overloaded-memfns.c
  clang/test/CodeGen/ubsan-builtin-checks.c
  clang/test/CodeGen/ubsan-builtin-ctz-clz.c
  clang/test/CodeGen/ubsan-builtin-mem_overloaded.c
  clang/test/CodeGenObjC/builtin-memfns.m
  clang/test/Sema/builtin-overloaded-memfns.cpp
  clang/test/SemaCXX/constexpr-string.cpp
  compiler-rt/lib/ubsan/ubsan_handlers.cpp
  compiler-rt/lib/ubsan/ubsan_handlers.h
  compiler-rt/test/ubsan/TestCases/Misc/builtins-ctz-clz.cpp
  compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_overloaded.cpp
  compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp

Index: compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
===
--- compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
+++ /dev/null
@@ -1,35 +0,0 @@
-// REQUIRES: arch=x86_64
-//
-// RUN: %clangxx -fsanitize=builtin -w %s -O3 -o %t
-// RUN: %run %t 2>&1 | FileCheck %s --check-prefix=RECOVER
-// RUN: %clangxx -fsanitize=builtin -fno-sanitize-recover=builtin -w %s -O3 -o %t.abort
-// RUN: not %run %t.abort 2>&1 | FileCheck %s --check-prefix=ABORT
-
-void check_ctz(int n) {
-  // ABORT: builtins.cpp:[[@LINE+2]]:17: runtime error: passing zero to ctz(), which is not a valid argument
-  // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctz(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctzl(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctzll(n);
-}
-
-void check_clz(int n) {
-  // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clz(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clzl(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clzll(n);
-}
-
-int main() {
-  check_ctz(0);
-  check_clz(0);
-  return 0;
-}
Index: compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_overloaded.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_overloaded.cpp
@@ -0,0 +1,93 @@
+// RUN: %clangxx -fsanitize=builtin -w %s -O3 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+
+using uintptr_t = __UINTPTR_TYPE__;
+using size_t = __SIZE_TYPE__;
+
+void check_memcpy_align(char *dst_aligned, char *dst_misaligned, const char *src_aligned, const char *src_misaligned, size_t sz) {
+  // OK.
+  __builtin_memcpy_overloaded(dst_aligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:31: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memcpy_overloaded(dst_misaligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:44: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memcpy_overloaded(dst_aligned, src_misaligned, sz, 2);
+}
+
+void check_memmove_align(char *dst_aligned, char *dst_misaligned, const char *src_aligned, const char *src_misaligned, size_t sz) {
+  // OK.
+  __builtin_memmove_overloaded(dst_aligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:32: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memmove_overloaded(dst_misaligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:45: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memmove_overloaded(dst_aligned, src_misaligned, sz, 2);
+}
+
+void check_memset_align(char *dst_aligned, char *dst_misaligned, size_t sz) {
+  // OK.
+  __builtin_memset_overloaded(dst_aligned, 0, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:31: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memset_overloaded(dst_misaligned, 0, sz, 

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

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 283390.
jfb added a comment.

Do UBSan change suggested by Vedant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/CodeGen/CGBuilder.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-overloaded-memfns.c
  clang/test/CodeGen/ubsan-builtin-checks.c
  clang/test/CodeGen/ubsan-builtin-ctz-clz.c
  clang/test/CodeGen/ubsan-builtin-mem_overloaded.c
  clang/test/CodeGenObjC/builtin-memfns.m
  clang/test/Sema/builtin-overloaded-memfns.cpp
  clang/test/SemaCXX/constexpr-string.cpp
  compiler-rt/lib/ubsan/ubsan_handlers.cpp
  compiler-rt/lib/ubsan/ubsan_handlers.h
  compiler-rt/test/ubsan/TestCases/Misc/builtins-ctz-clz.cpp
  compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_overloaded.cpp
  compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp

Index: compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
===
--- compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
+++ /dev/null
@@ -1,35 +0,0 @@
-// REQUIRES: arch=x86_64
-//
-// RUN: %clangxx -fsanitize=builtin -w %s -O3 -o %t
-// RUN: %run %t 2>&1 | FileCheck %s --check-prefix=RECOVER
-// RUN: %clangxx -fsanitize=builtin -fno-sanitize-recover=builtin -w %s -O3 -o %t.abort
-// RUN: not %run %t.abort 2>&1 | FileCheck %s --check-prefix=ABORT
-
-void check_ctz(int n) {
-  // ABORT: builtins.cpp:[[@LINE+2]]:17: runtime error: passing zero to ctz(), which is not a valid argument
-  // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctz(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctzl(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctzll(n);
-}
-
-void check_clz(int n) {
-  // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clz(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clzl(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clzll(n);
-}
-
-int main() {
-  check_ctz(0);
-  check_clz(0);
-  return 0;
-}
Index: compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_overloaded.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_overloaded.cpp
@@ -0,0 +1,93 @@
+// RUN: %clangxx -fsanitize=builtin -w %s -O3 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+
+using uintptr_t = __UINTPTR_TYPE__;
+using size_t = __SIZE_TYPE__;
+
+void check_memcpy_align(char *dst_aligned, char *dst_misaligned, const char *src_aligned, const char *src_misaligned, size_t sz) {
+  // OK.
+  __builtin_memcpy_overloaded(dst_aligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:31: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memcpy_overloaded(dst_misaligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:44: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memcpy_overloaded(dst_aligned, src_misaligned, sz, 2);
+}
+
+void check_memmove_align(char *dst_aligned, char *dst_misaligned, const char *src_aligned, const char *src_misaligned, size_t sz) {
+  // OK.
+  __builtin_memmove_overloaded(dst_aligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:32: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memmove_overloaded(dst_misaligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:45: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memmove_overloaded(dst_aligned, src_misaligned, sz, 2);
+}
+
+void check_memset_align(char *dst_aligned, char *dst_misaligned, size_t sz) {
+  // OK.
+  __builtin_memset_overloaded(dst_aligned, 0, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:31: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memset_overloaded(dst_misaligned, 0, sz, 2);
+}
+
+void 

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I thought part of the point of `__builtin_memcpy` was so that C library headers 
could do `#define memcpy(x, y, z) __builtin_memcpy(x, y, z)`.  If so, the 
conformance issue touches `__builtin_memcpy` as well, not just calls to the 
library builtin.

If that's not true, or if we're willing to ignore it, I agree that making 
`__builtin_memcpy` do the right thing for qualifiers in general is the right 
thing to do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

rsmith added a comment.

Two observations that are new to me in this review:

1. We already treat all builtins as being overloaded on address space.
2. The revised patch treats `__builtin_mem*_overloaded` as being overloaded 
*only* on address space, volatility, and atomicity. (We've tuned the design to 
a point where the other qualifiers don't matter any more.)

So, we're adding three features here: overloading on (a) address space, (b) 
volatility, and (c) atomicity. (a) is already available in the 
non-`_overloaded` form, and we seem to be approaching agreement that (b) should 
be available in the non-`_overloaded` form too. So that only leaves (c), which 
is really not `_overloaded` but `_atomic`.

Based on those observations I'm now thinking that we might prefer a somewhat 
different approach (but one that should require only minimal changes to the 
patch in front of us). Specifically:

1. Stop treating lib builtins (eg, plain `memcpy`) as overloaded on address 
space. That's a (pre-existing) conformance bug, at least for the Embedded C TR.
2. Keep `__builtin_` forms of lib builtins overloaded on address space. (No 
change.)
3. Also overload `__builtin_` forms of lib builtins on volatility where it 
makes sense, instead of adding new builtin names `__builtin_mem*_overloaded`.
4. Add a new name for the builtin for the atomic forms of `memcpy` and `memset` 
(`__builtin_memcpy_unordered_atomic` maybe?).
5. Convert the "trivial types" check from an error to a warning and apply it to 
all the mem* overloads. (Though I think we might actually already have such a 
check, so this might only require extending it to cover the atomic builtin.)

What do you think?




Comment at: clang/docs/LanguageExtensions.rst:2434
+* ``volatile``
+* ``restrict``
+* ``__unaligned``

Does `restrict` really make sense here? It seems like the only difference it 
could possibly make would be to treat `memcpy` as `memmove` if either operand 
is marked restrict, but
(a) if the caller wants that, they can just use `memcpy` directly, and
(b) it's not correct to propagate restrict-ness from the caller to the callee 
anyway, because restrict-ness is really a property of the declaration of the 
identifier in its scope, not a property of its type:
```
void f(void *restrict p) {
  __builtin_memmove(p, p + 1, 4);
}
```
(c) a restrict qualifier on the pointee type is irrelevant to memcpy and a 
restrict qualifier on the pointer type isn't part of QUAL.



Comment at: clang/docs/LanguageExtensions.rst:2435
+* ``restrict``
+* ``__unaligned``
+* non-default address spaces

I don't think ``__unaligned`` matters any more. We always take the actual 
alignment inferred from the pointer arguments, just like we do for 
non-overloaded `memcpy`.



Comment at: clang/docs/LanguageExtensions.rst:2455-2456
+
+The overloaded builtins expect both destination and source to be trivially
+copyable types.
+





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));

jfb wrote:
> rsmith wrote:
> > jfb wrote:
> > > 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 , 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()->getPointeeType();
> > >   return ArgTy->castAs()->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.
> > The example I gave should produce a non-volatile memcpy, and used to do so 
> > (we passed `false` as the fourth parameter to `CreateMemCpy`). With this 
> > patch, `getPtrArgType`will strip off the implicit conversion from `volatile 
> > void*` to `void*` in the 

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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/docs/LanguageExtensions.rst:2446
+order in which they occur (and in which they are observable) can only be
+guaranteed using appropriate fences around the function call. Element size must
+therefore be a lock-free size for the target architecture. It is a runtime

jfb wrote:
> rjmccall wrote:
> > "*The* element size must..."  But I would suggest using "access size" 
> > consistently rather than "element size".
> I'm being consistent with the naming for IR, which uses "element" as well. 
> I'm not enamored with the naming, but wanted to point out the purposeful 
> consistency to make sure you preferred "access size". Without precedent I 
> would indeed prefer "access size", but have a slight preference for 
> consistency here. This is extremely weakly held preference.
> 
> (I fixed "the").
IR naming is generally random fluff plucked from the mind of an inspired 
compiler engineer.  User documentation is the point where we're supposed to put 
our bad choices behind us and do something that makes sense to users. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Vedant Kumar via Phabricator via cfe-commits]

vsk added inline comments.



Comment at: compiler-rt/lib/ubsan/ubsan_handlers.cpp:659
+ uptr PtrOrSize) {
   GET_REPORT_OPTIONS(true);
+  handleInvalidBuiltin(Data, Opts, PtrOrSize);

It looks like `__ubsan_handle_invalid_builtin` is meant to be recoverable, so I 
think this should be `GET_REPORT_OPTIONS(false)`. Marking this unrecoverable 
makes it impossible to suppress redundant diagnostics at the same source 
location. It looks this isn't code you've added: feel free to punt this to me. 
If you don't mind folding in a fix, adding a test would be simple (perform UB 
in a loop and verify only one diagnostic is printed).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D79279#2195299 , @rjmccall wrote:

> Patch looks basically okay to me, although I'll second Richard's concern that 
> we shouldn't absent-mindedly start producing overloaded `memcpy`s for 
> ordinary `__builtin_memcpy`.

Yeah I think that's a leftover from the first patch. Should I drop it? At the 
same time, address spaces are currently accidentally "working", should I drop 
that in a patch before this change? Or leave as-is?




Comment at: clang/docs/LanguageExtensions.rst:2446
+order in which they occur (and in which they are observable) can only be
+guaranteed using appropriate fences around the function call. Element size must
+therefore be a lock-free size for the target architecture. It is a runtime

rjmccall wrote:
> "*The* element size must..."  But I would suggest using "access size" 
> consistently rather than "element size".
I'm being consistent with the naming for IR, which uses "element" as well. I'm 
not enamored with the naming, but wanted to point out the purposeful 
consistency to make sure you preferred "access size". Without precedent I would 
indeed prefer "access size", but have a slight preference for consistency here. 
This is extremely weakly held preference.

(I fixed "the").



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:
> jfb wrote:
> > 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`.
> Please document the trivial copy check.
Should I bubble this to the rest of the builtin in a follow-up patch? I know 
there are cases where that'll cause issues, but I worry that it would be a 
pretty noisy diagnostic (especially if we instead bubble it to C's `memcpy` 
instead).



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:
> jfb wrote:
> > 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 , 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()->getPointeeType();
> >   return ArgTy->castAs()->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.
> The example I gave should produce a non-volatile memcpy, and used to 

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

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 283121.
jfb marked 2 inline comments as done.
jfb added a comment.

Update docs


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/CodeGen/CGBuilder.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-overloaded-memfns.c
  clang/test/CodeGen/ubsan-builtin-checks.c
  clang/test/CodeGen/ubsan-builtin-ctz-clz.c
  clang/test/CodeGen/ubsan-builtin-mem_overloaded.c
  clang/test/CodeGenObjC/builtin-memfns.m
  clang/test/Sema/builtin-overloaded-memfns.cpp
  clang/test/SemaCXX/constexpr-string.cpp
  compiler-rt/lib/ubsan/ubsan_handlers.cpp
  compiler-rt/lib/ubsan/ubsan_handlers.h
  compiler-rt/test/ubsan/TestCases/Misc/builtins-ctz-clz.cpp
  compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_overloaded.cpp
  compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp

Index: compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
===
--- compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
+++ /dev/null
@@ -1,35 +0,0 @@
-// REQUIRES: arch=x86_64
-//
-// RUN: %clangxx -fsanitize=builtin -w %s -O3 -o %t
-// RUN: %run %t 2>&1 | FileCheck %s --check-prefix=RECOVER
-// RUN: %clangxx -fsanitize=builtin -fno-sanitize-recover=builtin -w %s -O3 -o %t.abort
-// RUN: not %run %t.abort 2>&1 | FileCheck %s --check-prefix=ABORT
-
-void check_ctz(int n) {
-  // ABORT: builtins.cpp:[[@LINE+2]]:17: runtime error: passing zero to ctz(), which is not a valid argument
-  // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctz(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctzl(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctzll(n);
-}
-
-void check_clz(int n) {
-  // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clz(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clzl(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clzll(n);
-}
-
-int main() {
-  check_ctz(0);
-  check_clz(0);
-  return 0;
-}
Index: compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_overloaded.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_overloaded.cpp
@@ -0,0 +1,93 @@
+// RUN: %clangxx -fsanitize=builtin -w %s -O3 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+
+using uintptr_t = __UINTPTR_TYPE__;
+using size_t = __SIZE_TYPE__;
+
+void check_memcpy_align(char *dst_aligned, char *dst_misaligned, const char *src_aligned, const char *src_misaligned, size_t sz) {
+  // OK.
+  __builtin_memcpy_overloaded(dst_aligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:31: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memcpy_overloaded(dst_misaligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:44: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memcpy_overloaded(dst_aligned, src_misaligned, sz, 2);
+}
+
+void check_memmove_align(char *dst_aligned, char *dst_misaligned, const char *src_aligned, const char *src_misaligned, size_t sz) {
+  // OK.
+  __builtin_memmove_overloaded(dst_aligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:32: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memmove_overloaded(dst_misaligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:45: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memmove_overloaded(dst_aligned, src_misaligned, sz, 2);
+}
+
+void check_memset_align(char *dst_aligned, char *dst_misaligned, size_t sz) {
+  // OK.
+  __builtin_memset_overloaded(dst_aligned, 0, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:31: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memset_overloaded(dst_misaligned, 0, sz, 2);
+}
+
+void 

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Patch looks basically okay to me, although I'll second Richard's concern that 
we shouldn't absent-mindedly start producing overloaded `memcpy`s for ordinary 
`__builtin_memcpy`.




Comment at: clang/docs/LanguageExtensions.rst:2446
+order in which they occur (and in which they are observable) can only be
+guaranteed using appropriate fences around the function call. Element size must
+therefore be a lock-free size for the target architecture. It is a runtime

"*The* element size must..."  But I would suggest using "access size" 
consistently rather than "element size".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

rsmith added a comment.

Thanks. I'd like @rjmccall to approve too, but the design of these intrinsics 
and the Sema and constant evaluation parts seem fine. (We don't strictly need 
constant evaluation to abort on library UB, so I think not catching the 
misalignment case is OK.)




Comment at: clang/docs/LanguageExtensions.rst:2447-2449
+therefore be a lock-free size for the target architecture. It is a runtime
+constraint violation to provide a memory locations which is aligned to less 
than
+the element size. It is a runtime constraint violation to provide a size which

"runtime constraint violation" is an odd phrase; in C, "constraint violation" 
means a diagnostic is required. Can we instead say that it results in undefined 
behavior?



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:

jfb wrote:
> 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`.
Please document the trivial copy check.



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));

jfb wrote:
> 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 , 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()->getPointeeType();
>   return ArgTy->castAs()->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.
The example I gave should produce a non-volatile memcpy, and used to do so (we 
passed `false` as the fourth parameter to `CreateMemCpy`). With this patch, 
`getPtrArgType`will strip off the implicit conversion from `volatile void*` to 
`void*` in the argument type, so `isVolatile` below will be `true`, so I think 
it will now create a volatile memcpy for the same testcase. If that's not 
what's happening, then I'd like to understand why not :)

I'm not saying that's necessarily a bad change, but it is a change, and it's 
one we should make intentionally if we make it at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: compiler-rt/test/ubsan/TestCases/Misc/builtins-ctz-clz.cpp:1
+// REQUIRES: arch=x86_64
+//

Phab is confused I did a git rename of 
`compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp` and it thinks this is new, 
and I deleted the other.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 , 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()->getPointeeType();
  return ArgTy->castAs()->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 {
>   

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

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 283078.
jfb marked 6 inline comments as done.
jfb added a comment.
Herald added a project: Sanitizers.
Herald added a subscriber: Sanitizers.

Address Richard's comments, add UBSan support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/CodeGen/CGBuilder.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-overloaded-memfns.c
  clang/test/CodeGen/ubsan-builtin-checks.c
  clang/test/CodeGen/ubsan-builtin-ctz-clz.c
  clang/test/CodeGen/ubsan-builtin-mem_overloaded.c
  clang/test/CodeGenObjC/builtin-memfns.m
  clang/test/Sema/builtin-overloaded-memfns.cpp
  clang/test/SemaCXX/constexpr-string.cpp
  compiler-rt/lib/ubsan/ubsan_handlers.cpp
  compiler-rt/lib/ubsan/ubsan_handlers.h
  compiler-rt/test/ubsan/TestCases/Misc/builtins-ctz-clz.cpp
  compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_overloaded.cpp
  compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp

Index: compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
===
--- compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
+++ /dev/null
@@ -1,35 +0,0 @@
-// REQUIRES: arch=x86_64
-//
-// RUN: %clangxx -fsanitize=builtin -w %s -O3 -o %t
-// RUN: %run %t 2>&1 | FileCheck %s --check-prefix=RECOVER
-// RUN: %clangxx -fsanitize=builtin -fno-sanitize-recover=builtin -w %s -O3 -o %t.abort
-// RUN: not %run %t.abort 2>&1 | FileCheck %s --check-prefix=ABORT
-
-void check_ctz(int n) {
-  // ABORT: builtins.cpp:[[@LINE+2]]:17: runtime error: passing zero to ctz(), which is not a valid argument
-  // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctz(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctzl(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to ctz(), which is not a valid argument
-  __builtin_ctzll(n);
-}
-
-void check_clz(int n) {
-  // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clz(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clzl(n);
-
-  // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to clz(), which is not a valid argument
-  __builtin_clzll(n);
-}
-
-int main() {
-  check_ctz(0);
-  check_clz(0);
-  return 0;
-}
Index: compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_overloaded.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/builtins-mem_overloaded.cpp
@@ -0,0 +1,93 @@
+// RUN: %clangxx -fsanitize=builtin -w %s -O3 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+
+using uintptr_t = __UINTPTR_TYPE__;
+using size_t = __SIZE_TYPE__;
+
+void check_memcpy_align(char *dst_aligned, char *dst_misaligned, const char *src_aligned, const char *src_misaligned, size_t sz) {
+  // OK.
+  __builtin_memcpy_overloaded(dst_aligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:31: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memcpy_overloaded(dst_misaligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:44: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memcpy_overloaded(dst_aligned, src_misaligned, sz, 2);
+}
+
+void check_memmove_align(char *dst_aligned, char *dst_misaligned, const char *src_aligned, const char *src_misaligned, size_t sz) {
+  // OK.
+  __builtin_memmove_overloaded(dst_aligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:32: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memmove_overloaded(dst_misaligned, src_aligned, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:45: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into __builtin_mem*_overloaded, element size 2
+  __builtin_memmove_overloaded(dst_aligned, src_misaligned, sz, 2);
+}
+
+void check_memset_align(char *dst_aligned, char *dst_misaligned, size_t sz) {
+  // OK.
+  __builtin_memset_overloaded(dst_aligned, 0, sz, 2);
+  // CHECK: builtins-mem_overloaded.cpp:[[@LINE+1]]:31: runtime error: passing pointer 0x{{[0-9a-f]*}} with invalid alignment 1 into 

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

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

rsmith added inline comments.



Comment at: clang/docs/LanguageExtensions.rst:2430
+
+These overloads support destinations and sources which are a mix of the
+following qualifiers:





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:

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.)



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;

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).



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));

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?



Comment at: clang/lib/CodeGen/CGExpr.cpp:1167-1168
 
+  if (E->getType()->isArrayType())
+return EmitArrayToPointerDecay(E, BaseInfo, TBAAInfo);
+

Do we still need this? We should be doing the decay in `Sema`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 = )``
+* ``__builtin_memmove_overloaded(QUAL void *dst, QUAL const void *src, size_t 
byte_size, size_t byte_element_size = )``
+* ``__builtin_memset_overloaded(QUAL void *dst, unsigned char val, size_t 
byte_size, size_t byte_element_size = )``

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 = )``
+* ``__builtin_memmove_overloaded(QUAL void *dst, QUAL const void *src, size_t 
byte_size, size_t byte_element_size = )``
+* ``__builtin_memset_overloaded(QUAL void *dst, unsigned char val, size_t 
byte_size, size_t byte_element_size = )``

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 

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

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 282281.
jfb marked 9 inline comments as done.
jfb added a comment.

Address Richard's comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/CodeGen/CGBuilder.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-overloaded-memfns.c
  clang/test/CodeGenObjC/builtin-memfns.m
  clang/test/Sema/builtin-overloaded-memfns.cpp
  clang/test/SemaCXX/constexpr-string.cpp

Index: clang/test/SemaCXX/constexpr-string.cpp
===
--- clang/test/SemaCXX/constexpr-string.cpp
+++ clang/test/SemaCXX/constexpr-string.cpp
@@ -675,4 +675,25 @@
 return true;
   }
   static_assert(test_address_of_incomplete_struct_type()); // expected-error {{constant}} expected-note {{in call}}
+
+  template 
+  constexpr auto test_memcpy_overloaded(int dst_off, int src_off, int num) {
+T dst[4] = {0, 0, 0, 0};
+const T src[4] = {1, 2, 3, 4};
+// expected-note@+2 {{size parameter is 12, expected a size that is evenly divisible by element size 8}}
+// expected-note@+1 {{size parameter is 4, expected a size that is evenly divisible by element size 8}}
+__builtin_memcpy_overloaded(dst + dst_off, src + src_off, num * sizeof(T), ElNum * sizeof(T));
+return result(dst);
+  }
+
+  static_assert(test_memcpy_overloaded(0, 0, 1) == 1000);
+  static_assert(test_memcpy_overloaded(0, 0, 2) == 1200);
+  static_assert(test_memcpy_overloaded(0, 0, 3) == 1230);
+  static_assert(test_memcpy_overloaded(0, 0, 4) == 1234);
+  static_assert(test_memcpy_overloaded(0, 0, 4) == 1234);
+
+  // expected-error@+1 {{static_assert expression is not an integral constant expression}}
+  static_assert(test_memcpy_overloaded(0, 0, 3) == 1234); // expected-note {{in call to 'test_memcpy_overloaded(0, 0, 3)'}}
+  // expected-error@+1 {{static_assert expression is not an integral constant expression}}
+  static_assert(test_memcpy_overloaded(0, 0, 1) == 1234); // expected-note {{in call to 'test_memcpy_overloaded(0, 0, 1)'}}
 }
Index: clang/test/Sema/builtin-overloaded-memfns.cpp
===
--- /dev/null
+++ clang/test/Sema/builtin-overloaded-memfns.cpp
@@ -0,0 +1,252 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -triple=arm64-unknown-unknown -fms-extensions -DCPY=1
+// RUN: %clang_cc1 %s -verify -fsyntax-only -triple=arm64-unknown-unknown -fms-extensions -DCPY=0
+
+// Test memcpy and memmove with the same code, since they're basically the same constraints.
+#if CPY
+#define MEM(...) __builtin_memcpy_overloaded(__VA_ARGS__)
+#else
+#define MEM(...) __builtin_memmove_overloaded(__VA_ARGS__)
+#endif
+
+#define NULL (void *)0
+#define nullptr __nullptr
+using size_t = __SIZE_TYPE__;
+using sizeless_t = __SVInt8_t;
+using float4 = float __attribute__((ext_vector_type(4)));
+struct Intish {
+  int i;
+};
+struct NotLockFree {
+  char buf[512];
+};
+struct TrivialCpy {
+  char buf[8];
+  TrivialCpy();
+  TrivialCpy(const TrivialCpy &) = default;
+};
+struct NotTrivialCpy {
+  char buf[8];
+  NotTrivialCpy();
+  NotTrivialCpy(const NotTrivialCpy &);
+};
+
+constexpr int CONSTEXPR_ONE = 1;
+
+void arg_count() {
+  MEM();  // expected-error {{too few arguments to function call, expected 3, have 0}}
+  MEM(0); // expected-error {{too few arguments to function call, expected 3, have 1}}
+  MEM(0, 0);  // expected-error {{too few arguments to function call, expected 3, have 2}}
+  MEM(0, 0, 0, 0, 0); // expected-error {{too many arguments to function call, expected 4, have 5}}
+  __builtin_memset_overloaded();  // expected-error {{too few arguments to function call, expected 3, have 0}}
+  __builtin_memset_overloaded(0); // expected-error {{too few arguments to function call, expected 3, have 1}}
+  __builtin_memset_overloaded(0, 0);  // expected-error {{too few arguments to function call, expected 3, have 2}}
+  __builtin_memset_overloaded(0, 0, 0, 0, 0); // expected-error {{too many arguments to function call, expected 4, have 5}}
+}
+
+void null(char *dst, const char *src, size_t size) {
+  MEM(0, src, 0);  // expected-error{{cannot initialize a parameter of type 'void *' with an rvalue of type 'int'}}
+  MEM(0, src, size);   // expected-error{{cannot initialize a parameter of type 'void *' with an rvalue of type 'int'}}
+  MEM(dst, 0, 0);  // 

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

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

rsmith added inline comments.



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 = )``
+* ``__builtin_memmove_overloaded(QUAL void *dst, QUAL const void *src, size_t 
byte_size, size_t byte_element_size = )``
+* ``__builtin_memset_overloaded(QUAL void *dst, unsigned char val, size_t 
byte_size, size_t byte_element_size = )``

What happens if `byte_element_size` does not divide `byte_size`?



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 = )``
+* ``__builtin_memmove_overloaded(QUAL void *dst, QUAL const void *src, size_t 
byte_size, size_t byte_element_size = )``
+* ``__builtin_memset_overloaded(QUAL void *dst, unsigned char val, size_t 
byte_size, size_t byte_element_size = )``

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?



Comment at: clang/docs/LanguageExtensions.rst:2444-2445
+parameter can be provided to specify element access size in bytes. When the
+element size is provided, the memory will be accessed with a sequence of
+operations of size equal to or a multiple of the pointer's element size. The
+order of operations is unspecified, and each access has unordered atomic

"the pointer's element size" -- do you mean "the provided element size"?

Does the element size need to be a compile-time constant? (Presumably, but you 
don't say so.)



Comment at: clang/docs/LanguageExtensions.rst:2446-2447
+operations of size equal to or a multiple of the pointer's element size. The
+order of operations is unspecified, and each access has unordered atomic
+semantics. This means that reads and writes do not tear at the individual
+element level, and they each occur exactly once, but the order in which they

Presumably this means that it's an error if we don't provide lock-free atomic 
access for that size. Would be worth saying so.



Comment at: clang/docs/LanguageExtensions.rst:2450-2451
+occur (and in which they are observable) can only be guaranteed using
+appropriate fences around the function call. Atomic memory operations must be 
to
+memory locations which are aligned to no less than the element size.
+

What happens if they're not? Is it UB, or is it just not guaranteed to be 
atomic?



Comment at: clang/docs/LanguageExtensions.rst:2456
+
+These can be used as building blocks for different facitilies:
+

*facilities



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8937-8938
+  "(%0 is volatile)">;
+def err_elsz_on_sizeless : Error<
+  "element size must not be on a sizeless type (%0 invalid)">;
+def err_elsz_must_be_lock_free : Error<

Given the new documentation, I would expect you don't need this any more.



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">;

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.



Comment at: clang/lib/AST/ExprConstant.cpp:8793
+BuiltinOp == Builtin::BI__builtin_memmove_overloaded ||
 BuiltinOp == Builtin::BI__builtin_wmemmove;
 

jfb wrote:
> If we end up making alignment a runtime constraint, then I'll need to check 
> it in consteval. Otherwise I don't think we need to check anything since Sema 
> ought to have done all the required checks already.
I don't see how you can check the alignment at compile time given a `void*` 
argument.

We presumably need to check that the element size (if given) divides the total 
size, assuming the outcome is UB if not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb marked an inline comment as done.
jfb added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:8793
+BuiltinOp == Builtin::BI__builtin_memmove_overloaded ||
 BuiltinOp == Builtin::BI__builtin_wmemmove;
 

If we end up making alignment a runtime constraint, then I'll need to check it 
in consteval. Otherwise I don't think we need to check anything since Sema 
ought to have done all the required checks already.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb added a subscriber: dneilson.
jfb added a comment.

I've addressed @rsmith @rjmccall suggestions (unless noted), thanks!
An open question: as of 6e4aa1e48138182685431c76184dfc36e620aea2 @dneilson 
added an assertion on `CreateElementUnorderedAtomicMemCpy` to check that the 
pointer arguments have alignments of at least the element size. That makes 
sense when the IR is only ever built internally to LLVM, but now that I'm 
adding a builtin it's more of a dynamic property. Should I also force this in 
the frontend (understanding that alignment isn't always well known at compile 
time), or should simply assume that the alignment is correct because it's a 
dynamic property?

I left some FIXMEs in the CodeGen test for this.




Comment at: clang/docs/LanguageExtensions.rst:2439-2440
 
+Clang provides versions of the following functions which are overloaded based 
on
+the pointer parameter types:
+

rsmith wrote:
> This is missing some important details:
> 
> - What does the size parameter mean? Is it number of bytes or number of 
> elements? If it's number of bytes, what happens if it's not a multiple of the 
> element size, particularly in the `_Atomic` case?
> - What does the value parameter to `memset` mean? Is it splatted to the 
> element width? Does it specify a complete element value?
> - For `_Atomic`, what memory order is used?
> - For `volatile`, what access size / type is used? Do we want to make any 
> promises?
> - Are the loads and stores typed or untyped? (In particular, do we annotate 
> with TBAA metadata?)
> - Do we guarantee to copy the object representation or only the value 
> representation? (Do we preserve the values of padding bits in the source, and 
> initialize padding bits in the destination?)
> 
> You should also document whether constant evaluation of these builtins is 
> supported.
Most of these are answered in the update.

Some of the issue is that the current documentation is silent on these points 
already, by saying "same as C's `mem*` function". I'm relying on that approach 
here as well.

Size is bytes.

`memset` value is an `unsigned char`.

Memory order is unordered, and accesses themselves are done in indeterminate 
order.

For `volatile`, it falls out of the new wording that we don't provide access 
size guarantees. We'd need to nail down IR better to do so, and I don't think 
it's the salient property (though as discussed above, it might be useful, and 
the `element_size` parameter make it easy to do so).

Same on TBAA, no mention because "same as C" (no TBAA annotations).

Same on copying bits as-is.

Good point on constant evaluation. I added support. Note that we don't have 
`memset` constant evaluation, so I didn't support it. Seems easy, but ought to 
be a separate patch.



Comment at: clang/docs/LanguageExtensions.rst:2446-2448
+These overloads support destinations and sources which are a mix of
+``volatile``, ``_Atomic``, ``restrict``, ``__unaligned``, and use non-default
+address spaces. These can be used as building blocks for different facitilies:

rsmith wrote:
> Mixing those qualifiers doesn't seem like it will work in many cases: we 
> don't allow mixing `volatile` and `_Atomic` (though I'm not sure why; LLVM 
> supports volatile atomic operations), and presumably we shouldn't allow 
> mixing `__unaligned` and `_Atomic` (although I don't see any tests for that, 
> and maybe we should just outright disallow combining `_Atomic` with 
> `__unaligned` in general).
`volatile` and `_Atomic` ought to work...

For this code I didn't make it work (even if it might be useful), because we'd 
need IR support for it.

On mixing `_Atomic __unaligned`: I left a FIXME because I'm not 100% sure, 
given the alignment discussion on atomic in general. Let's see where we settle: 
if we make it a pure runtime property then `__unaligned` ought to be fine 
because it's a constraint violation if the actual pointer is truly unaligned.



Comment at: clang/include/clang/Basic/Builtins.def:471
 
 // Random GCC builtins
 BUILTIN(__builtin_constant_p, "i.", "nctu")

rsmith wrote:
> Are these really GCC builtins?
Oops, I didn't see that comment, was just copying `__builtin_memcpy_inline`. 
I'll move it too.



Comment at: clang/lib/Sema/SemaChecking.cpp:1277-1278
 CallExpr *Call) {
-  if (Call->getNumArgs() != 1) {
-S.Diag(Call->getBeginLoc(), diag::err_opencl_builtin_to_addr_arg_num)
-<< Call->getDirectCallee() << Call->getSourceRange();
+  if (checkArgCount(S, Call, 1))
 return true;
 

rsmith wrote:
> There are a bunch of places in this file that do manual argument count 
> checking and could use `checkArgCount` instead (search for 
> `err_typecheck_call_too_` to find them). If you want to clean this up, please 
> do so in a separate change.
D84666



Comment at: 

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

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 281087.
jfb marked 15 inline comments as done.
jfb added a comment.

Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/CodeGen/CGBuilder.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-overloaded-memfns.c
  clang/test/CodeGenObjC/builtin-memfns.m
  clang/test/Sema/builtin-overloaded-memfns.cpp

Index: clang/test/Sema/builtin-overloaded-memfns.cpp
===
--- /dev/null
+++ clang/test/Sema/builtin-overloaded-memfns.cpp
@@ -0,0 +1,245 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -triple=arm64-unknown-unknown -fms-extensions -DCPY=1
+// RUN: %clang_cc1 %s -verify -fsyntax-only -triple=arm64-unknown-unknown -fms-extensions -DCPY=0
+
+// Test memcpy and memmove with the same code, since they're basically the same constraints.
+#if CPY
+#define MEM(...) __builtin_memcpy_overloaded(__VA_ARGS__)
+#else
+#define MEM(...) __builtin_memmove_overloaded(__VA_ARGS__)
+#endif
+
+#define NULL (void *)0
+#define nullptr __nullptr
+using size_t = __SIZE_TYPE__;
+using sizeless_t = __SVInt8_t;
+using float4 = float __attribute__((ext_vector_type(4)));
+struct Intish {
+  int i;
+};
+struct NotLockFree {
+  char buf[512];
+};
+struct TrivialCpy {
+  char buf[8];
+  TrivialCpy();
+  TrivialCpy(const TrivialCpy &) = default;
+};
+struct NotTrivialCpy {
+  char buf[8];
+  NotTrivialCpy();
+  NotTrivialCpy(const NotTrivialCpy &);
+};
+
+void arg_count() {
+  MEM();  // expected-error {{too few arguments to function call, expected 3, have 0}}
+  MEM(0); // expected-error {{too few arguments to function call, expected 3, have 1}}
+  MEM(0, 0);  // expected-error {{too few arguments to function call, expected 3, have 2}}
+  MEM(0, 0, 0, 0, 0); // expected-error {{too many arguments to function call, expected 4, have 5}}
+  __builtin_memset_overloaded();  // expected-error {{too few arguments to function call, expected 3, have 0}}
+  __builtin_memset_overloaded(0); // expected-error {{too few arguments to function call, expected 3, have 1}}
+  __builtin_memset_overloaded(0, 0);  // expected-error {{too few arguments to function call, expected 3, have 2}}
+  __builtin_memset_overloaded(0, 0, 0, 0, 0); // expected-error {{too many arguments to function call, expected 4, have 5}}
+}
+
+void null(char *dst, const char *src, size_t size) {
+  MEM(0, src, 0);  // expected-error{{cannot initialize a parameter of type 'void *' with an rvalue of type 'int'}}
+  MEM(0, src, size);   // expected-error{{cannot initialize a parameter of type 'void *' with an rvalue of type 'int'}}
+  MEM(dst, 0, 0);  // expected-error{{cannot initialize a parameter of type 'void *' with an rvalue of type 'int'}}
+  MEM(dst, 0, size);   // expected-error{{cannot initialize a parameter of type 'void *' with an rvalue of type 'int'}}
+  __builtin_memset_overloaded(0, 0, 0);// expected-error{{cannot initialize a parameter of type 'void *' with an rvalue of type 'int'}}
+  __builtin_memset_overloaded(0, 0, size); // expected-error{{cannot initialize a parameter of type 'void *' with an rvalue of type 'int'}}
+  MEM(dst, 0, 42); // expected-error{{cannot initialize a parameter of type 'void *' with an rvalue of type 'int'}}
+  MEM(dst, 0, 42); // expected-error{{cannot initialize a parameter of type 'void *' with an rvalue of type 'int'}}
+  MEM(dst, NULL, 42);  // expected-warning {{null passed to a callee that requires a non-null argument}}
+  MEM(dst, nullptr, 42);   // expected-error{{cannot initialize a parameter of type 'void *' with an rvalue of type 'nullptr_t'}}
+  MEM(0, src, 42); // expected-error{{cannot initialize a parameter of type 'void *' with an rvalue of type 'int'}}
+  MEM(NULL, src, 42);  // expected-warning {{null passed to a callee that requires a non-null argument}}
+  MEM(nullptr, src, 42);   // expected-error{{cannot initialize a parameter of type 'void *' with an rvalue of type 'nullptr_t'}}
+  __builtin_memset_overloaded(0, 0, 42);   // expected-error{{cannot initialize a parameter of type 'void *' with an rvalue of type 'int'}}
+  __builtin_memset_overloaded(NULL, 0, 42);// expected-warning {{null 

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

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

rsmith added a comment.

These new builtins should ideally support constant evaluation if possible.




Comment at: clang/docs/LanguageExtensions.rst:2439-2440
 
+Clang provides versions of the following functions which are overloaded based 
on
+the pointer parameter types:
+

This is missing some important details:

- What does the size parameter mean? Is it number of bytes or number of 
elements? If it's number of bytes, what happens if it's not a multiple of the 
element size, particularly in the `_Atomic` case?
- What does the value parameter to `memset` mean? Is it splatted to the element 
width? Does it specify a complete element value?
- For `_Atomic`, what memory order is used?
- For `volatile`, what access size / type is used? Do we want to make any 
promises?
- Are the loads and stores typed or untyped? (In particular, do we annotate 
with TBAA metadata?)
- Do we guarantee to copy the object representation or only the value 
representation? (Do we preserve the values of padding bits in the source, and 
initialize padding bits in the destination?)

You should also document whether constant evaluation of these builtins is 
supported.



Comment at: clang/docs/LanguageExtensions.rst:2446-2448
+These overloads support destinations and sources which are a mix of
+``volatile``, ``_Atomic``, ``restrict``, ``__unaligned``, and use non-default
+address spaces. These can be used as building blocks for different facitilies:

Mixing those qualifiers doesn't seem like it will work in many cases: we don't 
allow mixing `volatile` and `_Atomic` (though I'm not sure why; LLVM supports 
volatile atomic operations), and presumably we shouldn't allow mixing 
`__unaligned` and `_Atomic` (although I don't see any tests for that, and maybe 
we should just outright disallow combining `_Atomic` with `__unaligned` in 
general).



Comment at: clang/include/clang/Basic/Builtins.def:471
 
 // Random GCC builtins
 BUILTIN(__builtin_constant_p, "i.", "nctu")

Are these really GCC builtins?



Comment at: clang/include/clang/Basic/Builtins.def:1487
 
 // Clang builtins (not available in GCC).
 BUILTIN(__builtin_addressof, "v*v&", "nct")

The new builtins probably belong in this section of the file instead.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7961-7963
-def err_atomic_op_needs_trivial_copy : Error<
-  "address argument to atomic operation must be a pointer to a "
-  "trivially-copyable type (%0 invalid)">;

I'd prefer to keep this diagnostic separate, since it communicates more 
information than `err_argument_needs_trivial_copy` does: specifically that we 
need a trivial copy because we're performing an atomic operation.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8926-8934
+def err_const_arg : Error<"argument must be non-const, got %0">;
+
+def err_argument_needs_trivial_copy : Error<"address argument must be a 
pointer to a trivially-copyable type (%0 invalid)">;
+  
+def err_atomic_volatile_unsupported : Error<"mixing _Atomic and volatile 
qualifiers is unsupported (%select{%1|%1 and %2}0 cannot have both _Atomic and 
volatile)">;
+  
+def err_atomic_sizes_must_match : Error<"_Atomic sizes must match, %0 is %1 
bytes and %2 is %3 bytes">;

Please format these diagnostics consistently with the rest of the file: line 
break after `Error<`, wrap to 80 columns, don't leave blank lines between 
individual diagnostics in a group of related diagnostics.



Comment at: clang/lib/Sema/SemaChecking.cpp:1277-1278
 CallExpr *Call) {
-  if (Call->getNumArgs() != 1) {
-S.Diag(Call->getBeginLoc(), diag::err_opencl_builtin_to_addr_arg_num)
-<< Call->getDirectCallee() << Call->getSourceRange();
+  if (checkArgCount(S, Call, 1))
 return true;
 

There are a bunch of places in this file that do manual argument count checking 
and could use `checkArgCount` instead (search for `err_typecheck_call_too_` to 
find them). If you want to clean this up, please do so in a separate change.



Comment at: clang/lib/Sema/SemaChecking.cpp:5510-5512
+  if ((DstValTy->isAtomicType() || SrcValTy->isAtomicType()) &&
+  (!DstValTy.getUnqualifiedType()->isVoidType() &&
+   !SrcValTy.getUnqualifiedType()->isVoidType()) &&

Do we need this constraint?

- If one side is atomic and the other is not, then we can do all of the 
operations with the atomic width.
- If both sides are atomic, then one side is 2^N times the size of the other; 
we can do 2^N operations on one side for each operation on the other side.

Maybe the second case is not worth the effort, but permitting (for example) a 
memcpy from an `_Atomic int*` to a `char*` seems useful and there doesn't seem 
to be a good reason to disallow it.

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

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

>>> Do you think it'd be useful to have different guarantees for different 
>>> operands?  I guess it could come up, but it'd be a whole lot of extra 
>>> complexity that I can't imagine we'd ever support.
>> 
>> You mean, if `element_size` is passed then you get different guarantees?
> 
> No, sorry, I mean different guarantees for the different pointer operands.  
> In principle, we could allow you to say that the memcpy has to be done with 
> 4-byte accesses from the source and 2-byte accesses to the destination.  
> That's implementable but a lot of work.

Gotcha. Yeah I think it's useful as a niche thing, and if IR supports that for 
say `volatile` then we can honor lopsided `volatile` overloads (instead of 
treating the entire thing as `volatile`). I hadn't really thought about 
lopsided access sizes (since it fell out of `_Atomic`). Maybe it's useful? I 
was just banning unequal sizes before because it seemed like a mistake to copy 
to/from different types. If we wanted to support it, I suppose we could add 
another optional parameter, so I'm OK not doing it now, and adding later if 
useful.

Alright, I'll update the patch as discussed, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D79279#2170187 , @jfb wrote:

> In D79279#2170157 , @rjmccall wrote:
>
> > I think the argument is treated as if it were 1 if not given.  That's all 
> > that ordinary memcpy formally guarantees, which seems to work fine 
> > (semantically, if not performance-wise) for pretty much everything today.
>
>
> I'm not sure that's true: consider a `memcpy` implementation which copies 
> some bytes twice (at different access size, there's an overlap because 
> somehow it's more efficient). That would probably violate the programmer's 
> expectations, and I don't think `volatile` nor atomic `memcpy` allow this 
> (but regular `memcpy` does).


Yes, that's true, if you need an only-accessed-once guarantee, that's above and 
beyond just a minimum access size.  I agree that `volatile` would need to make 
this guarantee.

>> Do you think it'd be useful to have different guarantees for different 
>> operands?  I guess it could come up, but it'd be a whole lot of extra 
>> complexity that I can't imagine we'd ever support.
> 
> You mean, if `element_size` is passed then you get different guarantees?

No, sorry, I mean different guarantees for the different pointer operands.  In 
principle, we could allow you to say that the memcpy has to be done with 4-byte 
accesses from the source and 2-byte accesses to the destination.  That's 
implementable but a lot of work.

>> If one of the arguments is `volatile`, arguably the minimum access width (if 
>> given) needs to be exact.  If we don't support that right now, it's okay to 
>> make it an error, which is basically you've already done with the `_Atomic 
>> volatile` diagnostic.
> 
> Agreed. `volatile` with size makes a lot of sense, and the IR version of it, 
> once created, ought to not be able to widen accesses. `volatile` without a 
> size specified makes sense too, because you just want a single read and a 
> single write, don't care about tearing.

Right.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D79279#2170157 , @rjmccall wrote:

> I think the argument is treated as if it were 1 if not given.  That's all 
> that ordinary memcpy formally guarantees, which seems to work fine 
> (semantically, if not performance-wise) for pretty much everything today.


I'm not sure that's true: consider a `memcpy` implementation which copies some 
bytes twice (at different access size, there's an overlap because somehow it's 
more efficient). That would probably violate the programmer's expectations, and 
I don't think `volatile` nor atomic `memcpy` allow this (but regular `memcpy` 
does).

> I don't think you need any restrictions on element size.  It's probably 
> sensible to require the pointers to be dynamically aligned to a multiple of 
> the access width, but I don't think you can enforce that statically.

Agreed, if we're given a short and told to copy 4 bytes at a time then UBSan 
could find the constraint violation on alignment, but generally the only way we 
can diagnose is if the parameter is `__unaligned` (because there you're 
explicitly telling me it's not aligned, and the constraint is that it has to 
be).

> And of course the length needs to be a multiple of the access size.

Yeah.

> Do you think it'd be useful to have different guarantees for different 
> operands?  I guess it could come up, but it'd be a whole lot of extra 
> complexity that I can't imagine we'd ever support.

You mean, if `element_size` is passed then you get different guarantees? I 
think that's what makes sense: if you're asking for atomic `memcpy` then you 
get guarantees. If you're asking for `volatile` `mempcy` then you get others. 
That's why overloading (and multiple parameters) can be confusing, but at the 
same time I think it's better than having the combinatorial number of named 
functions instead.

> If one of the arguments is `volatile`, arguably the minimum access width (if 
> given) needs to be exact.  If we don't support that right now, it's okay to 
> make it an error, which is basically you've already done with the `_Atomic 
> volatile` diagnostic.

Agreed. `volatile` with size makes a lot of sense, and the IR version of it, 
once created, ought to not be able to widen accesses. `volatile` without a size 
specified makes sense too, because you just want a single read and a single 
write, don't care about tearing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I think the argument is treated as if it were 1 if not given.  That's all that 
ordinary memcpy formally guarantees, which seems to work fine (semantically, if 
not performance-wise) for pretty much everything today.  I don't think you need 
any restrictions on element size.  It's probably sensible to require the 
pointers to be dynamically aligned to a multiple of the access width, but I 
don't think you can enforce that statically.  And of course the length needs to 
be a multiple of the access size.

Do you think it'd be useful to have different guarantees for different 
operands?  I guess it could come up, but it'd be a whole lot of extra 
complexity that I can't imagine we'd ever support.

If one of the arguments is `volatile`, arguably the minimum access width (if 
given) needs to be exact.  If we don't support that right now, it's okay to 
make it an error, which is basically you've already done with the `_Atomic 
volatile` diagnostic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D79279#2170095 , @rjmccall wrote:

> I don't think any of these should allow _Atomic unless we're going to give it 
> some sort of consistent atomic semantics (which is hard to imagine being 
> useful), and I think you should just take an extra argument of the minimum 
> access width on all of them uniformly if you think that's important.  
> Builtins can have optional arguments.


OK so: `__builtin_memcpy_overloaded` which overloads on `volatile`, `restrict`, 
`__unaligned`, and address spaces, but not on `_Atomic` qualifiers. Optionally, 
a 4th integer parameter can be provided to represent `element_size`. If 
provided, this becomes an unordered atomic memcpy with element size equal to or 
greater than the provided `element_size`. That value must be a power of two, 
and must be lock-free (what we call maximum atomic inline width in target 
info). If provided, then `__unaligned` is invalid, and `volatile` ought to be 
valid but is currently unsupported because IR can't do atomic+`volatile` memcpy 
(it would be useful, say for shared memory, but Patches Welcome).

Do you think there should be any relationship at all between dst/src pointee 
type's size and `element_size`? i.e. if I copy `short*` using an element size 
of 1 byte, is that OK? It seems like larger element sizes is always OK, but 
smaller might be a programmer error? If that's what they wanted, they could 
have done `(char*)my_short`. Or is this trying to be too helpful?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I don't think any of these should allow _Atomic unless we're going to give it 
some sort of consistent atomic semantics (which is hard to imagine being 
useful), and I think you should just take an extra argument of the minimum 
access width on all of them uniformly if you think that's important.  Builtins 
can have optional arguments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

> My point is that this has nothing to do with the ordinary semantics of 
> `_Atomic`.  You're basically just looking at the word "atomic" and saying 
> that, hey, a minimum access size is sortof related to atomicity.
> 
> If you want this to be able to control the minimum access size, you should 
> allow that to be passed in as an optional argument instead.

OK so it sounds like you're suggesting *two* versions of the overloaded 
builtins:

1. `__builtin_memcpy_overloaded` which overloads on `volatile`, `restrict`, 
`__unaligned`, and address spaces, but **not** on `_Atomic` qualifiers.
2. `__builtin_atomic_memcpy_overloaded` which overloads on `volatile` (but 
unsupported for now), `restrict`, and address spaces, but **not** on `_Atomic` 
qualifiers (because it's implicit), and **not** on `__unaligned` because that's 
a constraint. This takes an extra "element size" parameter, which we hope folks 
don't confuse with the size parameter (I'd expect a template or macro wrapper 
to hide that extra parameter when actually using the builtin).

Of course, that's two versions for each of `memcpy`, `memmove`, `memset`, and 
any other `*mem` that we decide to add to this list of overloadable functions.

Is that correct?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D79279#2169522 , @jfb wrote:

> In D79279#2168649 , @rjmccall wrote:
>
> > In D79279#2168533 , @jfb wrote:
> >
> > > In D79279#2168479 , @rjmccall 
> > > wrote:
> > >
> > > > Is there a need for an atomic memcpy at all?  Why is it useful to allow 
> > > > this operation to take on "atomic" semantics — which aren't actually 
> > > > atomic because the loads and stores to elements are torn — with 
> > > > hardcoded memory ordering and somewhat arbitrary rules about what the 
> > > > atomic size is?
> > >
> > >
> > > Hans lays out a rationale for usefulness in his paper, but what I've 
> > > implemented is more useful: it's *unordered* so you can fence as you 
> > > desire around it, yet it guarantees a minimum memory access size based on 
> > > the pointer parameters. For example, copying an atomic `int` will be 4 
> > > byte operations which are single-copy-atomic, but the accesses from one 
> > > `int` to the next aren't performed in any guaranteed order (or observable 
> > > in any guaranteed order either). I talked about this with him a while ago 
> > > but IIRC he wasn't sure about implementation among other things, so when 
> > > you asked me to widen my original `volatile`-only `memcpy` to also do 
> > > other qualifiers, I realized that it was a neat way to do atomic as well 
> > > as other qualifiers. I've talked to a few SG1 folks about this, and I 
> > > believe (for other reasons too) it's where the design will end up for 
> > > Hans' paper.
> >
> >
> > I can see the usefulness of this operation, but it seems like a odd 
> > semantic mismatch for what is basically just a memcpy where one of the 
> > pointers happens to have `_Atomic` type, like you're shoe-horning it into 
> > this builtin just to avoid declaring a different one.
>
>
> I'm following the discussion we had here regarding overloading 
> :
>
> >> There are other qualifiers that can meaningfully contribute to the 
> >> operation here besides volatile, such as restrict and (more importantly) 
> >> address spaces. And again, for the copy operations these might differ 
> >> between the two pointer types.
> >> 
> >> In both cases, I’d say that the logical design is to allow the pointers to 
> >> be to arbitrarily-qualified types. We can then propagate that information 
> >> from the builtin into the LLVM intrinsic call as best as we’re allowed. So 
> >> I think you should make builtins called something like 
> >> __builtin_overloaded_memcpy (name to be decided) and just have their 
> >> semantics be type-directed.
> > 
> > Ah yes, I’d like to hear what others think of this. I hadn’t thought about 
> > it before you brought it up, and it sounds like a good idea.
>
> As you noted earlier, for `memcpy` you probably want to express differences 
> in destination and source qualification, even if today IR can't express e.g. 
> `volatile` source and non-`volatile` destination. You were talking about 
> `volatile`, but this applies to the entire combination of dst+src qualified 
> with zero-to-five `volatile`, `_Atomic`, `__unaligned`, `restrict`, and 
> address space. Pulling the entire combination space out into different 
> functions would create way too many functions. Right now the implementation 
> has a few limitations: it treats both dst and src as `volatile` if either 
> are, it can't do `_Atomic` with `volatile` so we diagnose, and it ignores 
> `restrict`.  Otherwise it supports all combinations.


My point is that this has nothing to do with the ordinary semantics of 
`_Atomic`.  You're basically just looking at the word "atomic" and saying that, 
hey, a minimum access size is sortof related to atomicity.

If you want this to be able to control the minimum access size, you should 
allow that to be passed in as an optional argument instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 280136.
jfb added a comment.

Re-update


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuilder.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-overloaded-memfns.c
  clang/test/CodeGenObjC/builtin-memfns.m
  clang/test/Sema/builtin-overloaded-memfns.cpp
  clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
  clang/test/SemaOpenCL/to_addr_builtin.cl

Index: clang/test/SemaOpenCL/to_addr_builtin.cl
===
--- clang/test/SemaOpenCL/to_addr_builtin.cl
+++ clang/test/SemaOpenCL/to_addr_builtin.cl
@@ -15,7 +15,7 @@
   // expected-error@-2{{implicit declaration of function 'to_global' is invalid in OpenCL}}
   // expected-warning@-3{{incompatible integer to pointer conversion assigning to '__global int *__private' from 'int'}}
 #else
-  // expected-error@-5{{invalid number of arguments to function: 'to_global'}}
+  // expected-error@-5{{too many arguments to function call, expected 1, have 2}}
 #endif
 
   int x;
Index: clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
===
--- clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
+++ clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
@@ -10,7 +10,7 @@
   read_pipe(p, );
   read_pipe(p, ptr);
   read_pipe(tmp, p);// expected-error {{first argument to 'read_pipe' must be a pipe type}}
-  read_pipe(p);   // expected-error {{invalid number of arguments to function: 'read_pipe'}}
+  read_pipe(p); // expected-error {{invalid number of arguments to function: 'read_pipe'}}
   read_pipe(p, rid, tmp, ptr);
   read_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function 'read_pipe' (expecting 'reserve_id_t' having '__private int')}}
   read_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function 'read_pipe' (expecting 'unsigned int' having '__private reserve_id_t')}}
@@ -39,7 +39,7 @@
   write_pipe(p, );
   write_pipe(p, ptr);
   write_pipe(tmp, p);// expected-error {{first argument to 'write_pipe' must be a pipe type}}
-  write_pipe(p);   // expected-error {{invalid number of arguments to function: 'write_pipe'}}
+  write_pipe(p); // expected-error {{invalid number of arguments to function: 'write_pipe'}}
   write_pipe(p, rid, tmp, ptr);
   write_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function 'write_pipe' (expecting 'reserve_id_t' having '__private int')}}
   write_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function 'write_pipe' (expecting 'unsigned int' having '__private reserve_id_t')}}
Index: clang/test/Sema/builtin-overloaded-memfns.cpp
===
--- /dev/null
+++ clang/test/Sema/builtin-overloaded-memfns.cpp
@@ -0,0 +1,222 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -triple=arm64-unknown-unknown -fms-extensions -DCPY=1
+// RUN: %clang_cc1 %s -verify -fsyntax-only -triple=arm64-unknown-unknown -fms-extensions -DCPY=0
+
+// Test memcpy and memmove with the same code, since they're basically the same constraints.
+#if CPY
+#define MEM(...) __builtin_memcpy_overloaded(__VA_ARGS__)
+#else
+#define MEM(...) __builtin_memmove_overloaded(__VA_ARGS__)
+#endif
+
+#define NULL (void *)0
+#define nullptr __nullptr
+using size_t = __SIZE_TYPE__;
+using sizeless_t = __SVInt8_t;
+using float4 = float __attribute__((ext_vector_type(4)));
+struct Intish {
+  int i;
+};
+struct NotLockFree {
+  char buf[512];
+};
+struct TrivialCpy {
+  char buf[8];
+  TrivialCpy();
+  TrivialCpy(const TrivialCpy &) = default;
+};
+struct NotTrivialCpy {
+  char buf[8];
+  NotTrivialCpy();
+  NotTrivialCpy(const NotTrivialCpy &);
+};
+
+void arg_count() {
+  MEM();   // expected-error {{too few arguments to function call, expected 3, have 0}}
+  MEM(0);  // expected-error {{too few arguments to function call, expected 3, have 1}}
+  MEM(0, 0);   // expected-error {{too few arguments to function call, expected 3, have 2}}
+  MEM(0, 0, 0, 0); // expected-error {{too many arguments to function call, expected 3, have 4}}
+  __builtin_memset_overloaded();   // expected-error {{too few arguments to function call, expected 3, have 0}}
+  __builtin_memset_overloaded(0);  // expected-error {{too few arguments to function call, expected 3, have 1}}
+  __builtin_memset_overloaded(0, 0);   // expected-error {{too few arguments to function call, expected 3, have 2}}
+  

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

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 280135.
jfb added a comment.

Improve documentation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

Files:
  clang/docs/LanguageExtensions.rst


Index: clang/docs/LanguageExtensions.rst
===
--- clang/docs/LanguageExtensions.rst
+++ clang/docs/LanguageExtensions.rst
@@ -2454,6 +2454,14 @@
 * Implement an atomic memory with atomic operations of a particular size,
   similar to that presented in C++ proposal [p1478](https://wg21.link/p1478).
 
+When using the `_Atomic` qualifier, the memory will be accessed with a sequence
+of operations of size equal to or a multiple of the pointer's element size. The
+order of operations is unspecified, and has unordered atomic semantics. This
+means that reads and writes do not tear at the individual element level, and
+they each occur exactly once, but the order in which they occur can only be
+guaranteed using appropriate fences. Atomic memory operations must be to memory
+locations which are aligned to no less than the element size.
+
 Atomic Min/Max builtins with memory ordering
 
 


Index: clang/docs/LanguageExtensions.rst
===
--- clang/docs/LanguageExtensions.rst
+++ clang/docs/LanguageExtensions.rst
@@ -2454,6 +2454,14 @@
 * Implement an atomic memory with atomic operations of a particular size,
   similar to that presented in C++ proposal [p1478](https://wg21.link/p1478).
 
+When using the `_Atomic` qualifier, the memory will be accessed with a sequence
+of operations of size equal to or a multiple of the pointer's element size. The
+order of operations is unspecified, and has unordered atomic semantics. This
+means that reads and writes do not tear at the individual element level, and
+they each occur exactly once, but the order in which they occur can only be
+guaranteed using appropriate fences. Atomic memory operations must be to memory
+locations which are aligned to no less than the element size.
+
 Atomic Min/Max builtins with memory ordering
 
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D79279#2168649 , @rjmccall wrote:

> In D79279#2168533 , @jfb wrote:
>
> > In D79279#2168479 , @rjmccall 
> > wrote:
> >
> > > Is there a need for an atomic memcpy at all?  Why is it useful to allow 
> > > this operation to take on "atomic" semantics — which aren't actually 
> > > atomic because the loads and stores to elements are torn — with hardcoded 
> > > memory ordering and somewhat arbitrary rules about what the atomic size 
> > > is?
> >
> >
> > Hans lays out a rationale for usefulness in his paper, but what I've 
> > implemented is more useful: it's *unordered* so you can fence as you desire 
> > around it, yet it guarantees a minimum memory access size based on the 
> > pointer parameters. For example, copying an atomic `int` will be 4 byte 
> > operations which are single-copy-atomic, but the accesses from one `int` to 
> > the next aren't performed in any guaranteed order (or observable in any 
> > guaranteed order either). I talked about this with him a while ago but IIRC 
> > he wasn't sure about implementation among other things, so when you asked 
> > me to widen my original `volatile`-only `memcpy` to also do other 
> > qualifiers, I realized that it was a neat way to do atomic as well as other 
> > qualifiers. I've talked to a few SG1 folks about this, and I believe (for 
> > other reasons too) it's where the design will end up for Hans' paper.
>
>
> I can see the usefulness of this operation, but it seems like a odd semantic 
> mismatch for what is basically just a memcpy where one of the pointers 
> happens to have `_Atomic` type, like you're shoe-horning it into this builtin 
> just to avoid declaring a different one.


I'm following the discussion we had here regarding overloading 
:

>> There are other qualifiers that can meaningfully contribute to the operation 
>> here besides volatile, such as restrict and (more importantly) address 
>> spaces. And again, for the copy operations these might differ between the 
>> two pointer types.
>> 
>> In both cases, I’d say that the logical design is to allow the pointers to 
>> be to arbitrarily-qualified types. We can then propagate that information 
>> from the builtin into the LLVM intrinsic call as best as we’re allowed. So I 
>> think you should make builtins called something like 
>> __builtin_overloaded_memcpy (name to be decided) and just have their 
>> semantics be type-directed.
> 
> Ah yes, I’d like to hear what others think of this. I hadn’t thought about it 
> before you brought it up, and it sounds like a good idea.

As you noted earlier, for `memcpy` you probably want to express differences in 
destination and source qualification, even if today IR can't express e.g. 
`volatile` source and non-`volatile` destination. You were talking about 
`volatile`, but this applies to the entire combination of dst+src qualified 
with zero-to-five `volatile`, `_Atomic`, `__unaligned`, `restrict`, and address 
space. Pulling the entire combination space out into different functions would 
create way too many functions. Right now the implementation has a few 
limitations: it treats both dst and src as `volatile` if either are, it can't 
do `_Atomic` with `volatile` so we diagnose, and it ignores `restrict`.  
Otherwise it supports all combinations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D79279#2168533 , @jfb wrote:

> In D79279#2168479 , @rjmccall wrote:
>
> > Is there a need for an atomic memcpy at all?  Why is it useful to allow 
> > this operation to take on "atomic" semantics — which aren't actually atomic 
> > because the loads and stores to elements are torn — with hardcoded memory 
> > ordering and somewhat arbitrary rules about what the atomic size is?
>
>
> Hans lays out a rationale for usefulness in his paper, but what I've 
> implemented is more useful: it's *unordered* so you can fence as you desire 
> around it, yet it guarantees a minimum memory access size based on the 
> pointer parameters. For example, copying an atomic `int` will be 4 byte 
> operations which are single-copy-atomic, but the accesses from one `int` to 
> the next aren't performed in any guaranteed order (or observable in any 
> guaranteed order either). I talked about this with him a while ago but IIRC 
> he wasn't sure about implementation among other things, so when you asked me 
> to widen my original `volatile`-only `memcpy` to also do other qualifiers, I 
> realized that it was a neat way to do atomic as well as other qualifiers. 
> I've talked to a few SG1 folks about this, and I believe (for other reasons 
> too) it's where the design will end up for Hans' paper.


I can see the usefulness of this operation, but it seems like a odd semantic 
mismatch for what is basically just a memcpy where one of the pointers happens 
to have `_Atomic` type, like you're shoe-horning it into this builtin just to 
avoid declaring a different one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D79279#2168479 , @rjmccall wrote:

> Is there a need for an atomic memcpy at all?  Why is it useful to allow this 
> operation to take on "atomic" semantics — which aren't actually atomic 
> because the loads and stores to elements are torn — with hardcoded memory 
> ordering and somewhat arbitrary rules about what the atomic size is?


Hans lays out a rationale for usefulness in his paper, but what I've 
implemented is more useful: it's *unordered* so you can fence as you desire 
around it, yet it guarantees a minimum memory access size based on the pointer 
parameters. For example, copying an atomic `int` will be 4 byte operations 
which are single-copy-atomic, but the accesses from one `int` to the next 
aren't performed in any guaranteed order (or observable in any guaranteed order 
either). I talked about this with him a while ago but IIRC he wasn't sure about 
implementation among other things, so when you asked me to widen my original 
`volatile`-only `memcpy` to also do other qualifiers, I realized that it was a 
neat way to do atomic as well as other qualifiers. I've talked to a few SG1 
folks about this, and I believe (for other reasons too) it's where the design 
will end up for Hans' paper.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Is there a need for an atomic memcpy at all?  Why is it useful to allow this 
operation to take on "atomic" semantics — which aren't actually atomic because 
the loads and stores to elements are torn — with hardcoded memory ordering and 
somewhat arbitrary rules about what the atomic size is?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8917
+
+def err_atomic_type_must_be_lock_free : Error<"_Atomic type must always be 
lock-free, %0 isn't">;
+

rjmccall wrote:
> I don't know why you're adding a bunch of new diagnostics about _Atomic.
Maybe the tests clarify this? Here's my rationale for the 3 new atomic 
diagnostics:

* Don't support mixing `volatile` and `atomic`, because we'd need to add IR 
support for it. It might be useful, but as a follow-up.
* Overloaded `memcpy` figures out the atomic operation size based on the 
element's own size. There's a destination and a source pointer, and we can't 
figure out the expected atomic operation size if they differ. It's likely an 
unintentional error to have different sizes when doing an atomic `memcpy`, so 
instead of figuring out the largest common matching size I figure it's better 
to diagnose.
* Supporting non-lock-free sizes seems fraught with peril, since it's likely 
unintentional. It's certainly doable (loop call the runtime support), but it's 
unclear if we should take the lock just once over the entire loop, or once for 
load+store, or once for load and once for store. I don't see a point in 
supporting it.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:636
+  return ArgTy->castAs()->getPointeeType();
+}
+

rjmccall wrote:
> Since arrays are handled separately now, this is just `getPointeeType()`, but 
> I don't know why you need to support ObjC object pointer types here at all.
I'll remove ObjC handling for now, I added it because of code like what's in:
clang/test/CodeGenObjC/builtin-memfns.m
```
// PR13697
void cpy1(int *a, id b) {
  // CHECK-LABEL: @cpy1(
  // CHECK: call void @llvm.memcpy.p0i8.p0i8.i64(i8* {{.*}}, i8* {{.*}}, i64 8, 
i1 false)
  memcpy(a, b, 8);
}
```
Should we support this? It seems to me like yes, but you seem to think 
otherwise?

On arrays / ObjC being handled now: that's not really true... or rather, it now 
is for the builtins I'm adding, but not for the previously existing builtins. 
We can't just get the pointer argument type for this code:
```
// 
// Make sure we don't over-estimate the alignment of fields of
// packed structs.
struct PS {
  int modes[4];
} __attribute__((packed));
struct PS ps;
void test8(int *arg) {
  // CHECK: @test8
  // CHECK: call void @llvm.memcpy{{.*}} align 4 {{.*}} align 1 {{.*}} 16, i1 
false)
  __builtin_memcpy(arg, ps.modes, sizeof(struct PS));
}
```

Because `__builtin_memcpy` doesn't perform the conversion. Arguable a 
pre-existing bug, which I can patch here as I have, or fix in Sema if you'd 
rather see that? LMK.



Comment at: clang/lib/Sema/SemaChecking.cpp:5468
+  bool isAtomic = (DstTy && DstValTy->isAtomicType()) ||
+  (SrcTy && SrcValTy->isAtomicType());
+

rjmccall wrote:
> You already know that DstTy and SrcTy are non-null here.
> 
> Why do you need to support atomic types for these operations anyway?  It just 
> seems treacherous and unnecessary.
Leftover from the refactoring :)

It's useful to get atomic memcpy, see https://wg21.link/P1478
It's also part of "support overloaded memcpy" which is what doing more than 
`volatile` implies.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 279984.
jfb marked 7 inline comments as done.
jfb added a comment.

Address all but one of John's comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuilder.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-overloaded-memfns.c
  clang/test/CodeGenObjC/builtin-memfns.m
  clang/test/Sema/builtin-overloaded-memfns.cpp
  clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
  clang/test/SemaOpenCL/to_addr_builtin.cl

Index: clang/test/SemaOpenCL/to_addr_builtin.cl
===
--- clang/test/SemaOpenCL/to_addr_builtin.cl
+++ clang/test/SemaOpenCL/to_addr_builtin.cl
@@ -15,7 +15,7 @@
   // expected-error@-2{{implicit declaration of function 'to_global' is invalid in OpenCL}}
   // expected-warning@-3{{incompatible integer to pointer conversion assigning to '__global int *__private' from 'int'}}
 #else
-  // expected-error@-5{{invalid number of arguments to function: 'to_global'}}
+  // expected-error@-5{{too many arguments to function call, expected 1, have 2}}
 #endif
 
   int x;
Index: clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
===
--- clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
+++ clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
@@ -10,7 +10,7 @@
   read_pipe(p, );
   read_pipe(p, ptr);
   read_pipe(tmp, p);// expected-error {{first argument to 'read_pipe' must be a pipe type}}
-  read_pipe(p);   // expected-error {{invalid number of arguments to function: 'read_pipe'}}
+  read_pipe(p); // expected-error {{invalid number of arguments to function: 'read_pipe'}}
   read_pipe(p, rid, tmp, ptr);
   read_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function 'read_pipe' (expecting 'reserve_id_t' having '__private int')}}
   read_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function 'read_pipe' (expecting 'unsigned int' having '__private reserve_id_t')}}
@@ -39,7 +39,7 @@
   write_pipe(p, );
   write_pipe(p, ptr);
   write_pipe(tmp, p);// expected-error {{first argument to 'write_pipe' must be a pipe type}}
-  write_pipe(p);   // expected-error {{invalid number of arguments to function: 'write_pipe'}}
+  write_pipe(p); // expected-error {{invalid number of arguments to function: 'write_pipe'}}
   write_pipe(p, rid, tmp, ptr);
   write_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function 'write_pipe' (expecting 'reserve_id_t' having '__private int')}}
   write_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function 'write_pipe' (expecting 'unsigned int' having '__private reserve_id_t')}}
Index: clang/test/Sema/builtin-overloaded-memfns.cpp
===
--- /dev/null
+++ clang/test/Sema/builtin-overloaded-memfns.cpp
@@ -0,0 +1,222 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -triple=arm64-unknown-unknown -fms-extensions -DCPY=1
+// RUN: %clang_cc1 %s -verify -fsyntax-only -triple=arm64-unknown-unknown -fms-extensions -DCPY=0
+
+// Test memcpy and memmove with the same code, since they're basically the same constraints.
+#if CPY
+#define MEM(...) __builtin_memcpy_overloaded(__VA_ARGS__)
+#else
+#define MEM(...) __builtin_memmove_overloaded(__VA_ARGS__)
+#endif
+
+#define NULL (void *)0
+#define nullptr __nullptr
+using size_t = __SIZE_TYPE__;
+using sizeless_t = __SVInt8_t;
+using float4 = float __attribute__((ext_vector_type(4)));
+struct Intish {
+  int i;
+};
+struct NotLockFree {
+  char buf[512];
+};
+struct TrivialCpy {
+  char buf[8];
+  TrivialCpy();
+  TrivialCpy(const TrivialCpy &) = default;
+};
+struct NotTrivialCpy {
+  char buf[8];
+  NotTrivialCpy();
+  NotTrivialCpy(const NotTrivialCpy &);
+};
+
+void arg_count() {
+  MEM();   // expected-error {{too few arguments to function call, expected 3, have 0}}
+  MEM(0);  // expected-error {{too few arguments to function call, expected 3, have 1}}
+  MEM(0, 0);   // expected-error {{too few arguments to function call, expected 3, have 2}}
+  MEM(0, 0, 0, 0); // expected-error {{too many arguments to function call, expected 3, have 4}}
+  __builtin_memset_overloaded();   // expected-error {{too few arguments to function call, expected 3, have 0}}
+  __builtin_memset_overloaded(0);  // expected-error {{too few arguments to function call, expected 3, have 1}}
+  __builtin_memset_overloaded(0, 0);   // expected-error 

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

You need to add user docs for these builtins.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8917
+
+def err_atomic_type_must_be_lock_free : Error<"_Atomic type must always be 
lock-free, %0 isn't">;
+

I don't know why you're adding a bunch of new diagnostics about _Atomic.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:636
+  return ArgTy->castAs()->getPointeeType();
+}
+

Since arrays are handled separately now, this is just `getPointeeType()`, but I 
don't know why you need to support ObjC object pointer types here at all.



Comment at: clang/lib/CodeGen/CGExpr.cpp:1070
   // We allow this with ObjC object pointers because of fragile ABIs.
-  assert(E->getType()->isPointerType() ||
+  assert(E->getType()->isPointerType() || E->getType()->isArrayType() ||
  E->getType()->isObjCObjectPointerType());

Why arrays?



Comment at: clang/lib/Sema/SemaChecking.cpp:5446
+return ExprError();
+  clang::Expr *SrcOp = SrcPtr.get();
+

Do you ever write these back into the call?



Comment at: clang/lib/Sema/SemaChecking.cpp:5468
+  bool isAtomic = (DstTy && DstValTy->isAtomicType()) ||
+  (SrcTy && SrcValTy->isAtomicType());
+

You already know that DstTy and SrcTy are non-null here.

Why do you need to support atomic types for these operations anyway?  It just 
seems treacherous and unnecessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 279896.
jfb added a comment.

Follow John's suggestions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuilder.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-overloaded-memfns.c
  clang/test/CodeGenObjC/builtin-memfns.m
  clang/test/Sema/builtin-overloaded-memfns.cpp
  clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
  clang/test/SemaOpenCL/to_addr_builtin.cl

Index: clang/test/SemaOpenCL/to_addr_builtin.cl
===
--- clang/test/SemaOpenCL/to_addr_builtin.cl
+++ clang/test/SemaOpenCL/to_addr_builtin.cl
@@ -15,7 +15,7 @@
   // expected-error@-2{{implicit declaration of function 'to_global' is invalid in OpenCL}}
   // expected-warning@-3{{incompatible integer to pointer conversion assigning to '__global int *__private' from 'int'}}
 #else
-  // expected-error@-5{{invalid number of arguments to function: 'to_global'}}
+  // expected-error@-5{{too many arguments to function call, expected 1, have 2}}
 #endif
 
   int x;
Index: clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
===
--- clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
+++ clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
@@ -10,7 +10,7 @@
   read_pipe(p, );
   read_pipe(p, ptr);
   read_pipe(tmp, p);// expected-error {{first argument to 'read_pipe' must be a pipe type}}
-  read_pipe(p);   // expected-error {{invalid number of arguments to function: 'read_pipe'}}
+  read_pipe(p); // expected-error {{invalid number of arguments to function: 'read_pipe'}}
   read_pipe(p, rid, tmp, ptr);
   read_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function 'read_pipe' (expecting 'reserve_id_t' having '__private int')}}
   read_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function 'read_pipe' (expecting 'unsigned int' having '__private reserve_id_t')}}
@@ -39,7 +39,7 @@
   write_pipe(p, );
   write_pipe(p, ptr);
   write_pipe(tmp, p);// expected-error {{first argument to 'write_pipe' must be a pipe type}}
-  write_pipe(p);   // expected-error {{invalid number of arguments to function: 'write_pipe'}}
+  write_pipe(p); // expected-error {{invalid number of arguments to function: 'write_pipe'}}
   write_pipe(p, rid, tmp, ptr);
   write_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function 'write_pipe' (expecting 'reserve_id_t' having '__private int')}}
   write_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function 'write_pipe' (expecting 'unsigned int' having '__private reserve_id_t')}}
Index: clang/test/Sema/builtin-overloaded-memfns.cpp
===
--- /dev/null
+++ clang/test/Sema/builtin-overloaded-memfns.cpp
@@ -0,0 +1,222 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -triple=arm64-unknown-unknown -fms-extensions -DCPY=1
+// RUN: %clang_cc1 %s -verify -fsyntax-only -triple=arm64-unknown-unknown -fms-extensions -DCPY=0
+
+// Test memcpy and memmove with the same code, since they're basically the same constraints.
+#if CPY
+#define MEM(...) __builtin_memcpy_overloaded(__VA_ARGS__)
+#else
+#define MEM(...) __builtin_memmove_overloaded(__VA_ARGS__)
+#endif
+
+#define NULL (void *)0
+#define nullptr __nullptr
+using size_t = __SIZE_TYPE__;
+using sizeless_t = __SVInt8_t;
+using float4 = float __attribute__((ext_vector_type(4)));
+struct Intish {
+  int i;
+};
+struct NotLockFree {
+  char buf[512];
+};
+struct TrivialCpy {
+  char buf[8];
+  TrivialCpy();
+  TrivialCpy(const TrivialCpy &) = default;
+};
+struct NotTrivialCpy {
+  char buf[8];
+  NotTrivialCpy();
+  NotTrivialCpy(const NotTrivialCpy &);
+};
+
+void arg_count() {
+  MEM();   // expected-error {{too few arguments to function call, expected 3, have 0}}
+  MEM(0);  // expected-error {{too few arguments to function call, expected 3, have 1}}
+  MEM(0, 0);   // expected-error {{too few arguments to function call, expected 3, have 2}}
+  MEM(0, 0, 0, 0); // expected-error {{too many arguments to function call, expected 3, have 4}}
+  __builtin_memset_overloaded();   // expected-error {{too few arguments to function call, expected 3, have 0}}
+  __builtin_memset_overloaded(0);  // expected-error {{too few arguments to function call, expected 3, have 1}}
+  __builtin_memset_overloaded(0, 0);   // expected-error {{too few arguments to function call, expected 3, have 2}}
+  

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

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet added inline comments.



Comment at: clang/include/clang/Basic/Builtins.def:489
 BUILTIN(__builtin_memcpy_inline, "vv*vC*Iz", "nt")
+BUILTIN(__builtin_overloaded_memcpy, "v*v*vC*z", "nt")
 BUILTIN(__builtin_memmove, "v*v*vC*z", "nF")

jfb wrote:
> gchatelet wrote:
> > `overloaded` doesn't bring much semantic (says the one who added 
> > `__builtin_memcpy_inline`...). Can you come up with something that 
> > describes more precisely what the intends are?
> > 
> > Also `memset`, `memcmp`, `memcpy`, `memmove` will have their `inline` and 
> > `overloaded` versions. This is becoming a crowded space. It may be 
> > confusing in the long run. If we want to go in that direction maybe we 
> > should come up with a consistent pattern: `__builtin__`. 
> > WDYT?
> Flipping it around is fine with me, see update (done with `sed`).
> 
> What's your approach on choosing what gets an `inline` variant and what 
> doesn't? `memcmp` is easy to add, but I wonder how far it's useful to go... I 
> can just wait for requests as well (as I imagine you're doing?).
I don't see `memmove_inline` being useful but memset and memcmp would make 
sense to add as building blocks for C++ implementations (e.g. [[ 
https://github.com/llvm/llvm-project/blob/master/libc/src/string/memcpy.cpp | 
libc memcpy ]])

As for this new addition, how about `__builtin_memcpy_honor_qualifiers`?
I fear that `__builtin_memcpy_overloaded` is too ambiguous.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:1605
+   }));
+  };
+

I am not a fan of this lambda style, not because I dislike lambdas, but because 
you've pulled a ton of code that's supporting one or two cases (that could 
easily be handled together) into a much wider scope.

Your helper code are doing a ton of redundant type checks and is probably not 
as general as you think it is.  You need to call 
`DefaultFunctionArrayLvalueConversion` on the pointer arguments, after which 
you can just check for a pointer type.  You also need to convert the size 
argument to a `size_t` as if initializing a parameter.  If you do these things, 
the IRGen code will get much simpler because e.g. it will not need to specially 
handle arrays anymore.  You will also start magically doing the right thing 
w.r.t ODR-uses of constexpr variables.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 275266.
jfb marked 18 inline comments as done.
jfb added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuilder.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-overloaded-memfns.c
  clang/test/CodeGenObjC/builtin-memfns.m
  clang/test/Sema/builtin-overloaded-memfns.cpp
  clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
  clang/test/SemaOpenCL/to_addr_builtin.cl

Index: clang/test/SemaOpenCL/to_addr_builtin.cl
===
--- clang/test/SemaOpenCL/to_addr_builtin.cl
+++ clang/test/SemaOpenCL/to_addr_builtin.cl
@@ -15,7 +15,7 @@
   // expected-error@-2{{implicit declaration of function 'to_global' is invalid in OpenCL}}
   // expected-warning@-3{{incompatible integer to pointer conversion assigning to '__global int *__private' from 'int'}}
 #else
-  // expected-error@-5{{invalid number of arguments to function: 'to_global'}}
+  // expected-error@-5{{too many arguments to function call, expected 1, have 2}}
 #endif
 
   int x;
Index: clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
===
--- clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
+++ clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
@@ -10,7 +10,7 @@
   read_pipe(p, );
   read_pipe(p, ptr);
   read_pipe(tmp, p);// expected-error {{first argument to 'read_pipe' must be a pipe type}}
-  read_pipe(p);   // expected-error {{invalid number of arguments to function: 'read_pipe'}}
+  read_pipe(p); // expected-error {{invalid number of arguments to function: 'read_pipe'}}
   read_pipe(p, rid, tmp, ptr);
   read_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function 'read_pipe' (expecting 'reserve_id_t' having '__private int')}}
   read_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function 'read_pipe' (expecting 'unsigned int' having '__private reserve_id_t')}}
@@ -39,7 +39,7 @@
   write_pipe(p, );
   write_pipe(p, ptr);
   write_pipe(tmp, p);// expected-error {{first argument to 'write_pipe' must be a pipe type}}
-  write_pipe(p);   // expected-error {{invalid number of arguments to function: 'write_pipe'}}
+  write_pipe(p); // expected-error {{invalid number of arguments to function: 'write_pipe'}}
   write_pipe(p, rid, tmp, ptr);
   write_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function 'write_pipe' (expecting 'reserve_id_t' having '__private int')}}
   write_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function 'write_pipe' (expecting 'unsigned int' having '__private reserve_id_t')}}
Index: clang/test/Sema/builtin-overloaded-memfns.cpp
===
--- /dev/null
+++ clang/test/Sema/builtin-overloaded-memfns.cpp
@@ -0,0 +1,220 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -triple=arm64-unknown-unknown -fms-extensions -DCPY=1
+// RUN: %clang_cc1 %s -verify -fsyntax-only -triple=arm64-unknown-unknown -fms-extensions -DCPY=0
+
+// Test memcpy and memmove with the same code, since they're basically the same constraints.
+#if CPY
+#define MEM(...) __builtin_memcpy_overloaded(__VA_ARGS__)
+#else
+#define MEM(...) __builtin_memmove_overloaded(__VA_ARGS__)
+#endif
+
+#define NULL (void *)0
+#define nullptr __nullptr
+using size_t = __SIZE_TYPE__;
+using sizeless_t = __SVInt8_t;
+using float4 = float __attribute__((ext_vector_type(4)));
+struct Intish {
+  int i;
+};
+struct NotLockFree {
+  char buf[512];
+};
+struct TrivialCpy {
+  char buf[8];
+  TrivialCpy();
+  TrivialCpy(const TrivialCpy &) = default;
+};
+struct NotTrivialCpy {
+  char buf[8];
+  NotTrivialCpy();
+  NotTrivialCpy(const NotTrivialCpy &);
+};
+
+void arg_count() {
+  MEM();   // expected-error {{too few arguments to function call, expected 3, have 0}}
+  MEM(0);  // expected-error {{too few arguments to function call, expected 3, have 1}}
+  MEM(0, 0);   // expected-error {{too few arguments to function call, expected 3, have 2}}
+  MEM(0, 0, 0, 0); // expected-error {{too many arguments to function call, expected 3, have 4}}
+  __builtin_memset_overloaded();   // expected-error {{too few arguments to function call, expected 3, have 0}}
+  __builtin_memset_overloaded(0);  // expected-error {{too few arguments to function call, expected 3, have 1}}
+  __builtin_memset_overloaded(0, 0);   // expected-error {{too few arguments to function call, expected 3, have 2}}
+  

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

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: clang/include/clang/Basic/Builtins.def:489
 BUILTIN(__builtin_memcpy_inline, "vv*vC*Iz", "nt")
+BUILTIN(__builtin_overloaded_memcpy, "v*v*vC*z", "nt")
 BUILTIN(__builtin_memmove, "v*v*vC*z", "nF")

gchatelet wrote:
> `overloaded` doesn't bring much semantic (says the one who added 
> `__builtin_memcpy_inline`...). Can you come up with something that describes 
> more precisely what the intends are?
> 
> Also `memset`, `memcmp`, `memcpy`, `memmove` will have their `inline` and 
> `overloaded` versions. This is becoming a crowded space. It may be confusing 
> in the long run. If we want to go in that direction maybe we should come up 
> with a consistent pattern: `__builtin__`. WDYT?
Flipping it around is fine with me, see update (done with `sed`).

What's your approach on choosing what gets an `inline` variant and what 
doesn't? `memcmp` is easy to add, but I wonder how far it's useful to go... I 
can just wait for requests as well (as I imagine you're doing?).



Comment at: clang/lib/Sema/SemaChecking.cpp:1442
 
+  enum class MemCheckType { Full, Basic };
+

erichkeane wrote:
> Oh boy... all these lambdas are making me squeamish. 
C++14 



Comment at: clang/lib/Sema/SemaChecking.cpp:1444
+
+  auto CheckArityIs = [&](unsigned ExpectedArity) {
+if (TheCall->getNumArgs() == ExpectedArity)

erichkeane wrote:
> What is wrong with CheckArgCount (static function at the top of the file)?  
> It seems to do some nice additions here too.
It is most wonderful and has now taken over for valiant `CheckArityIs`. I'd 
somehow not seen that! I had gripped for another error message and figured this 
was what I needed.



Comment at: clang/lib/Sema/SemaChecking.cpp:1452
+  };
+  auto getPointeeOrArrayType = [&](clang::Expr *E) {
+if (E->getType()->isArrayType())

erichkeane wrote:
> What is this doing that ->getType()->getPointeeOrArrayElementType() doesn't 
> do?
It keeps the qualifiers 
Maybe I can make a separate `QualType` helper that does this?



Comment at: clang/lib/Sema/SemaChecking.cpp:1465
+Expr::EvalResult Result;
+if (E->getType()->isIntegerType() && !E->isValueDependent() &&
+E->EvaluateAsInt(Result, Context) && (Result.Val.getInt() == 0))

erichkeane wrote:
> Why is a value-dependent expression not OK?  Typically you'd want to not 
> bother with dependence in the checking, and just check it during 
> instantiation (the 2nd time this is called).
> 
> Because it seems to me that this will error during phase 1 when an integer 
> template parameter (or 'auto' parameter?) would be fine later.
> 
> 
```
bool Expr::EvaluateAsInt(EvalResult , const ASTContext ,
 SideEffectsKind AllowSideEffects,
 bool InConstantContext) const {
  assert(!isValueDependent() &&
 "Expression evaluator can't be called on a dependent expression.");
  EvalInfo Info(Ctx, Result, EvalInfo::EM_IgnoreSideEffects);
  Info.InConstantContext = InConstantContext;
  return ::EvaluateAsInt(this, Result, Ctx, AllowSideEffects, Info);
}
```


It seems pretty common to use that check when trying to get a value out.



Comment at: clang/lib/Sema/SemaChecking.cpp:1509
+clang::Expr *E1) {
+if (!E0->getType()->isObjectPointerType() && !E0->getType()->isArrayType())
+  return true;

erichkeane wrote:
> What if 1 of them is of these types?  Is that OK?
It's to avoid weird corner cases where this check isn't super relevant, but 
subsequent ones are. It avoids making `isVolatileQualified` below sad because 
e.g. `void` makes the `QualType` null. That one can't be `_Atomic`, and it can 
be `volatile` but then the size won't match the `_Atomic`'s size.



Comment at: clang/lib/Sema/SemaChecking.cpp:1527
+   clang::Expr *E1) {
+if (!E0->getType()->isObjectPointerType() && !E0->getType()->isArrayType())
+  return true;

erichkeane wrote:
> Same question as above.  Is there other checks that need to happen here?  
> Also, is there any ability to reuse some of the logic between these funcitons?
I don't think so here either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I just took a look at SemaChecking, the rest I'm not sure I get the intent of 
the patch sufficiently to understand.




Comment at: clang/lib/Sema/SemaChecking.cpp:1442
 
+  enum class MemCheckType { Full, Basic };
+

Oh boy... all these lambdas are making me squeamish. 



Comment at: clang/lib/Sema/SemaChecking.cpp:1444
+
+  auto CheckArityIs = [&](unsigned ExpectedArity) {
+if (TheCall->getNumArgs() == ExpectedArity)

What is wrong with CheckArgCount (static function at the top of the file)?  It 
seems to do some nice additions here too.



Comment at: clang/lib/Sema/SemaChecking.cpp:1452
+  };
+  auto getPointeeOrArrayType = [&](clang::Expr *E) {
+if (E->getType()->isArrayType())

What is this doing that ->getType()->getPointeeOrArrayElementType() doesn't do?



Comment at: clang/lib/Sema/SemaChecking.cpp:1457
+  };
+  auto CheckIsObjectPointerOrArrayType = [&](clang::Expr *E) {
+if (E->getType()->isObjectPointerType())

Typically 'check' functions have the bool logic reversed.



Comment at: clang/lib/Sema/SemaChecking.cpp:1465
+Expr::EvalResult Result;
+if (E->getType()->isIntegerType() && !E->isValueDependent() &&
+E->EvaluateAsInt(Result, Context) && (Result.Val.getInt() == 0))

Why is a value-dependent expression not OK?  Typically you'd want to not bother 
with dependence in the checking, and just check it during instantiation (the 
2nd time this is called).

Because it seems to me that this will error during phase 1 when an integer 
template parameter (or 'auto' parameter?) would be fine later.





Comment at: clang/lib/Sema/SemaChecking.cpp:1472
+<< InitializedEntity::EK_Parameter << Context.VoidPtrTy << 
E->isLValue()
+<< E->getType() << 0 << E->getSourceRange();
+return false;

Put a comment in the message with some sort of hint as to what the '0' does.  
Typically a << /*Whatever*/ 0<<



Comment at: clang/lib/Sema/SemaChecking.cpp:1494
+  auto CheckIsTriviallyCopyablePointeeType = [&](clang::Expr *E) {
+if (!(E->getType()->isPointerType() || E->getType()->isArrayType()))
+  return true;

Can you invert this logic?  !Ptr && !Array?



Comment at: clang/lib/Sema/SemaChecking.cpp:1509
+clang::Expr *E1) {
+if (!E0->getType()->isObjectPointerType() && !E0->getType()->isArrayType())
+  return true;

What if 1 of them is of these types?  Is that OK?



Comment at: clang/lib/Sema/SemaChecking.cpp:1527
+   clang::Expr *E1) {
+if (!E0->getType()->isObjectPointerType() && !E0->getType()->isArrayType())
+  return true;

Same question as above.  Is there other checks that need to happen here?  Also, 
is there any ability to reuse some of the logic between these funcitons?



Comment at: clang/lib/Sema/SemaChecking.cpp:1533
+QualType El1Ty = getPointeeOrArrayType(E1);
+if (!(El0Ty->isAtomicType() || El1Ty->isAtomicType()))
+  return true;

invert these please.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet added inline comments.



Comment at: clang/include/clang/Basic/Builtins.def:489
 BUILTIN(__builtin_memcpy_inline, "vv*vC*Iz", "nt")
+BUILTIN(__builtin_overloaded_memcpy, "v*v*vC*z", "nt")
 BUILTIN(__builtin_memmove, "v*v*vC*z", "nF")

`overloaded` doesn't bring much semantic (says the one who added 
`__builtin_memcpy_inline`...). Can you come up with something that describes 
more precisely what the intends are?

Also `memset`, `memcmp`, `memcpy`, `memmove` will have their `inline` and 
`overloaded` versions. This is becoming a crowded space. It may be confusing in 
the long run. If we want to go in that direction maybe we should come up with a 
consistent pattern: `__builtin__`. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 275005.
jfb added a comment.

Add memmove and memset (but still missing the codegen tests)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuilder.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-overloaded-memfns.c
  clang/test/CodeGenObjC/builtin-memfns.m
  clang/test/Sema/builtin-overloaded-memfns.cpp
  clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
  clang/test/SemaOpenCL/to_addr_builtin.cl

Index: clang/test/SemaOpenCL/to_addr_builtin.cl
===
--- clang/test/SemaOpenCL/to_addr_builtin.cl
+++ clang/test/SemaOpenCL/to_addr_builtin.cl
@@ -15,7 +15,7 @@
   // expected-error@-2{{implicit declaration of function 'to_global' is invalid in OpenCL}}
   // expected-warning@-3{{incompatible integer to pointer conversion assigning to '__global int *__private' from 'int'}}
 #else
-  // expected-error@-5{{invalid number of arguments to function: 'to_global'}}
+  // expected-error@-5{{too many arguments to function call, expected 1, have 2}}
 #endif
 
   int x;
Index: clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
===
--- clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
+++ clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
@@ -10,7 +10,7 @@
   read_pipe(p, );
   read_pipe(p, ptr);
   read_pipe(tmp, p);// expected-error {{first argument to 'read_pipe' must be a pipe type}}
-  read_pipe(p);   // expected-error {{invalid number of arguments to function: 'read_pipe'}}
+  read_pipe(p); // expected-error {{too few arguments to function call, expected 2 or 4, have 1}}
   read_pipe(p, rid, tmp, ptr);
   read_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function 'read_pipe' (expecting 'reserve_id_t' having '__private int')}}
   read_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function 'read_pipe' (expecting 'unsigned int' having '__private reserve_id_t')}}
@@ -39,7 +39,7 @@
   write_pipe(p, );
   write_pipe(p, ptr);
   write_pipe(tmp, p);// expected-error {{first argument to 'write_pipe' must be a pipe type}}
-  write_pipe(p);   // expected-error {{invalid number of arguments to function: 'write_pipe'}}
+  write_pipe(p); // expected-error {{too few arguments to function call, expected 2 or 4, have 1}}
   write_pipe(p, rid, tmp, ptr);
   write_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function 'write_pipe' (expecting 'reserve_id_t' having '__private int')}}
   write_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function 'write_pipe' (expecting 'unsigned int' having '__private reserve_id_t')}}
Index: clang/test/Sema/builtin-overloaded-memfns.cpp
===
--- /dev/null
+++ clang/test/Sema/builtin-overloaded-memfns.cpp
@@ -0,0 +1,158 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -triple=arm64-unknown-unknown -fms-extensions -DCPY=1
+// RUN: %clang_cc1 %s -verify -fsyntax-only -triple=arm64-unknown-unknown -fms-extensions -DCPY=0
+
+// Test memcpy and memmove with the same code, since they're basically the same constraints.
+#if CPY
+#define MEM(...) __builtin_overloaded_memcpy(__VA_ARGS__)
+#else
+#define MEM(...) __builtin_overloaded_memmove(__VA_ARGS__)
+#endif
+
+#define NULL (void *)0
+#define nullptr __nullptr
+using size_t = __SIZE_TYPE__;
+struct Intish {
+  int i;
+};
+struct TrivialCpy {
+  char buf[8];
+  TrivialCpy();
+  TrivialCpy(const TrivialCpy &) = default;
+};
+struct NotTrivialCpy {
+  char buf[8];
+  NotTrivialCpy();
+  NotTrivialCpy(const NotTrivialCpy &);
+};
+
+void arg_count() {
+  MEM();   // expected-error {{too few arguments to function call, expected 3, have 0}}
+  MEM(0);  // expected-error {{too few arguments to function call, expected 3, have 1}}
+  MEM(0, 0);   // expected-error {{too few arguments to function call, expected 3, have 2}}
+  MEM(0, 0, 0, 0); // expected-error {{too many arguments to function call, expected 3, have 4}}
+  __builtin_overloaded_memset();   // expected-error {{too few arguments to function call, expected 3, have 0}}
+  __builtin_overloaded_memset(0);  // expected-error {{too few arguments to function call, expected 3, have 1}}
+  __builtin_overloaded_memset(0, 0);   // expected-error {{too few arguments to function call, expected 3, have 2}}
+  __builtin_overloaded_memset(0, 0, 0, 0); // expected-error {{too many arguments to function call, expected 3, have 4}}
+}
+
+void null(char *dst, 

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

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 274948.
jfb added a comment.

This builtin doesn't return anything.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

Files:
  clang/include/clang/Basic/Builtins.def


Index: clang/include/clang/Basic/Builtins.def
===
--- clang/include/clang/Basic/Builtins.def
+++ clang/include/clang/Basic/Builtins.def
@@ -486,7 +486,7 @@
 BUILTIN(__builtin_memcmp, "ivC*vC*z", "nF")
 BUILTIN(__builtin_memcpy, "v*v*vC*z", "nF")
 BUILTIN(__builtin_memcpy_inline, "vv*vC*Iz", "nt")
-BUILTIN(__builtin_overloaded_memcpy, "v*v*vC*z", "nt")
+BUILTIN(__builtin_overloaded_memcpy, "vv*vC*z", "nt")
 BUILTIN(__builtin_memmove, "v*v*vC*z", "nF")
 BUILTIN(__builtin_mempcpy, "v*v*vC*z", "nF")
 BUILTIN(__builtin_memset, "v*v*iz", "nF")


Index: clang/include/clang/Basic/Builtins.def
===
--- clang/include/clang/Basic/Builtins.def
+++ clang/include/clang/Basic/Builtins.def
@@ -486,7 +486,7 @@
 BUILTIN(__builtin_memcmp, "ivC*vC*z", "nF")
 BUILTIN(__builtin_memcpy, "v*v*vC*z", "nF")
 BUILTIN(__builtin_memcpy_inline, "vv*vC*Iz", "nt")
-BUILTIN(__builtin_overloaded_memcpy, "v*v*vC*z", "nt")
+BUILTIN(__builtin_overloaded_memcpy, "vv*vC*z", "nt")
 BUILTIN(__builtin_memmove, "v*v*vC*z", "nF")
 BUILTIN(__builtin_mempcpy, "v*v*vC*z", "nF")
 BUILTIN(__builtin_memset, "v*v*iz", "nF")
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 274949.
jfb added a comment.

Arcanist ate the rest of my commit and I am confused.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuilder.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-overloaded-memfns.c
  clang/test/Sema/builtin-overloaded-memfns.cpp
  clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
  clang/test/SemaOpenCL/to_addr_builtin.cl

Index: clang/test/SemaOpenCL/to_addr_builtin.cl
===
--- clang/test/SemaOpenCL/to_addr_builtin.cl
+++ clang/test/SemaOpenCL/to_addr_builtin.cl
@@ -15,7 +15,7 @@
   // expected-error@-2{{implicit declaration of function 'to_global' is invalid in OpenCL}}
   // expected-warning@-3{{incompatible integer to pointer conversion assigning to '__global int *__private' from 'int'}}
 #else
-  // expected-error@-5{{invalid number of arguments to function: 'to_global'}}
+  // expected-error@-5{{too many arguments to function call, expected 1, have 2}}
 #endif
 
   int x;
Index: clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
===
--- clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
+++ clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
@@ -10,7 +10,7 @@
   read_pipe(p, );
   read_pipe(p, ptr);
   read_pipe(tmp, p);// expected-error {{first argument to 'read_pipe' must be a pipe type}}
-  read_pipe(p);   // expected-error {{invalid number of arguments to function: 'read_pipe'}}
+  read_pipe(p); // expected-error {{too few arguments to function call, expected 2 or 4, have 1}}
   read_pipe(p, rid, tmp, ptr);
   read_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function 'read_pipe' (expecting 'reserve_id_t' having '__private int')}}
   read_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function 'read_pipe' (expecting 'unsigned int' having '__private reserve_id_t')}}
@@ -39,7 +39,7 @@
   write_pipe(p, );
   write_pipe(p, ptr);
   write_pipe(tmp, p);// expected-error {{first argument to 'write_pipe' must be a pipe type}}
-  write_pipe(p);   // expected-error {{invalid number of arguments to function: 'write_pipe'}}
+  write_pipe(p); // expected-error {{too few arguments to function call, expected 2 or 4, have 1}}
   write_pipe(p, rid, tmp, ptr);
   write_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function 'write_pipe' (expecting 'reserve_id_t' having '__private int')}}
   write_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function 'write_pipe' (expecting 'unsigned int' having '__private reserve_id_t')}}
Index: clang/test/Sema/builtin-overloaded-memfns.cpp
===
--- /dev/null
+++ clang/test/Sema/builtin-overloaded-memfns.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -triple=arm64-unknown-unknown -fms-extensions
+
+#define NULL (void *)0
+#define nullptr __nullptr
+using size_t = __SIZE_TYPE__;
+struct Intish {
+  int i;
+};
+struct TrivialCpy {
+  char buf[8];
+  TrivialCpy();
+  TrivialCpy(const TrivialCpy &) = default;
+};
+struct NotTrivialCpy {
+  char buf[8];
+  NotTrivialCpy();
+  NotTrivialCpy(const NotTrivialCpy &);
+};
+
+void memcpy_arg_count() {
+  __builtin_overloaded_memcpy();   // expected-error {{too few arguments to function call, expected 3, have 0}}
+  __builtin_overloaded_memcpy(0);  // expected-error {{too few arguments to function call, expected 3, have 1}}
+  __builtin_overloaded_memcpy(0, 0);   // expected-error {{too few arguments to function call, expected 3, have 2}}
+  __builtin_overloaded_memcpy(0, 0, 0, 0); // expected-error {{too many arguments to function call, expected 3, have 4}}
+}
+
+void memcpy_null(char *dst, const char *src, size_t size) {
+  __builtin_overloaded_memcpy(0, 0, 0);
+  __builtin_overloaded_memcpy(0, 0, size);
+  __builtin_overloaded_memcpy(dst, 0, 42);   // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __builtin_overloaded_memcpy(dst, 0, 42);   // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __builtin_overloaded_memcpy(dst, NULL, 42);// expected-warning {{null passed to a callee that requires a non-null argument}}
+  __builtin_overloaded_memcpy(dst, nullptr, 42); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __builtin_overloaded_memcpy(0, src, 42);   // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __builtin_overloaded_memcpy(NULL, src, 42);// expected-warning {{null passed to a 

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

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 274847.
jfb edited the summary of this revision.
jfb added a comment.

Overload a new builtin instead. For now I only did memcpy, to get feedback on 
the approach. I'll add other mem* functions if this makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuilder.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-overloaded-memfns.c
  clang/test/Sema/builtin-overloaded-memfns.cpp
  clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
  clang/test/SemaOpenCL/to_addr_builtin.cl

Index: clang/test/SemaOpenCL/to_addr_builtin.cl
===
--- clang/test/SemaOpenCL/to_addr_builtin.cl
+++ clang/test/SemaOpenCL/to_addr_builtin.cl
@@ -15,7 +15,7 @@
   // expected-error@-2{{implicit declaration of function 'to_global' is invalid in OpenCL}}
   // expected-warning@-3{{incompatible integer to pointer conversion assigning to '__global int *__private' from 'int'}}
 #else
-  // expected-error@-5{{invalid number of arguments to function: 'to_global'}}
+  // expected-error@-5{{too many arguments to function call, expected 1, have 2}}
 #endif
 
   int x;
Index: clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
===
--- clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
+++ clang/test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
@@ -10,7 +10,7 @@
   read_pipe(p, );
   read_pipe(p, ptr);
   read_pipe(tmp, p);// expected-error {{first argument to 'read_pipe' must be a pipe type}}
-  read_pipe(p);   // expected-error {{invalid number of arguments to function: 'read_pipe'}}
+  read_pipe(p); // expected-error {{too few arguments to function call, expected 2 or 4, have 1}}
   read_pipe(p, rid, tmp, ptr);
   read_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function 'read_pipe' (expecting 'reserve_id_t' having '__private int')}}
   read_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function 'read_pipe' (expecting 'unsigned int' having '__private reserve_id_t')}}
@@ -39,7 +39,7 @@
   write_pipe(p, );
   write_pipe(p, ptr);
   write_pipe(tmp, p);// expected-error {{first argument to 'write_pipe' must be a pipe type}}
-  write_pipe(p);   // expected-error {{invalid number of arguments to function: 'write_pipe'}}
+  write_pipe(p); // expected-error {{too few arguments to function call, expected 2 or 4, have 1}}
   write_pipe(p, rid, tmp, ptr);
   write_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function 'write_pipe' (expecting 'reserve_id_t' having '__private int')}}
   write_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function 'write_pipe' (expecting 'unsigned int' having '__private reserve_id_t')}}
Index: clang/test/Sema/builtin-overloaded-memfns.cpp
===
--- /dev/null
+++ clang/test/Sema/builtin-overloaded-memfns.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -triple=arm64-unknown-unknown -fms-extensions
+
+#define NULL (void *)0
+#define nullptr __nullptr
+using size_t = __SIZE_TYPE__;
+struct Intish {
+  int i;
+};
+struct TrivialCpy {
+  char buf[8];
+  TrivialCpy();
+  TrivialCpy(const TrivialCpy &) = default;
+};
+struct NotTrivialCpy {
+  char buf[8];
+  NotTrivialCpy();
+  NotTrivialCpy(const NotTrivialCpy &);
+};
+
+void memcpy_arg_count() {
+  __builtin_overloaded_memcpy();   // expected-error {{too few arguments to function call, expected 3, have 0}}
+  __builtin_overloaded_memcpy(0);  // expected-error {{too few arguments to function call, expected 3, have 1}}
+  __builtin_overloaded_memcpy(0, 0);   // expected-error {{too few arguments to function call, expected 3, have 2}}
+  __builtin_overloaded_memcpy(0, 0, 0, 0); // expected-error {{too many arguments to function call, expected 3, have 4}}
+}
+
+void memcpy_null(char *dst, const char *src, size_t size) {
+  __builtin_overloaded_memcpy(0, 0, 0);
+  __builtin_overloaded_memcpy(0, 0, size);
+  __builtin_overloaded_memcpy(dst, 0, 42);   // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __builtin_overloaded_memcpy(dst, 0, 42);   // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __builtin_overloaded_memcpy(dst, NULL, 42);// expected-warning {{null passed to a callee that requires a non-null argument}}
+  __builtin_overloaded_memcpy(dst, nullptr, 42); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __builtin_overloaded_memcpy(0, src, 42);   // expected-warning {{null passed to a