[PATCH] D155580: [trivial-auto-var-init] Do not emit initialization code for empty class

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

(sorry for sending twice, looks like email reply failed)

This is the same as padding, and is initialized on purpose. If it’s truly 
unused then the optimizer will eliminate it… unless it can’t prove whether the 
store is dead.

Does this show up in any meaningless performance case? If so, can we optimize 
it away?


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

https://reviews.llvm.org/D155580

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

[PATCH] D125142: [clang][auto-init] Remove -enable flag for "zero" mode

[JF Bastien via Phabricator via cfe-commits]

jfb added a subscriber: Florian.
jfb added a comment.

I think the most relevant post from @rsmith is: 
https://discourse.llvm.org/t/making-ftrivial-auto-var-init-zero-a-first-class-option/55143/40

He has a prototype: https://reviews.llvm.org/D79249
I assume he would like someone to pursue it further, it was a good faith 
attempt at not just demanding. I'd played with it and it needed a few fixes, 
but overall it was pretty complete. Does someone want to give it a go?

That prototype would still diverge from the GCC option, which I hear isn't 
desirable.
The discussions on standardizing this are perpetually stalled whenever they 
come up, so it's not an avenue that I ever expect to turn positive.

The fact remains that people have deployed zero init widely (you're likely 
running multiple zero-init codebases to read this), and they would not ever 
deploy zero-or-trap init. That's simply a fact.

Richard had said:

> If we want a separate flag to control whether / how much we use such a 
> trapping mode, I think that could be reasonable, subject to having a good 
> answer to the language dialect concern I expressed previously (eg, maybe 
> there’s never a guarantee that we won’t insert a trap, even though with the 
> flag on its lowest setting that is actually what happens).

If someone pursues Richard's patch above, then this would seem like a mutually 
agreeable resolution.

Separately, we'd discussed narrowing the performance gap between pattern and 
zero-init, @Florian and team had done a bunch of work 2+ years ago, but I don't 
know how "done" we can claim that work is since I haven't been tracking the 
work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125142

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

[PATCH] D115440: Provide __builtin_alloca*_uninitialized variants

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D115440#3183778 , @melver wrote:

> GCC devs say that initializing explicit alloca() is a bug, because they 
> aren't "automatic storage": 
> https://lkml.kernel.org/r/20211209201616.gu...@gate.crashing.org
> .. which is also the reason why GCC's behaviour differs here at the moment.
>
> I actually agree with that, and alloca auto-init behaviour needs a new -mllvm 
> param.

GCC developer Segher says:

> The space allocated by alloca is not an automatic variable, so of course it 
> is not affected by this compiler flag.  And it should not, this flag is 
> explicitly for *small fixed-size* stack variables (initialising others can be 
> much too expensive).
>
>> C. Introduce a new __builtin_alloca_uninitialized().
>
> That is completely backwards.  That is the normal behaviour of alloca 
> already.  Also you can get __builtin_alloca inserted by the compiler (for a 
> variable length array for example), and you typically do not want those 
> initialised either, for the same reasons.

Segher is simply wrong.

Let's consult libc's own documentation 
https://www.gnu.org/software/libc/manual/html_node/Variable-Size-Automatic.html

It states:

> Automatic Storage with Variable Size
> The function alloca supports a kind of half-dynamic allocation in which 
> blocks are allocated dynamically but freed automatically.

The manpage for `alloca(3)`, states:

> This temporary space is automatically freed when the function that called 
> alloca() returns to its caller".

Refer to C17 which says:

> Storage durations of objects
> An object has a storage duration that determines its lifetime. There are four 
> storage durations: static, thread, automatic, and allocated.

Clearly `alloca` is neither static, nor thread, nor allocated (see "memory 
management functions" too). It's automatic storage.

Further, Segher misunderstand the purpose of automatic initialization. The goal 
is explicitly to change implementation-defined behavior from "the compiler 
leaves stack and register slots with whatever value it happened to overlap with 
(potentially leading to info leaks or UaF)" to "the compiler always overwrites 
previous values that were in stack and register slots before they are reused 
(preventing a large number of info leaks and UaF)". Saying "the normal 
behaviour of alloca already" is correct, but it ignores the objective of the 
flag: to explicitly *change* that behavior. The flag opts-in to that 
implementation-defined behavior change. And it has a very specific purpose 
w.r.t. security. This has measurable results in terms of CVEs avoided.

Additionally the flag is not explicitly for small fixed-sized variables, that 
was stated very clearly when it was committed, and the documentation is 
unambiguous about this fact. If initialization is too expensive then developers 
needs to opt-out with mechanisms such as `__attribute__((uninitialized))`. The 
support for VLAs and `alloca` was done very much on purpose, and if developers 
turn on variable auto-init then they reasonably expect that all automatic 
variables are automatically initialized, including VLAs and `alloca`. The patch 
you propose here is a good idea, because there's currently no way to opt-out 
for `alloca`. We should adopt a solution such as this one, and synchronize with 
GCC so that developers can use the same code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115440

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

[PATCH] D108469: Improve handling of static assert messages.

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Can you test all the values in this? https://godbolt.org/z/h7n54fa5x




Comment at: clang/lib/Basic/Diagnostic.cpp:792
+static void pushEscapedString(StringRef Str, SmallVectorImpl &OutStr) {
+  OutStr.reserve(OutStr.size() + Str.size());
+  const unsigned char *Begin =

Can this addition overflow?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108469

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

[PATCH] D108469: Improve handling of static assert messages.

[JF Bastien via Phabricator via cfe-commits]

jfb added a subscriber: hwright.
jfb added a comment.

I worry that changing the general `static_assert` printing (adding a colon, and 
dropping the quotes) will get @hwright's law to drop on us. We can try and see 
if e.g. users of clang have automated checks for `static_assert` in their CI 
pipelines or something. I think your new format looks better, but Hyrum is 
finicky that way... What do others think?

Can you add tests for other special characters, to make sure they're all 
handled properly? Just copy/paste my twitter shitpost might be sufficient? I 
think I covered all the corner cases in it, gotta be thorough!




Comment at: clang/lib/Basic/Diagnostic.cpp:818
+  llvm::raw_string_ostream stream(number);
+  stream << "";
+  OutStr.append(number.begin(), number.end());

We don't have a better hex formatter? 😟
Not a big deal, but I'd hoped that ADT had something!



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16361
+  if (AssertMessage) {
+assert(isa(AssertMessage));
+if (StringLiteral *MsgStr = cast(AssertMessage)) {

`cast` should already handle this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108469

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

[PATCH] D104975: Implement P1949

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

It would be more user friendly to say which character is not allowed in the 
diagnostic.

Do we need to have a backwards compatible flag to preserve the old behavior, or 
do we believe that nobody will be affected by the change? We should make this 
choice explicitly and state it in the patch description.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104975

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

[PATCH] D100057: Remove warning "suggest braces" for aggregate initialization of an empty class with an aggregate base class.

[JF Bastien via Phabricator via cfe-commits]

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

A few nits, but this is good otherwise!




Comment at: clang/lib/Sema/SemaInit.cpp:1013
 
-  auto *ParentRD =
-  Entity.getParent()->getType()->castAs()->getDecl();
-  if (CXXRecordDecl *CXXRD = dyn_cast(ParentRD))
-if (CXXRD->getNumBases())
-  return false;
+  // Allows elide brace initialization for aggregates with empty base
+  if (Entity.getKind() == InitializedEntity::EK_Base) {

"Allow eliding brace initialization".

Also, period at the end of the sentence.




Comment at: clang/lib/Sema/SemaInit.cpp:1018
+if (CXXRecordDecl *CXXRD = dyn_cast(ParentRD)) {
+  if (CXXRD->getNumBases() == 1) {
+return ParentRD->field_begin() == ParentRD->field_end();

Multiple empty bases isn't a common thing, right? I don't think so, but would 
rather check.



Comment at: clang/lib/Sema/SemaInit.cpp:1025
 
-  auto FieldIt = ParentRD->field_begin();
-  assert(FieldIt != ParentRD->field_end() &&
- "no fields but have initializer for member?");
-  return ++FieldIt == ParentRD->field_end();
+  // Allows elide brace initilization for aggregates with one member
+  if (Entity.getKind() == InitializedEntity::EK_Member) {

Same here.



Comment at: clang/test/SemaCXX/aggregate-initialization.cpp:183
+  OnionBaseClass u = {1, 2, 3};
+  OnionBaseClass t = {{{1, 2, 3}}};
+

Haha I like the name! 🧅😭


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100057

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

[PATCH] D98995: [CGAtomic] Lift stronger requirements on cmpxch and add support for acquire failure mode

[JF Bastien via Phabricator via cfe-commits]

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

Maybe refer to http://wg21.link/p0418 directly in the commit message?


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

https://reviews.llvm.org/D98995

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

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Let's make sure that we follow the same semantics that GCC does, particularly 
w.r.t. union, bitfields, and padding at the end of an object (whether it's in 
an array or not).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87974

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

[PATCH] D75229: [clang-tidy] Add signal-in-multithreaded-program check

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Most of the tests as written should be failing right now, at least on macOS and 
Linux, because they likely should be identified as POSIX, right?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D75229

___
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] D87974: [Builtin] Add __builtin_zero_non_value_bits.

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1647
+QualType Ty) {
+  auto *I8Ptr = CGF.Builder.CreateBitCast(Ptr, CGF.Int8PtrTy);
+  auto *Zero = ConstantInt::get(CGF.Int8Ty, 0);

I'd like to hear @rsmith's thoughts on this approach, in particular w.r.t. 
aliasing concerns. I also wonder if below the GEPs should be inbounds, 
depending on how they're created.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1652
+auto Element = CGF.Builder.CreateGEP(I8Ptr, Index);
+CGF.Builder.CreateAlignedStore(Zero, Element, MaybeAlign());
+  };

You should use `alignmentAtOffset` here.



Comment at: clang/test/CodeGenCXX/builtin-zero-non-value-bits-codegen.cpp:16
+  Bar f;
+};
+

It would be helpful to have a comment with the final layout of the struct, 
including padding. Give each padding field a name, and reference them in the IR 
check below.



Comment at: clang/test/CodeGenCXX/builtin-zero-non-value-bits-codegen.cpp:26
+// CHECK: [[FOO_RAW_PTR:%.*]] = bitcast %struct.Foo* [[FOO_BASE]] to i8*
+// CHECK: [[PAD_1:%.*]] = getelementptr i8, i8* [[FOO_RAW_PTR]], i32 1
+// CHECK: store i8 0, i8* [[PAD_1]]

It would help read the tests if you had a comment on top of each store, for 
example here "padding byte X".



Comment at: clang/test/CodeGenCXX/builtin-zero-non-value-bits-codegen.cpp:46
+void test(Baz *baz) {
+  __builtin_zero_non_value_bits(baz);
+}

It would be useful to see a test for arrays with a type that contains tail 
padding.



Comment at: clang/test/CodeGenCXX/builtin-zero-non-value-bits.cpp:160
+
+int main() {
+  testAllForType<32, 16, char>(11, 22, 33, 44);

zoecarver wrote:
> jfb wrote:
> > Usually CodeGen tests will use lit to check the emitted IR matches 
> > expectations. I think that's what you want to do here.
> > 
> > Remember to test `volatile` qualified pointers, as well as address spaces 
> > too.
> What's a good place for me to put this end-to-end test?
I'm not sure, I don't usually add this type of test :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87974

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

[PATCH] D87974: Summary: [Builtin] Add __builtin_zero_non_value_bits.

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1682
+
+  size_t NumFeilds = std::distance(R->field_begin(), R->field_end());
+  auto CurrentField = R->field_begin();

Typo in "fields".



Comment at: clang/test/CodeGenCXX/builtin-zero-non-value-bits.cpp:160
+
+int main() {
+  testAllForType<32, 16, char>(11, 22, 33, 44);

Usually CodeGen tests will use lit to check the emitted IR matches 
expectations. I think that's what you want to do here.

Remember to test `volatile` qualified pointers, as well as address spaces too.



Comment at: clang/test/SemaCXX/builtin-zero-non-value-bits.cpp:11
+  __builtin_zero_non_value_bits(e); // This should not error.
+}

You should also check incomplete types, vector, variable width integers, 
`const` qualified.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87974

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

[PATCH] D87858: [hip] Add HIP scope atomic ops.

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D87858#2282150 , @yaxunl wrote:

> In D87858#2280429 , @jfb wrote:
>
>> Please provide documentation in this patch.
>
> opencl atomic builtins are documented as notes to `__c11_atomic builtins` 
> part of 
> https://clang.llvm.org/docs/LanguageExtensions.html#builtin-functions. These 
> new atomic builtins can be documented in a similar way after that.

I'd like to see the documentation added to this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87858

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

[PATCH] D87858: [hip] Add HIP scope atomic ops.

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Please provide documentation in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87858

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

[PATCH] D78938: Fixing all comparisons for C++20 compilation.

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

On C++20 mode rotting: it won't if someone sets up a bot. If it rots, then it's 
easier to un-rot with Barry's patch.




Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:805-817
+  return IsASCII ? "^" : (const char *)u8"\u2548";
 case LineChar::RangeMid:
-  return IsASCII ? "|" : u8"\u2503";
+  return IsASCII ? "|" : (const char *)u8"\u2503";
 case LineChar::RangeEnd:
-  return IsASCII ? "v" : u8"\u253b";
+  return IsASCII ? "v" : (const char *)u8"\u253b";
 case LineChar::LabelVert:
+  return IsASCII ? "|" : (const char *)u8"\u2502";

jhenderson wrote:
> BRevzin wrote:
> > jhenderson wrote:
> > > This seems unrelated to comparison checking?
> > > This seems unrelated to comparison checking?
> > 
> > It is unrelated. But In C++20, `u8` literals become their own type so this 
> > no longer compiled and I wanted to ensure that I could actually run the 
> > tests. 
> Could it be a pre-requisite patch then?
I'm fine with this if the patch title is changed to "make LLVM build in C++20 
mode", and description edited accordingly. Basically, it makes it easy to 
figure out which changes were done for C++20.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78938

___
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] D33825: [clang-tidy] signal handler must be plain old function check

[JF Bastien via Phabricator via cfe-commits]

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

MSC54-CPP refers to POF, which as I pointed out above isn't relevant anymore. 
I'd much rather have a diagnostic which honors the state of things after 
http://wg21.link/p0270.

Additionally, lots of what MSC54-CPP intends is implementation-defined. We 
shouldn't diagnose for things that clang has defined as signal safe, at least 
not by default.

From the paper, I'd like to see tests for these:

- a call to any standard library function, except for plain lock-free atomic 
atomic operations and functions explicitly identified as signal-safe. [Note: 
This implicitly excludes the use of new and delete expressions that rely on a 
library-provided memory allocator. – end note]; -- here I'd like to explicitly 
see the list of signal-safe functions, and examples of ones that are not
- an access to an object with thread storage duration;
- a dynamic_cast expression;
- throwing of an exception;
- control entering a try-block or function-try-block; or
- initialization of a variable with static storage duration requiring dynamic 
initialization (3.6.3 [basic.start.dynamic], 6.7 [stmt.decl])). [ Note: Such 
initialization might occur because it is the first odr-use (3.2 
[basic.def.odr]) of that variable. -- end note ]
- waiting for the completion of the initialization of a variable with static 
storage duration (6.7 [stmt.decl]).




Comment at: 
clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:84
+  else if (llvm::isa(stmt))
+return "Construction of an unresolved type here";
+  else

Most of the above are fine per p0270, and most don't have a test at the moment. 



Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-msc54-cpp.rst:15
+static void sig_handler(int sig) {}
+// warning: use 'external C' prefix for signal handlers
+

'extern', not 'external'



Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-msc54-cpp.rst:23
+extern "C" void cpp_signal_handler(int sig) {
+  // warning: do not use C++ constructs in signal handlers
+  throw "error message";

Update the example too.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:74
+  // CHECK-MESSAGES: :[[@LINE+2]]:3: note: function called here
+  // CHECK-MESSAGES: TestClass::static_function();
+  TestClass::static_function();

I don't think this should diagnose, it is signal safe for clang's 
implementation, and allowed by p0270.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:91
+  auto char c = '1';
+  extern _Thread_local double _Complex d;
+  static const unsigned long int i = sizeof(float);

_Thread_local isn't signal safe per p0270.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:114
+  do {
+_Atomic int v = _Alignof(char);
+_Static_assert(42 == 42, "True");

The atomic isn't used here, and atomic initialization isn't atomic, so the test 
isn't sufficient.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D33825

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

[PATCH] D33825: [clang-tidy] signal handler must be plain old function check

[JF Bastien via Phabricator via cfe-commits]

jfb requested changes to this revision.
jfb added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: dexonsmith.

Please consider these changes, and whether this is relevant as implemented: 
http://wg21.link/p0270




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:22
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not use C++ constructs in 
signal handlers [cert-msc54-cpp]
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: note: C++ construct used here
+  throw "error message";

"C++ construct" isn't particularly useful. Here it needs to call out throwing 
exceptions.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:57
+  // CHECK-MESSAGES: :[[@LINE-11]]:3: note: function called here
+  // CHECK-MESSAGES: :[[@LINE-23]]:3: note: C++ construct used here
+  recursive_function();

Here too, I have no idea what this means given just this message.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:73
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not call functions with C++ 
constructs in signal handlers [cert-msc54-cpp]
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: note: function called here
+  TestClass::static_function();

This is also very confusing.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D33825

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

[PATCH] D86000: Add an unsigned shift base sanitizer

[JF Bastien via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG82d29b397bb2: Add an unsigned shift base sanitizer (authored 
by jfb).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86000

Files:
  clang/docs/UndefinedBehaviorSanitizer.rst
  clang/include/clang/Basic/Sanitizers.def
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/test/CodeGen/unsigned-shift-base.c
  clang/test/Driver/fsanitize.c
  compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
  llvm/docs/ReleaseNotes.rst

Index: llvm/docs/ReleaseNotes.rst
===
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -157,6 +157,14 @@
 Changes to LLDB
 -
 
+Changes to Sanitizers
+-
+
+The integer sanitizer `-fsanitize=integer` now has a new sanitizer:
+`-fsanitize=unsigned-shift-base`. It's not undefined behavior for an unsigned
+left shift to overflow (i.e. to shift bits out), but it has been the source of
+bugs and exploits in certain codebases in the past.
+
 External Open Source Projects Using LLVM 12
 ===
 
Index: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
@@ -0,0 +1,54 @@
+// RUN: %clangxx -fsanitize=unsigned-shift-base %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+// RUN: %clangxx -fsanitize=unsigned-shift-base,shift-exponent %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+
+#define shift(val, amount) ({  \
+  volatile unsigned _v = (val);\
+  volatile unsigned _a = (amount); \
+  unsigned res = _v << _a; \
+  res; \
+})
+
+int main() {
+
+  shift(0b''', 31);
+  shift(0b'''0001, 31);
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0001, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 64 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 128 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 256 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 512 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 1024 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2048 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4096 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8192 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16384 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32768 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'0001'', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 65536 by 31 places cannot be repres

[PATCH] D86000: Add an unsigned shift base sanitizer

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp:6
+  volatile unsigned _v = (val);\
+  volatile unsigned _a = (amount); \
+  unsigned res = _v << _a; \

vsk wrote:
> jfb wrote:
> > vsk wrote:
> > > Does the test not work without the volatiles?
> > It seems that LLVM sees too much without volatile, yes.
> The optimizer isn't running. Perhaps this is necessary because clang's 
> constant folder kicks in?
Probably, I haven't investigate. It's brittle is the main thing, and this 
forces it to not be.



Comment at: llvm/docs/ReleaseNotes.rst:151
+
+One can silence the check with masking, for example:
+

vsk wrote:
> This could use some more explanation, maybe s/with masking/by clearing bits 
> that would be shifted out/?
I just dropped it, I don't think release notes are the right place for this 
anyways.



Comment at: llvm/docs/ReleaseNotes.rst:153
+
+  `(lhs & ~(~1U << ((sizeof(lhs)*CHAR_BIT) - rhs))) << rhs`
+

lebedev.ri wrote:
> vsk wrote:
> > jfb wrote:
> > > lebedev.ri wrote:
> > > > Surely not `~1U`.
> > > Indeed, fixed.
> > I don't think this pattern works when rhs = 0 
> > (https://godbolt.org/z/rvEGqh).
> Surely not `1U` either :)
Right, I don't think it's something we want to explain here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86000

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

[PATCH] D86000: Add an unsigned shift base sanitizer

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 288444.
jfb marked 4 inline comments as done.
jfb added a comment.

Remove the "suppress this" in release notes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86000

Files:
  clang/docs/UndefinedBehaviorSanitizer.rst
  clang/include/clang/Basic/Sanitizers.def
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/test/CodeGen/unsigned-shift-base.c
  clang/test/Driver/fsanitize.c
  compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
  llvm/docs/ReleaseNotes.rst

Index: llvm/docs/ReleaseNotes.rst
===
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -140,6 +140,14 @@
 Changes to LLDB
 -
 
+Changes to Sanitizers
+-
+
+The integer sanitizer `-fsanitize=integer` now has a new sanitizer:
+`-fsanitize=unsigned-shift-base`. It's not undefined behavior for an unsigned
+left shift to overflow (i.e. to shift bits out), but it has been the source of
+bugs and exploits in certain codebases in the past.
+
 External Open Source Projects Using LLVM 12
 ===
 
Index: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
@@ -0,0 +1,54 @@
+// RUN: %clangxx -fsanitize=unsigned-shift-base %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+// RUN: %clangxx -fsanitize=unsigned-shift-base,shift-exponent %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+
+#define shift(val, amount) ({  \
+  volatile unsigned _v = (val);\
+  volatile unsigned _a = (amount); \
+  unsigned res = _v << _a; \
+  res; \
+})
+
+int main() {
+
+  shift(0b''', 31);
+  shift(0b'''0001, 31);
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0001, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 64 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 128 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 256 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 512 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 1024 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2048 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4096 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8192 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16384 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32768 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'0001'', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 65536 by 31 places cannot be represented in type 'u

[PATCH] D86000: Add an unsigned shift base sanitizer

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: llvm/docs/ReleaseNotes.rst:153
+
+  `(lhs & ~(~1U << ((sizeof(lhs)*CHAR_BIT) - rhs))) << rhs`
+

lebedev.ri wrote:
> Surely not `~1U`.
Indeed, fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86000

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

[PATCH] D86000: Add an unsigned shift base sanitizer

[JF Bastien via Phabricator via cfe-commits]

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

Fix notes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86000

Files:
  clang/docs/UndefinedBehaviorSanitizer.rst
  clang/include/clang/Basic/Sanitizers.def
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/test/CodeGen/unsigned-shift-base.c
  clang/test/Driver/fsanitize.c
  compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
  llvm/docs/ReleaseNotes.rst

Index: llvm/docs/ReleaseNotes.rst
===
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -140,6 +140,18 @@
 Changes to LLDB
 -
 
+Changes to Sanitizers
+-
+
+The integer sanitizer `-fsanitize=integer` now has a new sanitizer:
+`-fsanitize=unsigned-shift-base`. It's not undefined behavior for an unsigned
+left shift to overflow (i.e. to shift bits out), but it has been the source of
+bugs and exploits in certain codebases in the past.
+
+One can silence the check with masking, for example:
+
+  `(lhs & ~(1U << ((sizeof(lhs)*CHAR_BIT) - rhs))) << rhs`
+
 External Open Source Projects Using LLVM 12
 ===
 
Index: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
@@ -0,0 +1,54 @@
+// RUN: %clangxx -fsanitize=unsigned-shift-base %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+// RUN: %clangxx -fsanitize=unsigned-shift-base,shift-exponent %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+
+#define shift(val, amount) ({  \
+  volatile unsigned _v = (val);\
+  volatile unsigned _a = (amount); \
+  unsigned res = _v << _a; \
+  res; \
+})
+
+int main() {
+
+  shift(0b''', 31);
+  shift(0b'''0001, 31);
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0001, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 64 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 128 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 256 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 512 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 1024 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2048 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4096 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8192 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16384 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32768 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'0001'', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]

[PATCH] D86000: Add an unsigned shift base sanitizer

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3872-3884
   llvm::Value *BitsShiftedOff = Builder.CreateLShr(
   Ops.LHS, Builder.CreateSub(PromotedWidthMinusOne, RHS, "shl.zeros",
  /*NUW*/ true, /*NSW*/ true),
   "shl.check");
-  if (CGF.getLangOpts().CPlusPlus) {
+  if (SanitizeUnsignedBase || CGF.getLangOpts().CPlusPlus) {
 // In C99, we are not permitted to shift a 1 bit into the sign bit.
 // Under C++11's rules, shifting a 1 bit into the sign bit is

lebedev.ri wrote:
> Why is this so complicated? Shouldn't this just be: 
> https://alive2.llvm.org/ce/z/scTqfX
> ```
> $ /repositories/alive2/build-Clang-release/alive-tv /tmp/test.ll 
> --smt-to=10 --disable-undef-input
> 
> 
> @2 = global 32 bytes, align 16
> 
> define i32 @src(i32 %arg, i32 %arg1) {
> %bb:
>   %i = icmp ugt i32 %arg1, 31
>   %i2 = sub nsw nuw i32 31, %arg1; NOPE
>   %i3 = lshr i32 %arg, %i2   ; NOPE
>   %i4 = icmp ult i32 %i3, 2  ; NOPE
>   %i5 = or i1 %i, %i4
>   br i1 %i5, label %bb9, label %bb6
> 
> %bb6:
>   %i7 = zext i32 %arg to i64
>   %i8 = zext i32 %arg1 to i64
>   %__constexpr_0 = bitcast * @2 to *
>   call void @__ubsan_handle_shift_out_of_bounds(* %__constexpr_0, i64 %i7, 
> i64 %i8)
>   br label %bb9
> 
> %bb9:
>   %i10 = shl i32 %arg, %arg1
>   ret i32 %i10
> }
> =>
> @2 = global 32 bytes, align 16
> 
> define i32 @tgt(i32 %arg, i32 %arg1) {
> %bb:
>   %i = icmp ugt i32 %arg1, 31
>   %iZZ0 = shl i32 %arg, %arg1; HI!
>   %iZZ1 = lshr i32 %iZZ0, %arg1  ; OVER HERE
>   %i4 = icmp eq i32 %arg, %iZZ1  ; LOOK!
>   %i5 = or i1 %i, %i4
>   br i1 %i5, label %bb9, label %bb6
> 
> %bb6:
>   %i7 = zext i32 %arg to i64
>   %i8 = zext i32 %arg1 to i64
>   %__constexpr_0 = bitcast * @2 to *
>   call void @__ubsan_handle_shift_out_of_bounds(* %__constexpr_0, i64 %i7, 
> i64 %i8)
>   br label %bb9
> 
> %bb9:
>   ret i32 %iZZ0
> }
> Transformation seems to be correct!
> 
> ```
> which will then be migrated to use `@llvm.ushl.with.overflow` once it's there.
Sure, but that's pre-existing and I'd rather not change it in this patch.



Comment at: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp:2
+// RUN: %clangxx -fsanitize=unsigned-shift-base %s -o %t1 && not %run %t1 2>&1 
| FileCheck %s
+// RUN: %clangxx -fsanitize=unsigned-shift-base,shift-exponent %s -o %t1 && 
not %run %t1 2>&1 | FileCheck %s
+

vsk wrote:
> jfb wrote:
> > I don't understand this test... when I run it with lit it fails (and it 
> > seems like the bots agree), but manually it works. Am I doing it wrong?
> Do you need to explicitly `return 1` or something to get a non-zero exit code?
That should be implicit, but I added it regardless to make sure. It's happy now 
🤷‍♂️



Comment at: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp:6
+  volatile unsigned _v = (val);\
+  volatile unsigned _a = (amount); \
+  unsigned res = _v << _a; \

vsk wrote:
> Does the test not work without the volatiles?
It seems that LLVM sees too much without volatile, yes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86000

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

[PATCH] D86000: Add an unsigned shift base sanitizer

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 288383.
jfb marked 6 inline comments as done.
jfb added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86000

Files:
  clang/docs/UndefinedBehaviorSanitizer.rst
  clang/include/clang/Basic/Sanitizers.def
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/test/CodeGen/unsigned-shift-base.c
  clang/test/Driver/fsanitize.c
  compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
  llvm/docs/ReleaseNotes.rst

Index: llvm/docs/ReleaseNotes.rst
===
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -140,6 +140,18 @@
 Changes to LLDB
 -
 
+Changes to Sanitizers
+-
+
+The integer sanitizer `-fsanitize=integer` now has a new sanitizer:
+`-fsanitize=unsigned-shift-base`. It's not undefined behavior for an unsigned
+left shift to overflow (i.e. to shift bits out), but it has been the source of
+bugs and exploits in certain codebases in the past.
+
+One can silence the check with masking, for example:
+
+  `(lhs & ~(~1U << ((sizeof(lhs)*CHAR_BIT) - rhs))) << rhs`
+
 External Open Source Projects Using LLVM 12
 ===
 
Index: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
@@ -0,0 +1,54 @@
+// RUN: %clangxx -fsanitize=unsigned-shift-base %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+// RUN: %clangxx -fsanitize=unsigned-shift-base,shift-exponent %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+
+#define shift(val, amount) ({  \
+  volatile unsigned _v = (val);\
+  volatile unsigned _a = (amount); \
+  unsigned res = _v << _a; \
+  res; \
+})
+
+int main() {
+
+  shift(0b''', 31);
+  shift(0b'''0001, 31);
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0001, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 64 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 128 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 256 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 512 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 1024 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2048 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4096 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8192 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16384 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32768 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b00

[PATCH] D86000: Add an unsigned shift base sanitizer

[JF Bastien via Phabricator via cfe-commits]

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

Re-upload with full context.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86000

Files:
  clang/docs/UndefinedBehaviorSanitizer.rst
  clang/include/clang/Basic/Sanitizers.def
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/test/CodeGen/unsigned-shift-base.c
  clang/test/Driver/fsanitize.c
  compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp

Index: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
@@ -0,0 +1,52 @@
+// RUN: %clangxx -fsanitize=unsigned-shift-base %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+// RUN: %clangxx -fsanitize=unsigned-shift-base,shift-exponent %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+
+#define shift(val, amount) ({  \
+  volatile unsigned _v = (val);\
+  volatile unsigned _a = (amount); \
+  unsigned res = _v << _a; \
+  res; \
+})
+
+int main() {
+
+  shift(0b''', 31);
+  shift(0b'''0001, 31);
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0001, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 64 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 128 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 256 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 512 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 1024 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2048 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4096 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8192 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16384 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32768 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'0001'', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 65536 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'0010'', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 131072 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'0100'', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 262144 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'1000'', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 524288 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'0001'', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 1048576 by 31 places cannot be represente

[PATCH] D86000: Add an unsigned shift base sanitizer

[JF Bastien via Phabricator via cfe-commits]

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

As Vedant suggested, make it part of 'integer' sanitizer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86000

Files:
  clang/include/clang/Basic/Sanitizers.def
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/Driver/fsanitize.c

Index: clang/test/Driver/fsanitize.c
===
--- clang/test/Driver/fsanitize.c
+++ clang/test/Driver/fsanitize.c
@@ -32,7 +32,7 @@
 // CHECK-COVERAGE-WIN64: "--dependent-lib={{[^"]*}}ubsan_standalone-x86_64.lib"
 
 // RUN: %clang -target %itanium_abi_triple -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
-// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change),?){8}"}}
+// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change|unsigned-shift-base),?){9}"}}
 
 // RUN: %clang -fsanitize=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
 // RUN: %clang -fsanitize=implicit-conversion -fsanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
@@ -899,18 +899,3 @@
 
 // RUN: %clang -fsanitize=undefined,float-divide-by-zero %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-DIVBYZERO-UBSAN
 // CHECK-DIVBYZERO-UBSAN: "-fsanitize={{.*}},float-divide-by-zero,{{.*}}"
-
-// RUN: %clang -fsanitize=unsigned-shift-base %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-unsigned-shift-base,CHECK-unsigned-shift-base-RECOVER
-// RUN: %clang -fsanitize=unsigned-shift-base -fsanitize-recover=unsigned-shift-base %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-unsigned-shift-base,CHECK-unsigned-shift-base-RECOVER
-// RUN: %clang -fsanitize=unsigned-shift-base -fno-sanitize-recover=unsigned-shift-base %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-unsigned-shift-base,CHECK-unsigned-shift-base-NORECOVER
-// RUN: %clang -fsanitize=unsigned-shift-base -fsanitize-trap=unsigned-shift-base %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-unsigned-shift-base,CHECK-unsigned-shift-base-TRAP
-// CHECK-unsigned-shift-base: "-fsanitize=unsigned-shift-base"
-// CHECK-unsigned-shift-base-RECOVER: "-fsanitize-recover=unsigned-shift-base"
-// CHECK-unsigned-shift-base-RECOVER-NOT: "-fno-sanitize-recover=unsigned-shift-base"
-// CHECK-unsigned-shift-base-RECOVER-NOT: "-fsanitize-trap=unsigned-shift-base"
-// CHECK-unsigned-shift-base-NORECOVER-NOT: "-fno-sanitize-recover=unsigned-shift-base"
-// CHECK-unsigned-shift-base-NORECOVER-NOT: "-fsanitize-recover=unsigned-shift-base"
-// CHECK-unsigned-shift-base-NORECOVER-NOT: "-fsanitize-trap=unsigned-shift-base"
-// CHECK-unsigned-shift-base-TRAP: "-fsanitize-trap=unsigned-shift-base"
-// CHECK-unsigned-shift-base-TRAP-NOT: "-fsanitize-recover=unsigned-shift-base"
-// CHECK-unsigned-shift-base-TRAP-NOT: "-fno-sanitize-recover=unsigned-shift-base"
Index: clang/lib/Driver/SanitizerArgs.cpp
===
--- clang/lib/Driver/SanitizerArgs.cpp
+++ clang/lib/Driver/SanitizerArgs.cpp
@@ -26,9 +26,9 @@
 
 static const SanitizerMask NeedsUbsanRt =
 SanitizerKind::Undefined | SanitizerKind::Integer |
-SanitizerKind::UnsignedShiftBase | SanitizerKind::ImplicitConversion |
-SanitizerKind::Nullability | SanitizerKind::CFI |
-SanitizerKind::FloatDivideByZero | SanitizerKind::ObjCCast;
+SanitizerKind::ImplicitConversion | SanitizerKind::Nullability |
+SanitizerKind::CFI | SanitizerKind::FloatDivideByZero |
+SanitizerKind::ObjCCast;
 static const SanitizerMask NeedsUbsanCxxRt =
 SanitizerKind::Vptr | SanitizerKind::CFI;
 static const SanitizerMask NotAllowedWithTrap = SanitizerKind::Vptr;
@@ -44,8 +44,7 @@
 SanitizerKind::KernelAddress | SanitizerKind::KernelHWAddress |
 SanitizerKind::MemTag | SanitizerKind::Memory |
 SanitizerKind::KernelMemory | SanitizerKind::Leak |
-SanitizerKind::Undefined | SanitizerKind::Integer |
-SanitizerKind::UnsignedShiftBase | SanitizerKind::Bounds |
+SanitizerKind::Undefined | SanitizerKind::Integer | SanitizerKind::Bounds |
 SanitizerKind::ImplicitConversion | SanitizerKind::Nullability |
 SanitizerKind::DataFlow | SanitizerKind::Fuzzer |
 SanitizerKind::FuzzerNoLink | SanitizerKind::FloatDivideByZero |
@@ -53,9 +52,8 @@
 SanitizerKind::Thread | SanitizerKind::ObjCCast;
 static const SanitizerMask RecoverableByDefault =
 Sanit

[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 *ds

[PATCH] D77219: UBSan ␇ runtime

[JF Bastien via Phabricator via cfe-commits]

jfb abandoned this revision.
jfb added a comment.
Herald added a subscriber: dang.

I don't think the world is ready for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77219

___
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, 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] D86000: Add an unsigned shift base sanitizer

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D86000#2219288 , @vsk wrote:

> It'd be nice to fold the new check into an existing sanitizer group to bring 
> this to a wider audience. Do you foresee adoption issues for existing 
> -fsanitize=integer adopters? Fwiw some recently-added implicit conversion 
> checks were folded in without much/any pushback.

`integer` does "not actually UB checks", right? I can certainly put it in there 
if you think I won't get yelled at 😄




Comment at: clang/test/Driver/fsanitize.c:911
+// CHECK-unsigned-shift-base-RECOVER-NOT: "-fsanitize-trap=unsigned-shift-base"
+// CHECK-unsigned-shift-base-NORECOVER-NOT: 
"-fno-sanitize-recover=unsigned-shift-base"
+// CHECK-unsigned-shift-base-NORECOVER-NOT: 
"-fsanitize-recover=unsigned-shift-base"

vsk wrote:
> Not sure I follow this one. Why is 'NORECOVER' not expecting to see 
> "-fno-sanitize-recover=unsigned-shift-base"?
I have no idea! Other parts of this file do this:
```
// CHECK-implicit-conversion-NORECOVER-NOT: 
"-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change),?){3}"}}
 // ???
// CHECK-implicit-integer-arithmetic-value-change-NORECOVER-NOT: 
"-fno-sanitize-recover={{((implicit-signed-integer-truncation|implicit-integer-sign-change),?){2}"}}
 // ???
// CHECK-implicit-integer-truncation-NORECOVER-NOT: 
"-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
 // ???
```
I was hoping someone who's touched this before would know.



Comment at: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp:2
+// RUN: %clangxx -fsanitize=unsigned-shift-base %s -o %t1 && not %run %t1 2>&1 
| FileCheck %s
+// RUN: %clangxx -fsanitize=unsigned-shift-base,shift-exponent %s -o %t1 && 
not %run %t1 2>&1 | FileCheck %s
+

I don't understand this test... when I run it with lit it fails (and it seems 
like the bots agree), but manually it works. Am I doing it wrong?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86000

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

[PATCH] D86000: Add an unsigned shift base sanitizer

[JF Bastien via Phabricator via cfe-commits]

jfb created this revision.
jfb added a reviewer: vsk.
Herald added subscribers: Sanitizers, cfe-commits, ributzka, dexonsmith, 
jkorous.
Herald added projects: clang, Sanitizers.
jfb requested review of this revision.

It's not undefined behavior for an unsigned left shift to overflow (i.e. to
shift bits out), but it has been the source of bugs and exploits in certain
codebases in the past. As we do in other parts of UBSan, this patch adds a
dynamic checker which acts beyond UBSan and checks other sources of errors. The
option is enabled completely separately from other checkers since it's unlikely
that folks who have currently adopted other checkers will want this one.

The flag is named: -fsanitize=unsigned-shift-base
This matches shift-base and shift-exponent flags.

rdar://problem/46129047


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86000

Files:
  clang/docs/UndefinedBehaviorSanitizer.rst
  clang/include/clang/Basic/Sanitizers.def
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Driver/SanitizerArgs.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/test/CodeGen/unsigned-shift-base.c
  clang/test/Driver/fsanitize.c
  compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp

Index: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
@@ -0,0 +1,52 @@
+// RUN: %clangxx -fsanitize=unsigned-shift-base %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+// RUN: %clangxx -fsanitize=unsigned-shift-base,shift-exponent %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+
+#define shift(val, amount) ({  \
+  volatile unsigned _v = (val);\
+  volatile unsigned _a = (amount); \
+  unsigned res = _v << _a; \
+  res; \
+})
+
+int main() {
+
+  shift(0b''', 31);
+  shift(0b'''0001, 31);
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0001, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 64 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 128 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 256 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 512 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 1024 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2048 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4096 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8192 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16384 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32768 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'0001'', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 65536 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'0010'', 3

[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, sr

[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: builtins-mem_sized.cpp:[[@LINE+

[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

[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
+  __builtin_memset_overloaded(dst_misal

[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
+  __builtin_memset_overloaded(dst_misaligned

[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, 2)

[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 check_memcpy_size(char

[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 &CGM, const CallExpr *E,
> >   unsigned ArgNo) {
> >   QualType ArgTy = E->getArg(ArgNo)->IgnoreImpCasts()->getType();
> >   if (ArgTy->isArrayType())
> > return CGM.getContext().getAsArrayType(ArgTy)->getElementType();
> >   if (ArgTy->isObjCObjectPointerType())
> > return ArgTy->castAs()->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 

[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 check_mem

[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 &CGM, const CallExpr *E,
  unsigned ArgNo) {
  QualType ArgTy = E->getArg(ArgNo)->IgnoreImpCasts()->getType();
  if (ArgTy->isArrayType())
return CGM.getContext().getAsArrayType(ArgTy)->getElementType();
  if (ArgTy->isObjCObjectPointerType())
return ArgTy->castAs()->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 __builtin_

[PATCH] D85102: [clang] improve diagnostics for misaligned and large atomics

[JF Bastien via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe18c6ef6b41a: [clang] improve diagnostics for misaligned and 
large atomics (authored by tschuett, committed by jfb).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85102

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CGAtomic.cpp
  clang/test/CodeGen/atomics-sema-alignment.c

Index: clang/test/CodeGen/atomics-sema-alignment.c
===
--- clang/test/CodeGen/atomics-sema-alignment.c
+++ clang/test/CodeGen/atomics-sema-alignment.c
@@ -12,10 +12,10 @@
 
 void func(IntPair *p) {
   IntPair res;
-  __atomic_load(p, &res, 0); // expected-warning {{misaligned atomic operation may incur significant performance penalty}}
-  __atomic_store(p, &res, 0); // expected-warning {{misaligned atomic operation may incur significant performance penalty}}
-  __atomic_fetch_add((unaligned_int *)p, 1, 2); // expected-warning {{misaligned atomic operation may incur significant performance penalty}}
-  __atomic_fetch_sub((unaligned_int *)p, 1, 3); // expected-warning {{misaligned atomic operation may incur significant performance penalty}}
+  __atomic_load(p, &res, 0);// expected-warning {{misaligned atomic operation may incur significant performance penalty; the expected alignment (8 bytes) exceeds the actual alignment (4 bytes)}}
+  __atomic_store(p, &res, 0);   // expected-warning {{misaligned atomic operation may incur significant performance penalty; the expected alignment (8 bytes) exceeds the actual alignment (4 bytes)}}
+  __atomic_fetch_add((unaligned_int *)p, 1, 2); // expected-warning {{misaligned atomic operation may incur significant performance penalty; the expected alignment (4 bytes) exceeds the actual alignment (1 bytes)}}
+  __atomic_fetch_sub((unaligned_int *)p, 1, 3); // expected-warning {{misaligned atomic operation may incur significant performance penalty; the expected alignment (4 bytes) exceeds the actual alignment (1 bytes)}}
 }
 
 void func1(LongStruct *p) {
@@ -25,3 +25,24 @@
   __atomic_fetch_add((int *)p, 1, 2);
   __atomic_fetch_sub((int *)p, 1, 3);
 }
+
+typedef struct {
+  void *a;
+  void *b;
+} Foo;
+
+typedef struct {
+  void *a;
+  void *b;
+  void *c;
+  void *d;
+} __attribute__((aligned(32))) ThirtyTwo;
+
+void braz(Foo *foo, ThirtyTwo *braz) {
+  Foo bar;
+  __atomic_load(foo, &bar, __ATOMIC_RELAXED); // expected-warning {{misaligned atomic operation may incur significant performance penalty; the expected alignment (16 bytes) exceeds the actual alignment (8 bytes)}}
+
+  ThirtyTwo thirtyTwo1;
+  ThirtyTwo thirtyTwo2;
+  __atomic_load(&thirtyTwo1, &thirtyTwo2, __ATOMIC_RELAXED); // expected-warning {{large atomic operation may incur significant performance penalty; the access size (32 bytes) exceeds the max lock-free size (16  bytes)}}
+}
Index: clang/lib/CodeGen/CGAtomic.cpp
===
--- clang/lib/CodeGen/CGAtomic.cpp
+++ clang/lib/CodeGen/CGAtomic.cpp
@@ -807,10 +807,20 @@
   bool Oversized = getContext().toBits(sizeChars) > MaxInlineWidthInBits;
   bool Misaligned = (Ptr.getAlignment() % sizeChars) != 0;
   bool UseLibcall = Misaligned | Oversized;
+  CharUnits MaxInlineWidth =
+  getContext().toCharUnitsFromBits(MaxInlineWidthInBits);
 
-  if (UseLibcall) {
-CGM.getDiags().Report(E->getBeginLoc(), diag::warn_atomic_op_misaligned)
-<< !Oversized;
+  DiagnosticsEngine &Diags = CGM.getDiags();
+
+  if (Misaligned) {
+Diags.Report(E->getBeginLoc(), diag::warn_atomic_op_misaligned)
+<< (int)sizeChars.getQuantity()
+<< (int)Ptr.getAlignment().getQuantity();
+  }
+
+  if (Oversized) {
+Diags.Report(E->getBeginLoc(), diag::warn_atomic_op_oversized)
+<< (int)sizeChars.getQuantity() << (int)MaxInlineWidth.getQuantity();
   }
 
   llvm::Value *Order = EmitScalarExpr(E->getOrder());
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -699,6 +699,7 @@
 def Reorder : DiagGroup<"reorder", [ReorderCtor, ReorderInitList]>;
 def UndeclaredSelector : DiagGroup<"undeclared-selector">;
 def ImplicitAtomic : DiagGroup<"implicit-atomic-properties">;
+def AtomicAlignment : DiagGroup<"atomic-alignment">;
 def CustomAtomic : DiagGroup<"custom-atomic-properties">;
 def AtomicProperties : DiagGroup<"atomic-properties",
  [ImplicitAtomic, CustomAtomic]>;
Index: clang/include/clang/Basic/DiagnosticFrontendKinds.td
===
--- clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ clang/include/clang/

[PATCH] D85102: [clang] improve diagnostics for misaligned and large atomics

[JF Bastien via Phabricator via cfe-commits]

jfb accepted this revision.
jfb added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/test/CodeGen/atomics-sema-alignment.c:43
+  Foo bar;
+  __atomic_load(foo, &bar, __ATOMIC_RELAXED); // expected-warning {{misaligned 
atomic operation may incur significant performance penalty; the expected (16 
bytes) exceeds the actual alignment (8 bytes)}}
+

"the expected (16 bytes)" should be "the expected alignment (16 bytes)"


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

https://reviews.llvm.org/D85102

___
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);  // expect

[PATCH] D85009: [Sema][BFloat] Forbid arithmetic on vectors of bfloat.

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D85009#2187631 , @LukeGeeson wrote:

> In D85009#2187621 , @jfb wrote:
>
>> In D85009#2187603 , @simon_tatham 
>> wrote:
>>
>>> In D85009#2187549 , @jfb wrote:
>>>
 Is that true of all vector bfloat implementations? It seems like 
 arithmetic on these types is something implementations would likely 
 support.
>>>
>>> As I understand it, Arm currently has the only implementation in clang so 
>>> far. But if other targets disagree, we can make this conditional on 
>>> `getVectorKind()`, so that `VectorType::NeonVector` gets this restriction 
>>> and other vector types get whatever they need.
>>
>> You mean: only aarch64 backend supports lowering bfloat16 vectors at the 
>> moment? Because the clang support isn't "ARM bfloat", it's just bfloat. The 
>> tests are ARM bfloat and I think that's fine (i.e. Sema should be able to 
>> check ISA-specific problems), but in general this property your checking for 
>> seems like a target property.
>>
>> If I write C or C++ code using bfloat, I'd like to know what that type 
>> actually means and what I can do with it. As a developer, it'll be super 
>> frustrating once other targets support bfloat... should those target have 
>> their own bfloat (because it won't be compatible with ARM's), or should 
>> bfloat work differently on different targets?
>>
>> I actually don't know what the intended approach is here, which is why I'm 
>> asking :)
>
> Yes there is an Intel bfloat type too, however we are the only target for the 
> bfloat c/ir type so far. The jury is also out as far as the standards are 
> concerned too, the best we can do now is prevent behavior we know is not 
> compatible, and like Simon says, add some predication later

Language-wise I think https://wg21.link/p1467 is where C++ is going, and C is 
taking a similar approach.

I'd like to make sure this is well thought out. Not just "the ISA does this, 
let's do the same". We know other ISAs act differently, and I'm not clear on 
what the intended behavior will be for people writing C and C++ code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85009

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

[PATCH] D85009: [Sema][BFloat] Forbid arithmetic on vectors of bfloat.

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D85009#2187603 , @simon_tatham 
wrote:

> In D85009#2187549 , @jfb wrote:
>
>> Is that true of all vector bfloat implementations? It seems like arithmetic 
>> on these types is something implementations would likely support.
>
> As I understand it, Arm currently has the only implementation in clang so 
> far. But if other targets disagree, we can make this conditional on 
> `getVectorKind()`, so that `VectorType::NeonVector` gets this restriction and 
> other vector types get whatever they need.

You mean: only aarch64 backend supports lowering bfloat16 vectors at the 
moment? Because the clang support isn't "ARM bfloat", it's just bfloat. The 
tests are ARM bfloat and I think that's fine (i.e. Sema should be able to check 
ISA-specific problems), but in general this property your checking for seems 
like a target property.

If I write C or C++ code using bfloat, I'd like to know what that type actually 
means and what I can do with it. As a developer, it'll be super frustrating 
once other targets support bfloat... should those target have their own bfloat 
(because it won't be compatible with ARM's), or should bfloat work differently 
on different targets?

I actually don't know what the intended approach is here, which is why I'm 
asking :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85009

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

[PATCH] D85009: [Sema][BFloat] Forbid arithmetic on vectors of bfloat.

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Is that true of all vector bfloat implementations? It seems like arithmetic on 
these types is something implementations would likely support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85009

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

[PATCH] D84666: [NFC] Sema: use checkArgCount instead of custom checking

[JF Bastien via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG389f009c5757: [NFC] Sema: use checkArgCount instead of 
custom checking (authored by jfb).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84666

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtins-ppc-error.c
  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/CodeGen/builtins-ppc-error.c
===
--- clang/test/CodeGen/builtins-ppc-error.c
+++ clang/test/CodeGen/builtins-ppc-error.c
@@ -32,16 +32,16 @@
 }
 
 void testXXPERMDI(int index) {
-  vec_xxpermdi(vsi); //expected-error {{too few arguments to function call, expected at least 3, have 1}}
-  vec_xxpermdi(vsi, vsi, 2, 4); //expected-error {{too many arguments to function call, expected at most 3, have 4}}
+  vec_xxpermdi(vsi); //expected-error {{too few arguments to function call, expected 3, have 1}}
+  vec_xxpermdi(vsi, vsi, 2, 4); //expected-error {{too many arguments to function call, expected 3, have 4}}
   vec_xxpermdi(vsi, vsi, index); //expected-error {{argument 3 to '__builtin_vsx_xxpermdi' must be a 2-bit unsigned literal (i.e. 0, 1, 2 or 3)}}
   vec_xxpermdi(1, 2, 3); //expected-error {{first two arguments to '__builtin_vsx_xxpermdi' must be vectors}}
   vec_xxpermdi(vsi, vuc, 2); //expected-error {{first two arguments to '__builtin_vsx_xxpermdi' must have the same type}}
 }
 
 void testXXSLDWI(int index) {
-  vec_xxsldwi(vsi); //expected-error {{too few arguments to function call, expected at least 3, have 1}}
-  vec_xxsldwi(vsi, vsi, 2, 4); //expected-error {{too many arguments to function call, expected at most 3, have 4}}
+  vec_xxsldwi(vsi); //expected-error {{too few arguments to function call, expected 3, have 1}}
+  vec_xxsldwi(vsi, vsi, 2, 4); //expected-error {{too many arguments to function call, expected 3, have 4}}
   vec_xxsldwi(vsi, vsi, index); //expected-error {{argument 3 to '__builtin_vsx_xxsldwi' must be a 2-bit unsigned literal (i.e. 0, 1, 2 or 3)}}
   vec_xxsldwi(1, 2, 3); //expected-error {{first two arguments to '__builtin_vsx_xxsldwi' must be vectors}}
   vec_xxsldwi(vsi, vuc, 2); //expected-error {{first two arguments to '__builtin_vsx_xxsldwi' must have the same type}}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -1274,11 +1274,8 @@
 // \return True if a semantic error has been found, false otherwise.
 static bool SemaOpenCLBuiltinToAddr(Sema &S, unsigned BuiltinID,
 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;
-  }
 
   auto RT = Call->getArg(0)->getType();
   if (!RT->isPointerType() || RT->getPointeeType()
@@ -5572,21 +5569,8 @@
   if (checkVAStartABI(*this, BuiltinID, Fn))
 return true;
 
-  if (TheCall->getNumArgs() > 2) {
-Diag(TheCall->getArg(2)->getBeginLoc(),
- diag::err_typecheck_call_too_many_args)
-<< 0 /*function call*/ << 2 << TheCall->getNumArgs()
-<< Fn->getSourceRange()
-<< SourceRange(TheCall->getArg(2)->getBeginLoc(),
-   (*(TheCall->arg_end() - 1))->getEndLoc());
+  if (checkArgCount(*this, TheCall, 2))
 return true;
-  }
-
-  if (TheCall->getNumArgs() < 2) {
-return Diag(TheCall->getEndLoc(),
-diag::err_typecheck_call_too_few_args_at_least)
-   << 0 /*function call*/ << 2 << TheCall->getNumArgs();
-  }
 
   // Type-check the first argument normally.
   if (checkBuiltinArgument(*this, TheCall, 0))
@@ -5696,15 +5680,8 @@
 /// SemaBuiltinUnorderedCompare - Handle functions like __builtin_isgreater and
 /// friends.  This is declared to take (...), so we have to check everything.
 bool Sema::SemaBuiltinUnorderedCompare(CallExpr *TheCall) {
-  if (TheCall->getNumArgs() < 2)
-return Diag(TheCall->getEndLoc(), diag::err_typecheck_call

[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: clang/lib/Sema/SemaCheck

[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 passed

[PATCH] D84666: [NFC] Sema: use checkArgCount instead of custom checking

[JF Bastien via Phabricator via cfe-commits]

jfb created this revision.
jfb added a reviewer: rsmith.
Herald added subscribers: cfe-commits, ributzka, dexonsmith, jkorous, kbarton, 
nemanjai.
Herald added a project: clang.

As requested in D79279 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84666

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtins-ppc-error.c
  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/CodeGen/builtins-ppc-error.c
===
--- clang/test/CodeGen/builtins-ppc-error.c
+++ clang/test/CodeGen/builtins-ppc-error.c
@@ -32,16 +32,16 @@
 }
 
 void testXXPERMDI(int index) {
-  vec_xxpermdi(vsi); //expected-error {{too few arguments to function call, expected at least 3, have 1}}
-  vec_xxpermdi(vsi, vsi, 2, 4); //expected-error {{too many arguments to function call, expected at most 3, have 4}}
+  vec_xxpermdi(vsi); //expected-error {{too few arguments to function call, expected 3, have 1}}
+  vec_xxpermdi(vsi, vsi, 2, 4); //expected-error {{too many arguments to function call, expected 3, have 4}}
   vec_xxpermdi(vsi, vsi, index); //expected-error {{argument 3 to '__builtin_vsx_xxpermdi' must be a 2-bit unsigned literal (i.e. 0, 1, 2 or 3)}}
   vec_xxpermdi(1, 2, 3); //expected-error {{first two arguments to '__builtin_vsx_xxpermdi' must be vectors}}
   vec_xxpermdi(vsi, vuc, 2); //expected-error {{first two arguments to '__builtin_vsx_xxpermdi' must have the same type}}
 }
 
 void testXXSLDWI(int index) {
-  vec_xxsldwi(vsi); //expected-error {{too few arguments to function call, expected at least 3, have 1}}
-  vec_xxsldwi(vsi, vsi, 2, 4); //expected-error {{too many arguments to function call, expected at most 3, have 4}}
+  vec_xxsldwi(vsi); //expected-error {{too few arguments to function call, expected 3, have 1}}
+  vec_xxsldwi(vsi, vsi, 2, 4); //expected-error {{too many arguments to function call, expected 3, have 4}}
   vec_xxsldwi(vsi, vsi, index); //expected-error {{argument 3 to '__builtin_vsx_xxsldwi' must be a 2-bit unsigned literal (i.e. 0, 1, 2 or 3)}}
   vec_xxsldwi(1, 2, 3); //expected-error {{first two arguments to '__builtin_vsx_xxsldwi' must be vectors}}
   vec_xxsldwi(vsi, vuc, 2); //expected-error {{first two arguments to '__builtin_vsx_xxsldwi' must have the same type}}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -1274,11 +1274,8 @@
 // \return True if a semantic error has been found, false otherwise.
 static bool SemaOpenCLBuiltinToAddr(Sema &S, unsigned BuiltinID,
 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;
-  }
 
   auto RT = Call->getArg(0)->getType();
   if (!RT->isPointerType() || RT->getPointeeType()
@@ -5572,21 +5569,8 @@
   if (checkVAStartABI(*this, BuiltinID, Fn))
 return true;
 
-  if (TheCall->getNumArgs() > 2) {
-Diag(TheCall->getArg(2)->getBeginLoc(),
- diag::err_typecheck_call_too_many_args)
-<< 0 /*function call*/ << 2 << TheCall->getNumArgs()
-<< Fn->getSourceRange()
-<< SourceRange(TheCall->getArg(2)->getBeginLoc(),
-   (*(TheCall->arg_end() - 1))->getEndLoc());
+  if (checkArgCount(*this, TheCall, 2))
 return true;
-  }
-
-  if (TheCall->getNumArgs() < 2) {
-return Diag(TheCall->getEndLoc(),
-diag::err_typecheck_call_too_few_args_at_least)
-   << 0 /*function call*/ << 2 << TheCall->getNumArgs();
-  }
 
   // Type-check the first argument normally.
   if (checkBuiltinArgument(*this, TheCall, 0))
@@ -5696,15 +5680,8 @@
 /// SemaBuiltinUnorderedCompare - Handle functions like __builtin_isgreater and
 /// friends.  This is declared to take (...), so we have to check everything.
 bool Sema::SemaBuiltinUnorderedCompare(CallExpr *TheCall) {
-  if (TheCall->getNumArgs() < 2)
-return Diag(TheCall->getEndLoc(), diag::err_typecheck_call_too_few_args)
-   << 0 << 2 << TheCall->getNumArgs() /*function call*/;
-  if (TheCall

[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

[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

[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

[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

[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, &tmp);
   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, &tmp);
   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

[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

[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, &tmp);
   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, &tmp);
   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

[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, &tmp);
   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, &tmp);
   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}}
+  __builtin_memset_o

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D71726#2165445 , @yaxunl wrote:

> In D71726#2165424 , @jyknight wrote:
>
> > Why not have clang always emit atomicrmw for floats, and let 
> > AtomicExpandPass handle legalizing that into integer atomics if necessary, 
> > rather than adding a target hook in clang?
>
>
> Not all targets can legalize fp atomics by AtomicExpandPass. Some targets 
> need library support.


What are they missing? It can be expanded to a cmpxchg loop with bitcast to an 
integer type of the same size.


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

https://reviews.llvm.org/D71726



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

[PATCH] D83509: CrashTracer: clang at clang: llvm::BitstreamWriter::ExitBlock

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

I don't remember if this will auto-close, since I forgot to add the Phabricator 
ID to the commit message. In any case it's in: 
00c9a504aeed2603bd8bc9b89d753534e929c8e8 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83509



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

[PATCH] D83509: CrashTracer: clang at clang: llvm::BitstreamWriter::ExitBlock

[JF Bastien via Phabricator via cfe-commits]

jfb accepted this revision.
jfb added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:123
+def warn_fe_serialized_diag_failure_during_finalisation : Warning<
+"Received warning after diagnostic serialisation teardown was underway: 
%0">,
+InGroup;

```
$ git grep -i "serializ"  | wc -l
6053
$ git grep -i "serialis"  | wc -l
   5
```
**Z** it is 😉


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83509



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

[PATCH] D75229: [clang-tidy] Add signal-in-multithreaded-program check

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

It seems like you don't want this check to trigger on POSIX platforms, given:

> Exceptions CON37-C-EX1: Implementations such as POSIX that provide defined 
> behavior when multithreaded programs use custom signal handlers are exempt 
> from this rule [IEEE Std 1003.1-2013].


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D75229



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

[PATCH] D77493: [clang-tidy] Add do-not-refer-atomic-twice check

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-do-not-refer-atomic-twice.cpp:10
+_Atomic int n2 = ATOMIC_VAR_INIT(0);
+_Atomic(int) n3 = ATOMIC_VAR_INIT(0);
+

Can you cover `std::atomic` as well?



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-do-not-refer-atomic-twice.cpp:10
+_Atomic int n2 = ATOMIC_VAR_INIT(0);
+_Atomic(int) n3 = ATOMIC_VAR_INIT(0);
+

jfb wrote:
> Can you cover `std::atomic` as well?
We deprecated `ATOMIC_VAR_INIT` in C++20 and C17, I wouldn't use it here (even 
if it's just your own macro).



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-do-not-refer-atomic-twice.cpp:77
+  n3++;
+}

Can you check that non-atomic-accesses to the variable also work, for example 
taking the atomic's address, using it in unevaluated `sizeof` / `offsetof`, etc.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D77493



___
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, &tmp);
   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, &tmp);
   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}}
+  __builtin_memset_over

[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 &Result, const ASTContext &Ctx,
 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

[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, &tmp);
   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, &tmp);
   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

[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, &tmp);
   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, &tmp);
   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 passe

[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, &tmp);
   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, &tmp);
   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

[PATCH] D82832: Correctly generate invert xor value for Binary Atomics of int size > 64

[JF Bastien via Phabricator via cfe-commits]

jfb accepted this revision.
jfb added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: dexonsmith.

This amused me.


Repository:
  rC Clang

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

https://reviews.llvm.org/D82832



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

[PATCH] D80055: Diagnose union tail padding

[JF Bastien via Phabricator via cfe-commits]

jfb abandoned this revision.
jfb added a comment.

I've gotten what I wanted out of this (diagnosed a particular codebase), and am 
not sure it's worth pursuing further. If anyone is interested, please let me 
know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80055



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

[PATCH] D79249: [NOT FOR REVIEW] Experimental support for zero-or-trap behavior foruninitialized variables.

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Overall I like this approach.

I think it needs three more things to make it:

- Better size optimizations. I measured the code size impact on a codebase 
which deploys variable auto-init already, and it's a 0.5% code size hit. 
Considering that auto-init itself was 1%, it means the mitigation now costs 50% 
more. I haven't dug into why the increase is such, and I assume that there's 
low-lying fruits.
- I haven't measure performance impact. It might be zero.
- I think we'd need opt remarks support, because we'd want to audit all the 
places where a trap is left behind.




Comment at: clang/lib/CodeGen/CGDecl.cpp:1166
+  &CGM.getModule(), llvm::Intrinsic::trapping, {V->getType()});
+  V = Builder.CreateCall(TrapFn, {V});
+}

Here you need something like:
```
  auto *PointerTy = Ty->getPointerTo(Loc.getAddressSpace());
  if (V->getType() != PointerTy)
Loc = Builder.CreateBitCast(Loc, PointerTy);
```
Because you can be storing a constant which is layout-compatible with the 
location, but which doesn't have the same type (say, when we have an explicit 
padding array of bytes). Line 1184 does that already for other code paths.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79249



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

[PATCH] D77168: Add a flag to debug automatic variable initialization

[JF Bastien via Phabricator via cfe-commits]

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

Two minor corrections, looks good otherwise.




Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:491
+def err_drv_trivial_auto_var_init_stop_after_invalid_value : Error<
+  "-ftrivial-auto-var-init-stop-after=* only accpets positive integers.">;
+

Typo "accepts".



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5973
+  DiagnosticsEngine::Warning,
+  "-ftrivial-auto-var-init-stop-after=* has been enabled to limit the "
+  "number of times ftrivial-auto-var-init=%0 gets applied.");

Could you print out the count here, instead of `*`?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77168



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

[PATCH] D77168: Add a flag to debug automatic variable initialization

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Can you add a test for the diagnostic firing after the correct number of 
initializations? This should include a few types of auto-init, including VLAs.




Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:489
+  "-ftrivial-auto-var-init-stop-after=* has been enabled to limit the number 
of times ftrivial-auto-var-init=zero/pattern gets applied.">,
+  InGroup>;
+

I don't think this is sufficiently clear: automatic variable initialization is 
now disabled because ftrivial-auto-var-init-stop-after has reached its limit.



Comment at: clang/lib/CodeGen/CGDecl.cpp:1684
+  if (CGM.stopAutoInit())
+return;
+  if (trivialAutoVarInit == LangOptions::TrivialAutoVarInitKind::Zero)

I'd rather repeat this than fallthrought and repeat the condition in if/else.



Comment at: clang/lib/CodeGen/CodeGenModule.h:1390
+  if (NumAutoVarInit >= StopAfter)
+return true;
+  ++NumAutoVarInit;

The first time this returns true is when the diagnostic should be emitted.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3092
+  Args.getLastArg(options::OPT_ftrivial_auto_var_init_stop_after)) {
+D.Diag(diag::warn_drv_trivial_auto_var_init_stop_after_enabled);
+A->claim();

Not here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77168



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

[PATCH] D80055: Diagnose union tail padding

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D80055#2042630 , @rsmith wrote:

> I'm not convinced that this is an especially useful diagnostic (much like 
> with `-Wpadded`), but I'm also not opposed if you are aware of cases where it 
> would be used.


I wrote it to help with issues a codebase was having adopting variable 
auto-init. They were surprised at what clang did, and wish they could have 
known. This is just one datapoint, and I agree that generally this isn't super 
useful.

> It seems worth pointing out that, at least in C, merely assigning to a union 
> member results in all padding becoming unspecified (6.2.6.1/7). So the 
> suggestion to `memset` the union to zero and then assign to a specific union 
> member does not work.

Indeed, this isn't something I'm trying to address here but it would be useful 
to do so.

I was wondering if any of the tests were surprising to you, or if the behavior 
described was as expected? It's certainly not tractable for most users of C and 
C++ (which might be fine, it shouldn't be relied on).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80055



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

[PATCH] D78938: Fixing all comparisons for C++20 compilation.

[JF Bastien via Phabricator via cfe-commits]

jfb accepted this revision.
jfb added a comment.

One suggestions, otherwise looks good. Thanks for doing this :)




Comment at: llvm/include/llvm/ADT/DirectedGraph.h:97
+  }
+  friend bool operator!=(const NodeType &M, const NodeType &N) { !(M == N); }
 

davidstone wrote:
> Missing `return`
😱

Did this not trigger a diagnostic when building? I wonder if it's just not on?



Comment at: llvm/include/llvm/ADT/DirectedGraph.h:40
   /// Static polymorphism: delegate implementation (via isEqualTo) to the
   /// derived class.
+  bool operator==(const DGEdge &E) const {

That comment, so informative! 😐



Comment at: llvm/unittests/ADT/STLExtrasTest.cpp:466
   std::unique_ptr V2 = std::make_unique(0);
-  EXPECT_EQ(V2.get(), to_address(V2));
+  EXPECT_EQ(V2.get(), (to_address)(V2));
 

Can you add a comment above (with "fancy pointer") so mere mortals understand 
the parens?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78938



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D71726#2047566 , @ldionne wrote:

> In D71726#1791904 , @jfb wrote:
>
> > This generally seems fine. Does it work on most backends? I want to make 
> > sure it doesn't fail in backends :)
> >
> > Also, @ldionne / @EricWF / @mclow.lists do you need this in libc++ for 
> > floating-point atomic support?
>
>
> Yes, I guess we do in order to implement `fetch_add` & friends on floating 
> point types (https://wg21.link/P0020R6).
>
> The builtins would need to work on `float`, `double` and `long double`. The 
> code seems to suggest it does, however the tests only check for `float`. Does 
> this support `__atomic_fetch_{add,sub}` on `double` and `long double`?


libc++ could implement `atomic` using a cmpxchg loop with `bit_cast` and 
the FP instruction in most cases, and only use these builtins if available.


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

https://reviews.llvm.org/D71726



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

[PATCH] D80055: Diagnose union tail padding

[JF Bastien via Phabricator via cfe-commits]

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



Comment at: clang/test/CodeGen/union-tail-padding.c:28-36
+union Front {
+  int i;
+  long long ll;
+};
+
+union Front front1;
+union Front front2 = {};// expected-warning {{Initializing union 
'Front' field 'i' only initializes the first 4 of 8 bytes, leaving the 
remaining 4 bytes undefined}}

dexonsmith wrote:
> jfb wrote:
> > dexonsmith wrote:
> > > Are these warnings actionable?  What should users do when they see this 
> > > warning?
> > Good point!
> > 
> > I was thinking about this, and was wondering if I should add a fixit which 
> > suggests using the first wider member of the union. The problem is to offer 
> > the same object representation... that's tricky on its own (there isn't 
> > always an exact match), and then we have to deal with type punning (in C++, 
> > not in C).
> > 
> > So I'd love ideas, because I'm not sure what to do. That being said, I 
> > wrote this partly because D68115 was surprising to folks, and partly 
> > because developers would like to opt-in to this diagnostic to find places 
> > where initialization isn't doing what they think.
> > 
> > Maybe instead we should suggest to leave uninitialized, and use an 
> > assignment, or `memset`?
> > Maybe instead we should suggest to leave uninitialized, and use an 
> > assignment, or `memset`?
> 
> It's not clear to me that those are better.  For example, `memset` doesn't 
> seem right for non-PODs in C++.  I'm not sure what to suggest though.
I'd argue that non-PODs in unions don't seem right either ;-)
It would be easy to diagnose non-trivially-copyable types differently in this 
circumstance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80055



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

[PATCH] D78938: Fixing all comparisons for C++20 compilation.

[JF Bastien via Phabricator via cfe-commits]

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

This seems fine, assuming you've run usual tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78938



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

[PATCH] D68115: Zero initialize padding in unions

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D68115#2040696 , @tschuett wrote:

> As an outsider: In Swift, reading an uninitialized variable is a compile-time 
> error. Clang is not powerful enough to do this analysis. Aren't you really 
> looking for the Clang Intermediate Language (CIL) ?


I have an entire talk about this: https://www.youtube.com/watch?v=I-XUHPimq3o


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68115



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

[PATCH] D80055: Diagnose union tail padding

[JF Bastien via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:264
+def warn_union_tail_padding_uninitialized : Warning<
+  "Initializing union %0 field %1 only initializes the first %2 of %3 bytes, 
leaving the remaining %4 bytes undefined">,
+  InGroup;

Something which I'm not sure matters:

```
union Unnamed {
  union {
int i;
float f;
  };
  char bytes[8];
};

union Unnamed unnamed = { 42 };
```

Will generate the following diagnostic:

```
warning: Initializing union 'Unnamed' field '' only initializes the first 4 of 
8 bytes, leaving the remaining 4 bytes undefined [-Wunion-tail-padding]
union Unnamed unnamed = { 42 };
^
```

I think that's generally how we handle unnamed members anyways?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80055



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

[PATCH] D80055: Diagnose union tail padding

[JF Bastien via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/Basic/DiagnosticGroups.td:825
+def Padding : DiagGroup<"padding", [UnionTailPadding]>,
+  DiagCategory<"Padding Issue">;
+

I'd like to hear what other diagnostics folks think should fit under the 
`Padding` group. I've focused not on declarations which have padding, but 
rather on initializations which leave uninitialized padding behind. It seems 
like internal padding for structs and between array elements would be desirable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80055



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

[PATCH] D68115: Zero initialize padding in unions

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

To get this unblocked a bit, I implemented a diagnostic: 
https://reviews.llvm.org/D80055


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68115



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

[PATCH] D80055: Diagnose union tail padding

[JF Bastien via Phabricator via cfe-commits]

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



Comment at: clang/test/CodeGen/union-tail-padding.c:28-36
+union Front {
+  int i;
+  long long ll;
+};
+
+union Front front1;
+union Front front2 = {};// expected-warning {{Initializing union 
'Front' field 'i' only initializes the first 4 of 8 bytes, leaving the 
remaining 4 bytes undefined}}

dexonsmith wrote:
> Are these warnings actionable?  What should users do when they see this 
> warning?
Good point!

I was thinking about this, and was wondering if I should add a fixit which 
suggests using the first wider member of the union. The problem is to offer the 
same object representation... that's tricky on its own (there isn't always an 
exact match), and then we have to deal with type punning (in C++, not in C).

So I'd love ideas, because I'm not sure what to do. That being said, I wrote 
this partly because D68115 was surprising to folks, and partly because 
developers would like to opt-in to this diagnostic to find places where 
initialization isn't doing what they think.

Maybe instead we should suggest to leave uninitialized, and use an assignment, 
or `memset`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80055



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

[PATCH] D80055: Diagnose union tail padding

[JF Bastien via Phabricator via cfe-commits]

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



Comment at: clang/test/CodeGen/union-tail-padding.c:9
+// RUN: %clang_cc1 -Wunion-tail-padding -xc++ -std=c++17 -triple 
aarch64-apple-darwin %s -emit-llvm -o /dev/null -verify
+// RUN: %clang_cc1 -Wunion-tail-padding -xc++ -std=c++2a -triple 
aarch64-apple-darwin %s -emit-llvm -o /dev/null -verify
+

This test, while it might be surprising, is testing the current behavior of 
clang. I haven't validated that it's correct for each standard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80055



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