[PATCH] D32265: Add __CLANG_ATOMIC__LOCK_FREE macros for use in MSVC compatibility mode.

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In https://reviews.llvm.org/D32265#731710, @EricWF wrote:

> In https://reviews.llvm.org/D32265#731709, @jfb wrote:
>
> > Is it a goal to support Microsoft's STL with this? If so, how does MSVC's 
> > STL implement `is_always_lock_free` at the moment? CL 19 2017 RTW doesn't 
> > seem to have anything ? Presumably they'll 
> > have to do *something*.
>
>
> The goal is to allow libc++ to implement`ATOMIC__LOCK_FREE` on Windows 
> using Clang. As you know libc++ currently uses the `__GCC_ATOMIC_FOO` macros, 
> but those aren't available on Windows.


OK so it's just a hygiene question of blanket-avoiding `__GCC_*` macros in 
clang-ms mode, but using `__CLANG_*` macros are OK? That sounds fine then. 
Would you all-out switch libc++ to use `__CLANG_*` then?

> Also AFAIK MSVC leaves the implementation of atomics up to the library, not 
> the frontend. So W/E MSVC's STL does it's likely not sane or desirable.

Wait what? How does that even work? This calls for a Twitter ping of Billy!


https://reviews.llvm.org/D32265



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

[PATCH] D32265: Add __CLANG_ATOMIC__LOCK_FREE macros for use in MSVC compatibility mode.

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

It looks like Billy is going to do something somewhat similar when he rewrites 
it: https://twitter.com/jfbastien/status/855168230918307840
For now it's kinda `#define IS_LOCK_FREE ¯\_(ツ)_/¯`


https://reviews.llvm.org/D32265



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

[PATCH] D32265: Add __CLANG_ATOMIC__LOCK_FREE macros for use in MSVC compatibility mode.

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Is it a goal to support Microsoft's STL with this? If so, how does MSVC's STL 
implement `is_always_lock_free` at the moment? CL 19 2017 RTW doesn't seem to 
have anything ? Presumably they'll have to do 
*something*.


https://reviews.llvm.org/D32265



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

[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In https://reviews.llvm.org/D42933#1092077, @smeenai wrote:

> In https://reviews.llvm.org/D42933#1092048, @rjmccall wrote:
>
> > I agree that the format-specifier checker is not intended to be a 
> > portability checker.
>


I don't disagree with the original intent, but AFAICT that's exactly the intent 
of the warning I'd like to get rid of. I'm making a very narrow point: there is 
no portability problem with the warning I'm specifically trying to silence 
(using `%z` with `NS[U]Integer` on Darwin). If we decide that `-Wformat` 
shouldn't emit portability warnings then my point is moot, so let's see if 
people agree to that. If not, then my point still stands.

>> Any attempt to check portability problems has to account for two things:
>> 
>> - Not all code is intended to be portable.  If you're going to warn about 
>> portability problems, you need some way to not annoy programmers who might 
>> have good reason to say that they only care about their code running on 
>> Linux / macOS / 64-bit / 32-bit / whatever.  Generally this means splitting 
>> the portability warning out as a separate warning group.

Right, it seems like recently `-Wformat` went against this, and users aren't 
thrilled.

>> - There are always established idioms for solving portability problems.  In 
>> this case, a certain set of important typedefs from the C standard have been 
>> blessed with dedicated format modifiers like "z", but every other typedef in 
>> the world has to find some other solution, either by asserting that some 
>> existing format is good enough (as NSInteger does) or by introducing a macro 
>> that expands to an appropriate format (as some things in POSIX do).  A 
>> proper format-portability warning would have to know about all of these 
>> conventions (in order to check that e.g. part of the format string came from 
>> the right macro), which just seems wildly out-of-scope for a compiler 
>> warning.

We could provide a macro for `NS[U]Integer`, but it's been long enough and 
there's enough existing code with `%z`. Not sure the code churn on user is 
worth it, if we can instead say "`%z` works".

> Apple's current recommendation for using printf with the NSInteger types is 
> casting to a long, per 
> https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Strings/Articles/formatSpecifiers.html.
>  Are you suggesting that recommendation would change to using `%zd` instead?

Indeed, I believe that came from 
https://github.com/llvm-mirror/clang/commit/ec08735e1b6a51c11533311b774bf6336c0a5d63
 and I intend to update documentation once the compiler is updated. It's not 
*wrong*, it's kinda more portable in that it's what you probably want to do if 
it's not `NS[U]Integer`, but it's more typing and users clearly haven't been 
following the recommendation (because let's be honest, `%z` is fine).


Repository:
  rC Clang

https://reviews.llvm.org/D42933



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

[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In https://reviews.llvm.org/D42933#1091809, @jyknight wrote:

> I also think that special casing these two specifiers doesn't make sense. The 
> problem is a general issue -- and one I've often found irritating. This exact 
> same situation comes up all the time in non-Darwin contexts too.


I don't think that's true. In this very specific case the platform guarantees 
that `(sizeof(size_t) == sizeof(NSInteger)) && (sizeof(ssize_t) == 
sizeof(NSUInteger))` for all architectures this platform supports. This exact 
same situation does not come up all the time in other contexts because the 
`int` / `long` / `long long` distinction isn't backed by a portability 
guarantee. The warning is there to say "this code isn't portable!", but in the 
very specific case of `NSInteger` and `NSUInteger` it is and users rely on it 
so it cannot be broken. The warning is therefore spurious, users therefore 
rightly complain.


Repository:
  rC Clang

https://reviews.llvm.org/D42933



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

[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In https://reviews.llvm.org/D42933#1090286, @smeenai wrote:

> Note that the alignment matters in addition to the size.


Sure, but AFAICT from `./lib/Basic/Targets/*` the alignment is also specified 
properly, is it not?

> The pattern I've seen internally is people using `%zd` for NSInteger and 
> `%tu` for NSUInteger, since until clang 6 neither of those were 
> format-checked at all.
> 
> I'd be fine with adding an option to relax the printf checking if the size 
> and alignment of the specifier and the actual type match, even if the types 
> themselves differ (`-Wformat-relaxed` or something similar), so that you'd 
> still get warnings on cases where the specifier mismatch could cause runtime 
> issues.

What are the cases that you're worried about? The only ones I'm trying to 
capture here are `NSInteger` with `%zd` and `NSUInteger` with `%zu`, are there 
others?

> I think that would be preferable to special-casing the Apple types.

If there are more that should be captured and a similar point solution doesn't 
apply, agreed. However I'd like to understand if we agree on the guarantees 
that the platform offers for the two specific cases I'm targeting.


Repository:
  rC Clang

https://reviews.llvm.org/D42933



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

[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.
Herald added a reviewer: javed.absar.

I was just looking at this, and I think @arphaman's patch is pretty much the 
right approach (with 2 suggested fixes below).

I don't think the code we're currently warning on is broken: a user code has 
`NSInteger` with `%zd` or `NSUInteger` with `%zu`, and on all platforms which 
support those types the implementor has guaranteed that `(sizeof(size_t) == 
sizeof(NSInteger)) && (sizeof(ssize_t) == sizeof(NSUInteger))`. I agree that, 
if we're playing C++ pedant and look at the typedefs, then it's undefined 
behavior and the code is broken. However the implementation provided more 
guarantees than C++ does through its platform typedefs so pendantry need not 
apply (or rather: clang should look no further than the typedef, and trust the 
platform in this particular case).

We of course should still warn on sign mismatch.

I don't think this should even be a warning with pedantic on: there's no 
portability issue at all because all OSes and architectures where this could 
ever fire are guaranteed to do the right thing.

Suggested fixes:

1. Don't compare `CastTyName` directly, instead recurse as 
`shouldNotPrintDirectly` does so `typedef` nesting is also handled.
2. Add a test that sign mismatched is still diagnosed.

If you all agree I'll post an updated patch.


Repository:
  rC Clang

https://reviews.llvm.org/D42933



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

[PATCH] D22711: Diagnose invalid failure memory orderings.

[JF Bastien via Phabricator via cfe-commits]

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

Given http://wg21.link/p0418 I think this requires an update. I don't think the 
old behavior is worth supporting in older `-std=` versions, unless we're 
worried that it would make code less portable.


https://reviews.llvm.org/D22711



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

[PATCH] D23041: Un-XFAIL GCC atomics.align

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.
Herald added subscribers: christof, aheejin.

In https://reviews.llvm.org/D23041#632708, @EricWF wrote:

> Have you filed a bug against GCC regarding its current behavior?
>
> Also it seems like a bad idea to add `-fabi-version=6`, since it selects an 
> older ABI version and not a newer one. Testing the old behavior is not what 
> we want.
>
> I think the best plan is to simply split the `vector_size` tests into another 
> file and XFAIL that for GCC. At least then we get some coverage.


Reviving this old thread... Do you think this is still the right approach? I 
can file the GCC bug and split off the test.


https://reviews.llvm.org/D23041



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

[PATCH] D46613: _Atomic of empty struct shouldn't assert

[JF Bastien via Phabricator via cfe-commits]

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

- Assert on zero alignment, instead of making it always byte-aligned.


Repository:
  rC Clang

https://reviews.llvm.org/D46613

Files:
  lib/AST/ASTContext.cpp
  test/CodeGen/c11atomics-ios.c
  test/CodeGen/c11atomics.c


Index: test/CodeGen/c11atomics.c
===
--- test/CodeGen/c11atomics.c
+++ test/CodeGen/c11atomics.c
@@ -474,3 +474,17 @@
   // CHECK:   ret i1 [[RES]]
   return __c11_atomic_compare_exchange_strong(addr, desired, *new, 5, 5);
 }
+
+struct Empty {};
+
+struct Empty test_empty_struct_load(_Atomic(struct Empty)* empty) {
+  // CHECK-LABEL: @test_empty_struct_load(
+  // CHECK: call arm_aapcscc zeroext i8 @__atomic_load_1(i8* %{{.*}}, i32 5)
+  return __c11_atomic_load(empty, 5);
+}
+
+void test_empty_struct_store(_Atomic(struct Empty)* empty, struct Empty value) 
{
+  // CHECK-LABEL: @test_empty_struct_store(
+  // CHECK: call arm_aapcscc void @__atomic_store_1(i8* %{{.*}}, i8 zeroext 
%{{.*}}, i32 5)
+  __c11_atomic_store(empty, value, 5);
+}
Index: test/CodeGen/c11atomics-ios.c
===
--- test/CodeGen/c11atomics-ios.c
+++ test/CodeGen/c11atomics-ios.c
@@ -319,3 +319,19 @@
 
   return __c11_atomic_compare_exchange_strong(addr, desired, *new, 5, 5);
 }
+
+struct Empty {};
+
+struct Empty testEmptyStructLoad(_Atomic(struct Empty)* empty) {
+  // CHECK-LABEL: @testEmptyStructLoad(
+  // CHECK-NOT: @__atomic_load
+  // CHECK: load atomic i8, i8* %{{.*}} seq_cst, align 1
+  return *empty;
+}
+
+void testEmptyStructStore(_Atomic(struct Empty)* empty, struct Empty value) {
+  // CHECK-LABEL: @testEmptyStructStore(
+  // CHECK-NOT: @__atomic_store
+  // CHECK: store atomic i8 %{{.*}}, i8* %{{.*}} seq_cst, align 1
+  *empty = value;
+}
Index: lib/AST/ASTContext.cpp
===
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -1958,10 +1958,16 @@
 Width = Info.Width;
 Align = Info.Align;
 
-// If the size of the type doesn't exceed the platform's max
-// atomic promotion width, make the size and alignment more
-// favorable to atomic operations:
-if (Width != 0 && Width <= Target->getMaxAtomicPromoteWidth()) {
+if (!Width) {
+  // An otherwise zero-sized type should still generate an
+  // atomic operation.
+  Width = Target->getCharWidth();
+  assert(Align);
+} else if (Width <= Target->getMaxAtomicPromoteWidth()) {
+  // If the size of the type doesn't exceed the platform's max
+  // atomic promotion width, make the size and alignment more
+  // favorable to atomic operations:
+
   // Round the size up to a power of 2.
   if (!llvm::isPowerOf2_64(Width))
 Width = llvm::NextPowerOf2(Width);


Index: test/CodeGen/c11atomics.c
===
--- test/CodeGen/c11atomics.c
+++ test/CodeGen/c11atomics.c
@@ -474,3 +474,17 @@
   // CHECK:   ret i1 [[RES]]
   return __c11_atomic_compare_exchange_strong(addr, desired, *new, 5, 5);
 }
+
+struct Empty {};
+
+struct Empty test_empty_struct_load(_Atomic(struct Empty)* empty) {
+  // CHECK-LABEL: @test_empty_struct_load(
+  // CHECK: call arm_aapcscc zeroext i8 @__atomic_load_1(i8* %{{.*}}, i32 5)
+  return __c11_atomic_load(empty, 5);
+}
+
+void test_empty_struct_store(_Atomic(struct Empty)* empty, struct Empty value) {
+  // CHECK-LABEL: @test_empty_struct_store(
+  // CHECK: call arm_aapcscc void @__atomic_store_1(i8* %{{.*}}, i8 zeroext %{{.*}}, i32 5)
+  __c11_atomic_store(empty, value, 5);
+}
Index: test/CodeGen/c11atomics-ios.c
===
--- test/CodeGen/c11atomics-ios.c
+++ test/CodeGen/c11atomics-ios.c
@@ -319,3 +319,19 @@
 
   return __c11_atomic_compare_exchange_strong(addr, desired, *new, 5, 5);
 }
+
+struct Empty {};
+
+struct Empty testEmptyStructLoad(_Atomic(struct Empty)* empty) {
+  // CHECK-LABEL: @testEmptyStructLoad(
+  // CHECK-NOT: @__atomic_load
+  // CHECK: load atomic i8, i8* %{{.*}} seq_cst, align 1
+  return *empty;
+}
+
+void testEmptyStructStore(_Atomic(struct Empty)* empty, struct Empty value) {
+  // CHECK-LABEL: @testEmptyStructStore(
+  // CHECK-NOT: @__atomic_store
+  // CHECK: store atomic i8 %{{.*}}, i8* %{{.*}} seq_cst, align 1
+  *empty = value;
+}
Index: lib/AST/ASTContext.cpp
===
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -1958,10 +1958,16 @@
 Width = Info.Width;
 Align = Info.Align;
 
-// If the size of the type doesn't exceed the platform's max
-// atomic promotion width, make the size and alignment more
-// favorable to atomic operations:
-if (Width != 0 && Width <= Target->getMaxAtomicPromoteWidth()) {
+if (!Width) {
+  // An otherwise zero-sized type should still generate an
+ 

[PATCH] D46613: _Atomic of empty struct shouldn't assert

[JF Bastien via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
jfb marked an inline comment as done.
Closed by commit rC331845: _Atomic of empty struct shouldnt assert 
(authored by jfb, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D46613?vs=145858=145859#toc

Repository:
  rC Clang

https://reviews.llvm.org/D46613

Files:
  lib/AST/ASTContext.cpp
  test/CodeGen/c11atomics-ios.c
  test/CodeGen/c11atomics.c


Index: lib/AST/ASTContext.cpp
===
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -1958,10 +1958,16 @@
 Width = Info.Width;
 Align = Info.Align;
 
-// If the size of the type doesn't exceed the platform's max
-// atomic promotion width, make the size and alignment more
-// favorable to atomic operations:
-if (Width != 0 && Width <= Target->getMaxAtomicPromoteWidth()) {
+if (!Width) {
+  // An otherwise zero-sized type should still generate an
+  // atomic operation.
+  Width = Target->getCharWidth();
+  assert(Align);
+} else if (Width <= Target->getMaxAtomicPromoteWidth()) {
+  // If the size of the type doesn't exceed the platform's max
+  // atomic promotion width, make the size and alignment more
+  // favorable to atomic operations:
+
   // Round the size up to a power of 2.
   if (!llvm::isPowerOf2_64(Width))
 Width = llvm::NextPowerOf2(Width);
Index: test/CodeGen/c11atomics-ios.c
===
--- test/CodeGen/c11atomics-ios.c
+++ test/CodeGen/c11atomics-ios.c
@@ -319,3 +319,19 @@
 
   return __c11_atomic_compare_exchange_strong(addr, desired, *new, 5, 5);
 }
+
+struct Empty {};
+
+struct Empty testEmptyStructLoad(_Atomic(struct Empty)* empty) {
+  // CHECK-LABEL: @testEmptyStructLoad(
+  // CHECK-NOT: @__atomic_load
+  // CHECK: load atomic i8, i8* %{{.*}} seq_cst, align 1
+  return *empty;
+}
+
+void testEmptyStructStore(_Atomic(struct Empty)* empty, struct Empty value) {
+  // CHECK-LABEL: @testEmptyStructStore(
+  // CHECK-NOT: @__atomic_store
+  // CHECK: store atomic i8 %{{.*}}, i8* %{{.*}} seq_cst, align 1
+  *empty = value;
+}
Index: test/CodeGen/c11atomics.c
===
--- test/CodeGen/c11atomics.c
+++ test/CodeGen/c11atomics.c
@@ -474,3 +474,17 @@
   // CHECK:   ret i1 [[RES]]
   return __c11_atomic_compare_exchange_strong(addr, desired, *new, 5, 5);
 }
+
+struct Empty {};
+
+struct Empty test_empty_struct_load(_Atomic(struct Empty)* empty) {
+  // CHECK-LABEL: @test_empty_struct_load(
+  // CHECK: call arm_aapcscc zeroext i8 @__atomic_load_1(i8* %{{.*}}, i32 5)
+  return __c11_atomic_load(empty, 5);
+}
+
+void test_empty_struct_store(_Atomic(struct Empty)* empty, struct Empty value) 
{
+  // CHECK-LABEL: @test_empty_struct_store(
+  // CHECK: call arm_aapcscc void @__atomic_store_1(i8* %{{.*}}, i8 zeroext 
%{{.*}}, i32 5)
+  __c11_atomic_store(empty, value, 5);
+}


Index: lib/AST/ASTContext.cpp
===
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -1958,10 +1958,16 @@
 Width = Info.Width;
 Align = Info.Align;
 
-// If the size of the type doesn't exceed the platform's max
-// atomic promotion width, make the size and alignment more
-// favorable to atomic operations:
-if (Width != 0 && Width <= Target->getMaxAtomicPromoteWidth()) {
+if (!Width) {
+  // An otherwise zero-sized type should still generate an
+  // atomic operation.
+  Width = Target->getCharWidth();
+  assert(Align);
+} else if (Width <= Target->getMaxAtomicPromoteWidth()) {
+  // If the size of the type doesn't exceed the platform's max
+  // atomic promotion width, make the size and alignment more
+  // favorable to atomic operations:
+
   // Round the size up to a power of 2.
   if (!llvm::isPowerOf2_64(Width))
 Width = llvm::NextPowerOf2(Width);
Index: test/CodeGen/c11atomics-ios.c
===
--- test/CodeGen/c11atomics-ios.c
+++ test/CodeGen/c11atomics-ios.c
@@ -319,3 +319,19 @@
 
   return __c11_atomic_compare_exchange_strong(addr, desired, *new, 5, 5);
 }
+
+struct Empty {};
+
+struct Empty testEmptyStructLoad(_Atomic(struct Empty)* empty) {
+  // CHECK-LABEL: @testEmptyStructLoad(
+  // CHECK-NOT: @__atomic_load
+  // CHECK: load atomic i8, i8* %{{.*}} seq_cst, align 1
+  return *empty;
+}
+
+void testEmptyStructStore(_Atomic(struct Empty)* empty, struct Empty value) {
+  // CHECK-LABEL: @testEmptyStructStore(
+  // CHECK-NOT: @__atomic_store
+  // CHECK: store atomic i8 %{{.*}}, i8* %{{.*}} seq_cst, align 1
+  *empty = value;
+}
Index: test/CodeGen/c11atomics.c
===
--- test/CodeGen/c11atomics.c
+++ test/CodeGen/c11atomics.c
@@ -474,3 +474,17 @@
   // CHECK:  

[PATCH] D46613: _Atomic of empty struct shouldn't assert

[JF Bastien via Phabricator via cfe-commits]

jfb created this revision.
jfb added reviewers: arphaman, rjmccall.
Herald added subscribers: cfe-commits, aheejin.

An _Atomic of an empty struct is pretty silly. In general we just widen empty 
structs to hold a byte's worth of storage, and we represent size and alignment 
as 0 internally and let LLVM figure out what to do. For _Atomic it's a bit 
different: the memory model mandates concrete effects occur when atomic 
operations occur, so in most cases actual instructions need to get emitted. 
It's really not worth trying to optimize empty struct atomics by figuring out 
e.g. that a fence would do, even though sane compilers should do optimize 
atomics. Further, wg21.link/p0528 will fix C++20 atomics with padding bits so 
that cmpxchg on them works, which means that we'll likely need to do the 
zero-init song and dance for empty atomic structs anyways (and I think we 
shouldn't special-case this behavior to C++20 because prior standards are just 
broken).

This patch therefore makes a minor change to r176658 "Promote atomic type sizes 
up to a power of two": if the width of the atomic's value type is 0, just use 1 
byte for width and alignment.

This fixes an assertion:

  (NumBits >= MIN_INT_BITS && "bitwidth too small"), function get, file 
../lib/IR/Type.cpp, line 241.

It seems like this has run into other assertions before (namely the unreachable 
Kind check in ImpCastExprToType), but I haven't reproduced that issue with 
tip-of-tree.

rdar://problem/39678063


Repository:
  rC Clang

https://reviews.llvm.org/D46613

Files:
  lib/AST/ASTContext.cpp
  test/CodeGen/c11atomics-ios.c
  test/CodeGen/c11atomics.c


Index: test/CodeGen/c11atomics.c
===
--- test/CodeGen/c11atomics.c
+++ test/CodeGen/c11atomics.c
@@ -474,3 +474,17 @@
   // CHECK:   ret i1 [[RES]]
   return __c11_atomic_compare_exchange_strong(addr, desired, *new, 5, 5);
 }
+
+struct Empty {};
+
+struct Empty test_empty_struct_load(_Atomic(struct Empty)* empty) {
+  // CHECK-LABEL: @test_empty_struct_load(
+  // CHECK: call arm_aapcscc zeroext i8 @__atomic_load_1(i8* %{{.*}}, i32 5)
+  return __c11_atomic_load(empty, 5);
+}
+
+void test_empty_struct_store(_Atomic(struct Empty)* empty, struct Empty value) 
{
+  // CHECK-LABEL: @test_empty_struct_store(
+  // CHECK: call arm_aapcscc void @__atomic_store_1(i8* %{{.*}}, i8 zeroext 
%{{.*}}, i32 5)
+  __c11_atomic_store(empty, value, 5);
+}
Index: test/CodeGen/c11atomics-ios.c
===
--- test/CodeGen/c11atomics-ios.c
+++ test/CodeGen/c11atomics-ios.c
@@ -319,3 +319,19 @@
 
   return __c11_atomic_compare_exchange_strong(addr, desired, *new, 5, 5);
 }
+
+struct Empty {};
+
+struct Empty testEmptyStructLoad(_Atomic(struct Empty)* empty) {
+  // CHECK-LABEL: @testEmptyStructLoad(
+  // CHECK-NOT: @__atomic_load
+  // CHECK: load atomic i8, i8* %{{.*}} seq_cst, align 1
+  return *empty;
+}
+
+void testEmptyStructStore(_Atomic(struct Empty)* empty, struct Empty value) {
+  // CHECK-LABEL: @testEmptyStructStore(
+  // CHECK-NOT: @__atomic_store
+  // CHECK: store atomic i8 %{{.*}}, i8* %{{.*}} seq_cst, align 1
+  *empty = value;
+}
Index: lib/AST/ASTContext.cpp
===
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -1958,10 +1958,16 @@
 Width = Info.Width;
 Align = Info.Align;
 
-// If the size of the type doesn't exceed the platform's max
-// atomic promotion width, make the size and alignment more
-// favorable to atomic operations:
-if (Width != 0 && Width <= Target->getMaxAtomicPromoteWidth()) {
+if (!Width) {
+  // An otherwise zero-sized type should still generate an
+  // atomic operation.
+  Width = Target->getCharWidth();
+  Align = Target->getCharWidth();
+} else if (Width <= Target->getMaxAtomicPromoteWidth()) {
+  // If the size of the type doesn't exceed the platform's max
+  // atomic promotion width, make the size and alignment more
+  // favorable to atomic operations:
+
   // Round the size up to a power of 2.
   if (!llvm::isPowerOf2_64(Width))
 Width = llvm::NextPowerOf2(Width);


Index: test/CodeGen/c11atomics.c
===
--- test/CodeGen/c11atomics.c
+++ test/CodeGen/c11atomics.c
@@ -474,3 +474,17 @@
   // CHECK:   ret i1 [[RES]]
   return __c11_atomic_compare_exchange_strong(addr, desired, *new, 5, 5);
 }
+
+struct Empty {};
+
+struct Empty test_empty_struct_load(_Atomic(struct Empty)* empty) {
+  // CHECK-LABEL: @test_empty_struct_load(
+  // CHECK: call arm_aapcscc zeroext i8 @__atomic_load_1(i8* %{{.*}}, i32 5)
+  return __c11_atomic_load(empty, 5);
+}
+
+void test_empty_struct_store(_Atomic(struct Empty)* empty, struct Empty value) {
+  // CHECK-LABEL: @test_empty_struct_store(
+  // CHECK: call arm_aapcscc void @__atomic_store_1(i8* %{{.*}}, i8 zeroext 

[PATCH] D46613: _Atomic of empty struct shouldn't assert

[JF Bastien via Phabricator via cfe-commits]

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



Comment at: lib/AST/ASTContext.cpp:1965
+  Width = Target->getCharWidth();
+  Align = Target->getCharWidth();
+} else if (Width <= Target->getMaxAtomicPromoteWidth()) {

rjmccall wrote:
> Alignment, unlike size, is definitely never 0.  I think you should leave the 
> original alignment in place; it's a weird case, but we honor over-aligned 
> empty types in a bunch of other places.
Yeah that makes total sense. I turned it to an assert.


Repository:
  rC Clang

https://reviews.llvm.org/D46613



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

[PATCH] D45470: Emit an error when include after

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In https://reviews.llvm.org/D45470#1092212, @vsapsai wrote:

> Here is another approach that should emit an error only when mixing headers
>  causes compilation problems.
>
> Have no ideas how to test the change. `-verify` doesn't work with fatal errors
>  and libcxx doesn't use FileCheck. Performed only manual testing.


This worked with `` before `` as well as with the order 
reversed?

LGTM pending testing answer. This seems simpler than implementing  
http://wg21.link/p0943 outright, though that would still be the best solution 
IMO.




Comment at: libcxx/include/atomic:558
 #endif
+#ifdef kill_dependency
+#error C++ standard library is incompatible with 

I checked and C guarantees that this is a macro :)


https://reviews.llvm.org/D45470



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

[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In https://reviews.llvm.org/D42933#1093503, @smeenai wrote:

> Yeah, I think we all agree now that a portability warning isn't really 
> tractable. Note that even for the warnings that motivated this diff, they 
> should have only fired if `size_t` and NSInteger had separate types, so it 
> wasn't a portability warning in that sense to begin with (as in, it would 
> only warn if there was a mismatch for your current target, not if there was a 
> potential mismatch for any target).
>
> We still have two options:
>
> 1. Special-case NSInteger/NSUInteger on Apple platforms so that they can 
> always be printed using `%z` without any issues.
> 2. Relax the format specifier warnings (either under a new warning option, 
> e.g. `-Wformat-relaxed`, or by relaxing `-Wformat` itself and adding 
> something like `-Wformat-pedantic`) so that you don't warn when the specifier 
> and the actual type have the same size and alignment, even when the actual 
> type is different (which would also cover the case in 1).
>
>   I'm personally in favor of 2, and I can start a discussion on cfe-dev if 
> you think we should try to achieve a broader consensus. Whichever option we 
> went with, we would also have to ensure that the optimizer didn't do anything 
> bad (as @aaron.ballman pointed out), both now and in the future.


2 sounds better overall. However I don’t like a relaxed flag because it’s weird 
when you have relaxed/normal/pedantic. How do they mix? What if I want format 
warnings, but not “portability” ones. I think we either want to move this to 
pedantic and (as you propose) only warm on `-Wformat` if size or alignment 
don’t match. We can also have a separate `-Wformat-portability` warning where 
we try (and fail) to be helpful.

Please do start a thread on cfe-dev, much appreciated!


Repository:
  rC Clang

https://reviews.llvm.org/D42933



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

[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In https://reviews.llvm.org/D42933#1091234, @aaron.ballman wrote:

> In https://reviews.llvm.org/D42933#1090268, @jfb wrote:
>
> > I was just looking at this, and I think @arphaman's patch is pretty much 
> > the right approach (with 2 suggested fixes below).
> >
> > I don't think the code we're currently warning on is broken: a user code 
> > has `NSInteger` with `%zd` or `NSUInteger` with `%zu`, and on all platforms 
> > which support those types the implementor has guaranteed that 
> > `(sizeof(size_t) == sizeof(NSInteger)) && (sizeof(ssize_t) == 
> > sizeof(NSUInteger))`.
>
>
> Yes, but is this guaranteed to be the case or does it happen to be the case? 
> I'm worried about the less mainstream uses where you might find ObjC code 
> where this does not hold but -Wformat points out a portability concern.


For Darwin platform, yes. That's why this diagnostic should only be squelched 
for Darwin platforms.

>> However the implementation provided more guarantees than C++ does through 
>> its platform typedefs so pendantry need not apply (or rather: clang should 
>> look no further than the typedef, and trust the platform in this particular 
>> case).
>> 
>> We of course should still warn on sign mismatch.
>> 
>> I don't think this should even be a warning with pedantic on: there's no 
>> portability issue at all because all OSes and architectures where this could 
>> ever fire are guaranteed to do the right thing.
> 
> Can you point me to this guarantee, as I was unable to find one (but that 
> doesn't mean it doesn't exist)?

Once this patch is committed I'll update the corresponding documentation on 
developer.apple.com

In https://reviews.llvm.org/D42933#1091411, @smeenai wrote:

> In https://reviews.llvm.org/D42933#1091234, @aaron.ballman wrote:
>
> > In https://reviews.llvm.org/D42933#1090268, @jfb wrote:
> >
> > > I was just looking at this, and I think @arphaman's patch is pretty much 
> > > the right approach (with 2 suggested fixes below).
> > >
> > > I don't think the code we're currently warning on is broken: a user code 
> > > has `NSInteger` with `%zd` or `NSUInteger` with `%zu`, and on all 
> > > platforms which support those types the implementor has guaranteed that 
> > > `(sizeof(size_t) == sizeof(NSInteger)) && (sizeof(ssize_t) == 
> > > sizeof(NSUInteger))`.
> >
> >
> > Yes, but is this guaranteed to be the case or does it happen to be the 
> > case? I'm worried about the less mainstream uses where you might find ObjC 
> > code where this does not hold but -Wformat points out a portability concern.
>
>
> Also keep in mind that Obj-C might be used on non-Apple platforms, e.g. 
> there's an active review going on for GNUstep ABI v2 right now 
> (https://reviews.llvm.org/D46052), and WinObjC is also a thing. Those 
> platforms might not necessarily provide the same guarantees or consistency 
> that Apple's do.


Indeed, I want to make sure that this change only affects Darwin platforms.

>>> I agree that, if we're playing C++ pedant and look at the typedefs, then 
>>> it's undefined behavior and the code is broken.
>> 
>> This is reason alone to diagnose the code, because the optimizer is welcome 
>> to use that UB to perform transformations the programmer did not expect. 
>> However, I'm not certain our optimizer is currently paying attention to that 
>> UB directly, so this may not be an immediate issue (but could be a pitfall 
>> for the future) but I'm not certain about other optimizers for which 
>> portability would still be a concern.
> 
> I would honestly find it a bit surprising (and scary) if the optimizer 
> actually took advantage of UB in the case where the size and alignment of the 
> specifier and the actual type matches.

Hear, hear! I'm happy to fix any such optimization out of their misguided 
optimism :-)


Repository:
  rC Clang

https://reviews.llvm.org/D42933



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

[PATCH] D47096: CodeGen: block capture shouldn't ICE

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In https://reviews.llvm.org/D47096#1105455, @rjmccall wrote:

> `children()` is actually defined at the `Stmt` level, and if you look at how 
> it's implemented on e.g. `IfStmt`, you can see that it visits all of the 
> child `Stmt`s, including the if-condition.  So it should be fine.


Thanks for pointing this out! I was reading the code but missed that, and 
visitor seemed like the only reliable way to do what I wanted. Here's an update 
patch.


Repository:
  rC Clang

https://reviews.llvm.org/D47096



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

[PATCH] D47096: CodeGen: block capture shouldn't ICE

[JF Bastien via Phabricator via cfe-commits]

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

- move test


Repository:
  rC Clang

https://reviews.llvm.org/D47096

Files:
  lib/CodeGen/CGDecl.cpp
  test/CodeGenCXX/block-capture.cpp


Index: test/CodeGenCXX/block-capture.cpp
===
--- /dev/null
+++ test/CodeGenCXX/block-capture.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -x c++ -std=c++11 -fblocks -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: %struct.__block_byref_baz = type { i8*, %struct.__block_byref_baz*, 
i32, i32, i32 }
+// CHECK: [[baz:%[0-9a-z_]*]] = alloca %struct.__block_byref_baz
+// CHECK: [[bazref:%[0-9a-z_\.]*]] = getelementptr inbounds 
%struct.__block_byref_baz, %struct.__block_byref_baz* [[baz]], i32 0, i32 1
+// CHECK: store %struct.__block_byref_baz* [[baz]], 
%struct.__block_byref_baz** [[bazref]]
+// CHECK: [[disposable:%[0-9a-z_]*]] = bitcast %struct.__block_byref_baz* 
[[baz]] to i8*
+// CHECK: call void @_Block_object_dispose(i8* [[disposable]]
+
+int main() {
+  __block int baz = [&]() { return 0; }();
+  return 0;
+}
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1244,37 +1244,50 @@
   return emission;
 }
 
+static bool isCapturedBy(const VarDecl &, const Expr *);
+
+/// Determines whether the given __block variable is potentially
+/// captured by the given statement.
+static bool isCapturedBy(const VarDecl , const Stmt *S) {
+  if (const Expr *E = dyn_cast(S))
+return isCapturedBy(Var, E);
+  for (const Stmt *SubStmt : S->children())
+if (isCapturedBy(Var, SubStmt))
+  return true;
+  return false;
+}
+
 /// Determines whether the given __block variable is potentially
 /// captured by the given expression.
-static bool isCapturedBy(const VarDecl , const Expr *e) {
+static bool isCapturedBy(const VarDecl , const Expr *E) {
   // Skip the most common kinds of expressions that make
   // hierarchy-walking expensive.
-  e = e->IgnoreParenCasts();
+  E = E->IgnoreParenCasts();
 
-  if (const BlockExpr *be = dyn_cast(e)) {
-const BlockDecl *block = be->getBlockDecl();
-for (const auto  : block->captures()) {
-  if (I.getVariable() == )
+  if (const BlockExpr *BE = dyn_cast(E)) {
+const BlockDecl *Block = BE->getBlockDecl();
+for (const auto  : Block->captures()) {
+  if (I.getVariable() == )
 return true;
 }
 
 // No need to walk into the subexpressions.
 return false;
   }
 
-  if (const StmtExpr *SE = dyn_cast(e)) {
+  if (const StmtExpr *SE = dyn_cast(E)) {
 const CompoundStmt *CS = SE->getSubStmt();
 for (const auto *BI : CS->body())
-  if (const auto *E = dyn_cast(BI)) {
-if (isCapturedBy(var, E))
-return true;
+  if (const auto *BIE = dyn_cast(BI)) {
+if (isCapturedBy(Var, BIE))
+  return true;
   }
   else if (const auto *DS = dyn_cast(BI)) {
   // special case declarations
   for (const auto *I : DS->decls()) {
   if (const auto *VD = dyn_cast((I))) {
 const Expr *Init = VD->getInit();
-if (Init && isCapturedBy(var, Init))
+if (Init && isCapturedBy(Var, Init))
   return true;
   }
   }
@@ -1286,8 +1299,8 @@
 return false;
   }
 
-  for (const Stmt *SubStmt : e->children())
-if (isCapturedBy(var, cast(SubStmt)))
+  for (const Stmt *SubStmt : E->children())
+if (isCapturedBy(Var, SubStmt))
   return true;
 
   return false;


Index: test/CodeGenCXX/block-capture.cpp
===
--- /dev/null
+++ test/CodeGenCXX/block-capture.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -x c++ -std=c++11 -fblocks -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: %struct.__block_byref_baz = type { i8*, %struct.__block_byref_baz*, i32, i32, i32 }
+// CHECK: [[baz:%[0-9a-z_]*]] = alloca %struct.__block_byref_baz
+// CHECK: [[bazref:%[0-9a-z_\.]*]] = getelementptr inbounds %struct.__block_byref_baz, %struct.__block_byref_baz* [[baz]], i32 0, i32 1
+// CHECK: store %struct.__block_byref_baz* [[baz]], %struct.__block_byref_baz** [[bazref]]
+// CHECK: [[disposable:%[0-9a-z_]*]] = bitcast %struct.__block_byref_baz* [[baz]] to i8*
+// CHECK: call void @_Block_object_dispose(i8* [[disposable]]
+
+int main() {
+  __block int baz = [&]() { return 0; }();
+  return 0;
+}
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1244,37 +1244,50 @@
   return emission;
 }
 
+static bool isCapturedBy(const VarDecl &, const Expr *);
+
+/// Determines whether the given __block variable is potentially
+/// captured by the given statement.
+static bool isCapturedBy(const VarDecl , const Stmt *S) {
+  if (const Expr *E = dyn_cast(S))
+return isCapturedBy(Var, E);
+  for (const Stmt *SubStmt : 

[PATCH] D47096: CodeGen: block capture shouldn't ICE

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In https://reviews.llvm.org/D47096#1105492, @rjmccall wrote:

> Test case should go in test/CodeGenCXX.  Also, there already is a 
> `blocks.cpp` there.


I moved it, but didn't merge with the existing block.cpp because it just checks 
for not crashing. I'd rather make sure that the checks don't inadvertently pick 
up the wrong test.


Repository:
  rC Clang

https://reviews.llvm.org/D47096



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

[PATCH] D47096: CodeGen: block capture shouldn't ICE

[JF Bastien via Phabricator via cfe-commits]

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

- Follow John's suggestion.


Repository:
  rC Clang

https://reviews.llvm.org/D47096

Files:
  lib/CodeGen/CGDecl.cpp
  test/CodeGen/block-capture.cpp


Index: test/CodeGen/block-capture.cpp
===
--- /dev/null
+++ test/CodeGen/block-capture.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -x c++ -std=c++11 -fblocks -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: %struct.__block_byref_baz = type { i8*, %struct.__block_byref_baz*, 
i32, i32, i32 }
+// CHECK: [[baz:%[0-9a-z_]*]] = alloca %struct.__block_byref_baz
+// CHECK: [[bazref:%[0-9a-z_\.]*]] = getelementptr inbounds 
%struct.__block_byref_baz, %struct.__block_byref_baz* [[baz]], i32 0, i32 1
+// CHECK: store %struct.__block_byref_baz* [[baz]], 
%struct.__block_byref_baz** [[bazref]]
+// CHECK: [[disposable:%[0-9a-z_]*]] = bitcast %struct.__block_byref_baz* 
[[baz]] to i8*
+// CHECK: call void @_Block_object_dispose(i8* [[disposable]]
+
+int main() {
+  __block int baz = [&]() { return 0; }();
+  return 0;
+}
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1244,37 +1244,50 @@
   return emission;
 }
 
+static bool isCapturedBy(const VarDecl &, const Expr *);
+
+/// Determines whether the given __block variable is potentially
+/// captured by the given statement.
+static bool isCapturedBy(const VarDecl , const Stmt *S) {
+  if (const Expr *E = dyn_cast(S))
+return isCapturedBy(Var, E);
+  for (const Stmt *SubStmt : S->children())
+if (isCapturedBy(Var, SubStmt))
+  return true;
+  return false;
+}
+
 /// Determines whether the given __block variable is potentially
 /// captured by the given expression.
-static bool isCapturedBy(const VarDecl , const Expr *e) {
+static bool isCapturedBy(const VarDecl , const Expr *E) {
   // Skip the most common kinds of expressions that make
   // hierarchy-walking expensive.
-  e = e->IgnoreParenCasts();
+  E = E->IgnoreParenCasts();
 
-  if (const BlockExpr *be = dyn_cast(e)) {
-const BlockDecl *block = be->getBlockDecl();
-for (const auto  : block->captures()) {
-  if (I.getVariable() == )
+  if (const BlockExpr *BE = dyn_cast(E)) {
+const BlockDecl *Block = BE->getBlockDecl();
+for (const auto  : Block->captures()) {
+  if (I.getVariable() == )
 return true;
 }
 
 // No need to walk into the subexpressions.
 return false;
   }
 
-  if (const StmtExpr *SE = dyn_cast(e)) {
+  if (const StmtExpr *SE = dyn_cast(E)) {
 const CompoundStmt *CS = SE->getSubStmt();
 for (const auto *BI : CS->body())
-  if (const auto *E = dyn_cast(BI)) {
-if (isCapturedBy(var, E))
-return true;
+  if (const auto *BIE = dyn_cast(BI)) {
+if (isCapturedBy(Var, BIE))
+  return true;
   }
   else if (const auto *DS = dyn_cast(BI)) {
   // special case declarations
   for (const auto *I : DS->decls()) {
   if (const auto *VD = dyn_cast((I))) {
 const Expr *Init = VD->getInit();
-if (Init && isCapturedBy(var, Init))
+if (Init && isCapturedBy(Var, Init))
   return true;
   }
   }
@@ -1286,8 +1299,8 @@
 return false;
   }
 
-  for (const Stmt *SubStmt : e->children())
-if (isCapturedBy(var, cast(SubStmt)))
+  for (const Stmt *SubStmt : E->children())
+if (isCapturedBy(Var, SubStmt))
   return true;
 
   return false;


Index: test/CodeGen/block-capture.cpp
===
--- /dev/null
+++ test/CodeGen/block-capture.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -x c++ -std=c++11 -fblocks -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: %struct.__block_byref_baz = type { i8*, %struct.__block_byref_baz*, i32, i32, i32 }
+// CHECK: [[baz:%[0-9a-z_]*]] = alloca %struct.__block_byref_baz
+// CHECK: [[bazref:%[0-9a-z_\.]*]] = getelementptr inbounds %struct.__block_byref_baz, %struct.__block_byref_baz* [[baz]], i32 0, i32 1
+// CHECK: store %struct.__block_byref_baz* [[baz]], %struct.__block_byref_baz** [[bazref]]
+// CHECK: [[disposable:%[0-9a-z_]*]] = bitcast %struct.__block_byref_baz* [[baz]] to i8*
+// CHECK: call void @_Block_object_dispose(i8* [[disposable]]
+
+int main() {
+  __block int baz = [&]() { return 0; }();
+  return 0;
+}
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1244,37 +1244,50 @@
   return emission;
 }
 
+static bool isCapturedBy(const VarDecl &, const Expr *);
+
+/// Determines whether the given __block variable is potentially
+/// captured by the given statement.
+static bool isCapturedBy(const VarDecl , const Stmt *S) {
+  if (const Expr *E = dyn_cast(S))
+return isCapturedBy(Var, E);
+  for (const Stmt *SubStmt : 

[PATCH] D47096: CodeGen: block capture shouldn't ICE

[JF Bastien via Phabricator via cfe-commits]

jfb closed this revision.
jfb added a comment.

r332801


Repository:
  rC Clang

https://reviews.llvm.org/D47096



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

[PATCH] D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin

[JF Bastien via Phabricator via cfe-commits]

jfb created this revision.
jfb added reviewers: ahatanak, vsapsai, alexshap, aaron.ballman, javed.absar, 
jfb, rjmccall.
Herald added subscribers: cfe-commits, aheejin, kristof.beyls.

Pick https://reviews.llvm.org/D42933 back up, and make NSInteger/NSUInteger 
with %zu/%zi specifiers on Darwin warn only in pedantic mode. The default 
-Wformat recently started warning for the following code because of the added 
support for analysis for the '%zi' specifier.

  NSInteger i = NSIntegerMax;
  NSLog(@"max NSInteger = %zi", i);

The problem is that on armv7 %zi is 'long', and NSInteger is typedefed to 'int' 
in Foundation. We should avoid this warning as it's inconvenient to our users: 
it's target specific (happens only on armv7 and not arm64), and breaks their 
existing code. We should also silence the warning for the '%zu' specifier to 
ensure consistency. This is acceptable because Darwin guarantees that, despite 
the unfortunate choice of typedef, sizeof(size_t) == sizeof(NS[U]Integer), the 
warning is therefore noisy for pedantic reasons. Once this is in I'll update 
public documentation.

Related discussion on cfe-dev:
http://lists.llvm.org/pipermail/cfe-dev/2018-May/058050.html

rdar://36874921


Repository:
  rC Clang

https://reviews.llvm.org/D47290

Files:
  include/clang/Analysis/Analyses/FormatString.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Analysis/PrintfFormatString.cpp
  lib/Sema/SemaChecking.cpp
  test/FixIt/fixit-format-ios-nopedantic.m
  test/FixIt/fixit-format-ios.m
  test/SemaObjC/format-size-spec-nsinteger-nopedantic.m
  test/SemaObjC/format-size-spec-nsinteger.m

Index: test/SemaObjC/format-size-spec-nsinteger.m
===
--- /dev/null
+++ test/SemaObjC/format-size-spec-nsinteger.m
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple thumbv7-apple-ios -Wformat-pedantic -fsyntax-only -Wno-objc-root-class %s 2>&1 | FileCheck %s
+
+#if __LP64__
+typedef unsigned long NSUInteger;
+typedef long NSInteger;
+#else
+typedef unsigned int NSUInteger;
+typedef int NSInteger;
+#endif
+
+@class NSString;
+
+extern void NSLog(NSString *format, ...);
+
+void testSizeSpecifier() {
+  NSInteger i = 0;
+  NSUInteger j = 0;
+  NSLog(@"max NSInteger = %zi", i);  // CHECK: values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead
+  NSLog(@"max NSUinteger = %zu", j); // CHECK: values of type 'NSUInteger' should not be used as format arguments; add an explicit cast to 'unsigned long' instead
+}
Index: test/SemaObjC/format-size-spec-nsinteger-nopedantic.m
===
--- /dev/null
+++ test/SemaObjC/format-size-spec-nsinteger-nopedantic.m
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple thumbv7-apple-ios -Wformat -fsyntax-only -fblocks -verify -Wno-objc-root-class %s
+// RUN: %clang_cc1 -triple arm64-apple-ios -Wformat -fsyntax-only -fblocks -verify -Wno-objc-root-class %s
+// expected-no-diagnostics
+
+#if __LP64__
+typedef unsigned long NSUInteger;
+typedef long NSInteger;
+#else
+typedef unsigned int NSUInteger;
+typedef int NSInteger;
+#endif
+
+@class NSString;
+
+extern void NSLog(NSString *format, ...);
+
+void testSizeSpecifier() {
+  NSInteger i = 0;
+  NSUInteger j = 0;
+  NSLog(@"max NSInteger = %zi", i);
+  NSLog(@"max NSUinteger = %zu", j);
+}
Index: test/FixIt/fixit-format-ios.m
===
--- test/FixIt/fixit-format-ios.m
+++ test/FixIt/fixit-format-ios.m
@@ -1,5 +1,5 @@
 // RUN: cp %s %t
-// RUN: %clang_cc1 -triple thumbv7-apple-ios8.0.0 -fsyntax-only -Wformat -fixit %t
+// RUN: %clang_cc1 -triple thumbv7-apple-ios8.0.0 -fsyntax-only -Wformat-pedantic -fixit %t
 // RUN: grep -v CHECK %t | FileCheck %s
 
 int printf(const char * restrict, ...);
Index: test/FixIt/fixit-format-ios-nopedantic.m
===
--- /dev/null
+++ test/FixIt/fixit-format-ios-nopedantic.m
@@ -0,0 +1,21 @@
+// RUN: cp %s %t
+// RUN: %clang_cc1 -triple thumbv7-apple-ios8.0.0 -fsyntax-only -Wformat -Werror -fixit %t
+
+int printf(const char *restrict, ...);
+typedef unsigned int NSUInteger;
+typedef int NSInteger;
+NSUInteger getNSUInteger();
+NSInteger getNSInteger();
+
+void test() {
+  // For thumbv7-apple-ios8.0.0 the underlying type of ssize_t is long
+  // and the underlying type of size_t is unsigned long.
+
+  printf("test 1: %zu", getNSUInteger());
+
+  printf("test 2: %zu %zu", getNSUInteger(), getNSUInteger());
+
+  printf("test 3: %zd", getNSInteger());
+
+  printf("test 4: %zd %zd", getNSInteger(), getNSInteger());
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -6594,11 +6594,11 @@
 ExprTy = TET->getUnderlyingExpr()->getType();
   }
 
-  analyze_printf::ArgType::MatchKind match = AT.matchesType(S.Context, ExprTy);
-
-  if 

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

This is on by default for any version of C? AFAICK `_Accum` isn't on the C17 
draft that I have, I'd expect to have to specify a command-line flag pertaining 
to TR 18037 to get this. At a minimum I'd be OK having it with the GNU variant 
of C, but not the `__ANSI_C__` one.


Repository:
  rC Clang

https://reviews.llvm.org/D46084



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

[PATCH] D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin

[JF Bastien via Phabricator via cfe-commits]

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



Comment at: test/FixIt/fixit-format-ios-nopedantic.m:21
+  printf("test 4: %zd %zd", getNSInteger(), getNSInteger());
+}

alexshap wrote:
> maybe i'm missing smth, but i don't see any verification in this test.
`-Werror` makes the test fail if something is reported.


Repository:
  rC Clang

https://reviews.llvm.org/D47290



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

[PATCH] D47229: Make atomic non-member functions as nonnull

[JF Bastien via Phabricator via cfe-commits]

jfb created this revision.
jfb added a reviewer: arphaman.
Herald added subscribers: cfe-commits, aheejin.

As a companion to libc++ patch https://reviews.llvm.org/D47225, mark builtin 
atomic non-member functions which accept pointers as nonnull.

The atomic non-member functions accept pointers to std::atomic / 
std::atomic_flag as well as to the non-atomic value. These are all dereferenced 
unconditionally when lowered, and therefore will fault if null. It's a tiny 
gotcha for new users, especially when they pass in NULL as expected value 
(instead of passing a pointer to a NULL value).

rdar://problem/18473124


Repository:
  rC Clang

https://reviews.llvm.org/D47229

Files:
  lib/Sema/SemaChecking.cpp
  test/Sema/atomic-ops.c

Index: test/Sema/atomic-ops.c
===
--- test/Sema/atomic-ops.c
+++ test/Sema/atomic-ops.c
@@ -531,8 +531,80 @@
 }
 
 void nullPointerWarning(_Atomic(int) *Ap, int *p, int val) {
-  // The 'expected' pointer shouldn't be NULL.
-  (void)__c11_atomic_compare_exchange_strong(Ap, NULL, val, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
-  (void)atomic_compare_exchange_weak(Ap, ((void*)0), val); // expected-warning {{null passed to a callee that requires a non-null argument}}
-  (void)__atomic_compare_exchange_n(p, NULL, val, 0, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  volatile _Atomic(int) vai;
+  _Atomic(int) ai;
+  volatile int vi = 42;
+  int i = 42;
+
+  __c11_atomic_init((volatile _Atomic(int)*)0, 42); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __c11_atomic_init((_Atomic(int)*)0, 42); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __c11_atomic_store((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __c11_atomic_store((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_load((volatile _Atomic(int)*)0, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_load((_Atomic(int)*)0, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_exchange((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_exchange((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_weak((volatile _Atomic(int)*)0, , 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_weak((_Atomic(int)*)0, , 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_weak(, (int*)0, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_weak(, (int*)0, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_strong((volatile _Atomic(int)*)0, , 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_strong((_Atomic(int)*)0, , 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_strong(, (int*)0, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_strong(, (int*)0, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_add((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_add((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_sub((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_sub((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a 

[PATCH] D47225: Add nonnull; use it for atomics

[JF Bastien via Phabricator via cfe-commits]

jfb created this revision.
jfb added reviewers: arphaman, EricWF.
Herald added subscribers: cfe-commits, christof, aheejin.

The atomic non-member functions accept pointers to std::atomic / 
std::atomic_flag as well as to the non-atomic value. These are all dereferenced 
unconditionally when lowered, and therefore will fault if null. It's a tiny 
gotcha for new users, especially when they pass in NULL as expected value 
(instead of passing a pointer to a NULL value). We can therefore use the 
nonnull attribute to denote that:

- A warning should be generated if the argument is null
- It is undefined behavior if the argument is null (because a dereference will 
segfault)

This patch adds support for this attribute for clang and GCC, and sticks to the 
subset of the syntax both supports. In particular, work around this GCC oddity:

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60625

The attributes are documented:

- https://gcc.gnu.org/onlinedocs/gcc-4.0.0/gcc/Function-Attributes.html
- https://clang.llvm.org/docs/AttributeReference.html#nullability-attributes

I'm authoring a companion clang patch for the __c11_* and __atomic_* builtins, 
which currently only warn on a subset of the pointer parameters.

In all cases the check needs to be explicit and not use the empty nonnull list, 
because some of the overloads are for atomic and the values themselves are 
allowed to be null.

rdar://problem/18473124


Repository:
  rCXX libc++

https://reviews.llvm.org/D47225

Files:
  include/__config
  include/atomic
  test/libcxx/atomics/diagnose_nonnull.fail.cpp

Index: test/libcxx/atomics/diagnose_nonnull.fail.cpp
===
--- /dev/null
+++ test/libcxx/atomics/diagnose_nonnull.fail.cpp
@@ -0,0 +1,92 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// REQUIRES: verify-support
+// UNSUPPORTED: libcpp-has-no-threads
+
+// 
+
+// Test that null pointer parameters are diagnosed.
+
+#include 
+
+int main() {
+  std::atomic ai = ATOMIC_VAR_INIT(0);
+  volatile std::atomic vai = ATOMIC_VAR_INIT(0);
+  int i = 42;
+
+  atomic_is_lock_free((const volatile std::atomic*)0); // expected-error {{null passed to a callee that requires a non-null argument}}
+  atomic_is_lock_free((const std::atomic*)0); // expected-error {{null passed to a callee that requires a non-null argument}}
+  atomic_init((volatile std::atomic*)0, 42); // expected-error {{null passed to a callee that requires a non-null argument}}
+  atomic_init((std::atomic*)0, 42); // expected-error {{null passed to a callee that requires a non-null argument}}
+  atomic_store((volatile std::atomic*)0, 42); // expected-error {{null passed to a callee that requires a non-null argument}}
+  atomic_store((std::atomic*)0, 42); // expected-error {{null passed to a callee that requires a non-null argument}}
+  atomic_store_explicit((volatile std::atomic*)0, 42, std::memory_order_relaxed); // expected-error {{null passed to a callee that requires a non-null argument}}
+  atomic_store_explicit((std::atomic*)0, 42, std::memory_order_relaxed); // expected-error {{null passed to a callee that requires a non-null argument}}
+  (void)atomic_load((const volatile std::atomic*)0); // expected-error {{null passed to a callee that requires a non-null argument}}
+  (void)atomic_load((const std::atomic*)0); // expected-error {{null passed to a callee that requires a non-null argument}}
+  (void)atomic_load_explicit((const volatile std::atomic*)0, std::memory_order_relaxed); // expected-error {{null passed to a callee that requires a non-null argument}}
+  (void)atomic_load_explicit((const std::atomic*)0, std::memory_order_relaxed); // expected-error {{null passed to a callee that requires a non-null argument}}
+  (void)atomic_exchange((volatile std::atomic*)0, 42); // expected-error {{null passed to a callee that requires a non-null argument}}
+  (void)atomic_exchange((std::atomic*)0, 42); // expected-error {{null passed to a callee that requires a non-null argument}}
+  (void)atomic_exchange_explicit((volatile std::atomic*)0, 42, std::memory_order_relaxed); // expected-error {{null passed to a callee that requires a non-null argument}}
+  (void)atomic_exchange_explicit((std::atomic*)0, 42, std::memory_order_relaxed); // expected-error {{null passed to a callee that requires a non-null argument}}
+  (void)atomic_compare_exchange_weak((volatile std::atomic*)0, , 42); // expected-error {{null passed to a callee that requires a non-null argument}}
+  (void)atomic_compare_exchange_weak((std::atomic*)0, , 42); // expected-error {{null passed to a callee that requires a non-null argument}}
+  

[PATCH] D47229: Make atomic non-member functions as nonnull

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Fixed by r333290.


Repository:
  rL LLVM

https://reviews.llvm.org/D47229



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

[PATCH] D47229: Make atomic non-member functions as nonnull

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In https://reviews.llvm.org/D47229#1112549, @jakehehrlich wrote:

> This is causing breaks in fuchsia,
>
> Code that looks like this
>
>   uintptr_t last_unlogged =
>atomic_load_explicit(_tail, memory_order_acquire);
>do { 
>if (last_unlogged == 0)
>return;
>} while (!atomic_compare_exchange_weak_explicit(_tail,
>_unlogged, 0,
>memory_order_acq_rel,
>memory_order_relaxed));
>   
>
> Where unlogged_tail is somewhere on the stack. And 
> 'atomic_compare_exchange_weak_explicit' is an #define alias for 
> '__c11_atomic_compare_exchange_weak'
>
> Full context here: 
> https://fuchsia.googlesource.com/zircon/+/master/third_party/ulib/musl/ldso/dynlink.c#822


Here's a repro for what seems to be happening:

  #include 
  void foo() {
atomic_int s;
int a;
atomic_compare_exchange_weak_explicit(, , 0, memory_order_acq_rel, 
memory_order_relaxed);
  }

Yields:

  ./foo.c:5:3: warning: null passed to a callee that requires a non-null 
argument [-Wnonnull]
atomic_compare_exchange_weak_explicit(, , 0, memory_order_acq_rel, 
memory_order_relaxed);
^ ~
  /s/llvm1/llvm/debug/lib/clang/7.0.0/include/stdatomic.h:144:47: note: 
expanded from macro 'atomic_compare_exchange_weak_explicit'
  #define atomic_compare_exchange_weak_explicit 
__c11_atomic_compare_exchange_weak
^

The problem is that my patch checks that the "desired" value is non-null, but 
that's incorrect because it's not a pointer. I'll do a follow-up fix. Thanks 
for catching this!


Repository:
  rL LLVM

https://reviews.llvm.org/D47229



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

[PATCH] D47225: Add nonnull; use it for atomics

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

GCC in  libcxx-libcxxabi-x86_64-linux-ubuntu-cxx03  seems to mis-handle 
ATOMIC_VAR_INIT:

  File 
/home/llvm-builder/llvm-buildslave-root/libcxx-libcxxabi-x86_64-linux-ubuntu-cxx03/llvm/projects/libcxx/test/libcxx/atomics/diagnose_nonnull.fail.cpp
 Line 20: non-aggregate type 'std::atomic' cannot be initialized with an 
initializer list
  File 
/home/llvm-builder/llvm-buildslave-root/libcxx-libcxxabi-x86_64-linux-ubuntu-cxx03/llvm/projects/libcxx/test/libcxx/atomics/diagnose_nonnull.fail.cpp
 Line 21: non-aggregate type 'volatile std::atomic' cannot be initialized 
with an initializer list

I'll drop the initialization for now, it's not required anyways.


Repository:
  rL LLVM

https://reviews.llvm.org/D47225



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

[PATCH] D47225: Add nonnull; use it for atomics

[JF Bastien via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL25: Add nonnull; use it for atomics (authored by jfb, 
committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D47225

Files:
  libcxx/trunk/include/__config
  libcxx/trunk/include/atomic
  libcxx/trunk/test/libcxx/atomics/diagnose_nonnull.fail.cpp

Index: libcxx/trunk/include/atomic
===
--- libcxx/trunk/include/atomic
+++ libcxx/trunk/include/atomic
@@ -646,19 +646,23 @@
 } // namespace __gcc_atomic
 
 template 
+_LIBCPP_NONNULL(1)
 static inline
 typename enable_if<
 __gcc_atomic::__can_assign::value>::type
-__c11_atomic_init(volatile _Atomic(_Tp)* __a,  _Tp __val) {
+__c11_atomic_init(volatile _Atomic(_Tp)* __a,  _Tp __val)
+{
   __a->__a_value = __val;
 }
 
 template 
+_LIBCPP_NONNULL(1)
 static inline
 typename enable_if<
 !__gcc_atomic::__can_assign::value &&
  __gcc_atomic::__can_assign< _Atomic(_Tp)*, _Tp>::value>::type
-__c11_atomic_init(volatile _Atomic(_Tp)* __a,  _Tp __val) {
+__c11_atomic_init(volatile _Atomic(_Tp)* __a,  _Tp __val)
+{
   // [atomics.types.generic]p1 guarantees _Tp is trivially copyable. Because
   // the default operator= in an object is not volatile, a byte-by-byte copy
   // is required.
@@ -671,7 +675,9 @@
 }
 
 template 
-static inline void __c11_atomic_init(_Atomic(_Tp)* __a,  _Tp __val) {
+_LIBCPP_NONNULL(1)
+static inline void __c11_atomic_init(_Atomic(_Tp)* __a,  _Tp __val)
+{
   __a->__a_value = __val;
 }
 
@@ -684,88 +690,108 @@
 }
 
 template 
+_LIBCPP_NONNULL(1)
 static inline void __c11_atomic_store(volatile _Atomic(_Tp)* __a,  _Tp __val,
-  memory_order __order) {
+  memory_order __order)
+{
   return __atomic_store(&__a->__a_value, &__val,
 __gcc_atomic::__to_gcc_order(__order));
 }
 
 template 
+_LIBCPP_NONNULL(1)
 static inline void __c11_atomic_store(_Atomic(_Tp)* __a,  _Tp __val,
-  memory_order __order) {
+  memory_order __order)
+{
   __atomic_store(&__a->__a_value, &__val,
  __gcc_atomic::__to_gcc_order(__order));
 }
 
 template 
+_LIBCPP_NONNULL(1)
 static inline _Tp __c11_atomic_load(volatile _Atomic(_Tp)* __a,
-memory_order __order) {
+memory_order __order)
+{
   _Tp __ret;
   __atomic_load(&__a->__a_value, &__ret,
 __gcc_atomic::__to_gcc_order(__order));
   return __ret;
 }
 
 template 
-static inline _Tp __c11_atomic_load(_Atomic(_Tp)* __a, memory_order __order) {
+_LIBCPP_NONNULL(1)
+static inline _Tp __c11_atomic_load(_Atomic(_Tp)* __a, memory_order __order)
+{
   _Tp __ret;
   __atomic_load(&__a->__a_value, &__ret,
 __gcc_atomic::__to_gcc_order(__order));
   return __ret;
 }
 
 template 
+_LIBCPP_NONNULL(1)
 static inline _Tp __c11_atomic_exchange(volatile _Atomic(_Tp)* __a,
-_Tp __value, memory_order __order) {
+_Tp __value, memory_order __order)
+{
   _Tp __ret;
   __atomic_exchange(&__a->__a_value, &__value, &__ret,
 __gcc_atomic::__to_gcc_order(__order));
   return __ret;
 }
 
 template 
+_LIBCPP_NONNULL(1)
 static inline _Tp __c11_atomic_exchange(_Atomic(_Tp)* __a, _Tp __value,
-memory_order __order) {
+memory_order __order)
+{
   _Tp __ret;
   __atomic_exchange(&__a->__a_value, &__value, &__ret,
 __gcc_atomic::__to_gcc_order(__order));
   return __ret;
 }
 
 template 
+_LIBCPP_NONNULL(1, 2)
 static inline bool __c11_atomic_compare_exchange_strong(
 volatile _Atomic(_Tp)* __a, _Tp* __expected, _Tp __value,
-memory_order __success, memory_order __failure) {
+memory_order __success, memory_order __failure)
+{
   return __atomic_compare_exchange(&__a->__a_value, __expected, &__value,
false,
__gcc_atomic::__to_gcc_order(__success),
__gcc_atomic::__to_gcc_failure_order(__failure));
 }
 
 template 
+_LIBCPP_NONNULL(1, 2)
 static inline bool __c11_atomic_compare_exchange_strong(
 _Atomic(_Tp)* __a, _Tp* __expected, _Tp __value, memory_order __success,
-memory_order __failure) {
+memory_order __failure)
+{
   return __atomic_compare_exchange(&__a->__a_value, __expected, &__value,
false,
__gcc_atomic::__to_gcc_order(__success),
__gcc_atomic::__to_gcc_failure_order(__failure));
 }
 
 template 
+_LIBCPP_NONNULL(1, 2)
 static inline bool __c11_atomic_compare_exchange_weak(
 volatile _Atomic(_Tp)* __a, _Tp* 

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Can you also add a test for `_Bool _Accum`.

Also, `-enable-fixed-point -x c++` failing.




Comment at: lib/AST/ExprConstant.cpp:7361
+case BuiltinType::ULongAccum:
+  // GCC does not cover FIXED_POINT_TYPE in it's switch stmt and defaults 
to
+  // no_type_class

"its"



Comment at: lib/Basic/TargetInfo.cpp:45
+  AccumWidth = AccumAlign = 32;
+  LongAccumWidth = LongAccumAlign = 64;
   SuitableAlign = 64;

This seems weird because Targets which don't have these values for the 
non-Accum versions will have .e.g. `sizeof(short) != sizeof(short _Accum)`. Is 
there a point in ever having `_Accum` differ in size, width, and alignment from 
the underlying type? If not, can you set these values after the sub-target has 
specified its preferences?


Repository:
  rC Clang

https://reviews.llvm.org/D46084



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

[PATCH] D47229: Make atomic non-member functions as nonnull

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Addressed nit.


Repository:
  rC Clang

https://reviews.llvm.org/D47229



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

[PATCH] D47229: Make atomic non-member functions as nonnull

[JF Bastien via Phabricator via cfe-commits]

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

- Address nit.


Repository:
  rC Clang

https://reviews.llvm.org/D47229

Files:
  lib/Sema/SemaChecking.cpp
  test/Sema/atomic-ops.c

Index: test/Sema/atomic-ops.c
===
--- test/Sema/atomic-ops.c
+++ test/Sema/atomic-ops.c
@@ -531,8 +531,80 @@
 }
 
 void nullPointerWarning(_Atomic(int) *Ap, int *p, int val) {
-  // The 'expected' pointer shouldn't be NULL.
-  (void)__c11_atomic_compare_exchange_strong(Ap, NULL, val, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
-  (void)atomic_compare_exchange_weak(Ap, ((void*)0), val); // expected-warning {{null passed to a callee that requires a non-null argument}}
-  (void)__atomic_compare_exchange_n(p, NULL, val, 0, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  volatile _Atomic(int) vai;
+  _Atomic(int) ai;
+  volatile int vi = 42;
+  int i = 42;
+
+  __c11_atomic_init((volatile _Atomic(int)*)0, 42); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __c11_atomic_init((_Atomic(int)*)0, 42); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __c11_atomic_store((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __c11_atomic_store((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_load((volatile _Atomic(int)*)0, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_load((_Atomic(int)*)0, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_exchange((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_exchange((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_weak((volatile _Atomic(int)*)0, , 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_weak((_Atomic(int)*)0, , 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_weak(, (int*)0, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_weak(, (int*)0, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_strong((volatile _Atomic(int)*)0, , 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_strong((_Atomic(int)*)0, , 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_strong(, (int*)0, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_strong(, (int*)0, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_add((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_add((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_sub((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_sub((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_and((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_and((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_or((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that 

[PATCH] D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin

[JF Bastien via Phabricator via cfe-commits]

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

- Merge format-size-spec-nsinteger


Repository:
  rC Clang

https://reviews.llvm.org/D47290

Files:
  include/clang/Analysis/Analyses/FormatString.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Analysis/PrintfFormatString.cpp
  lib/Sema/SemaChecking.cpp
  test/FixIt/fixit-format-ios-nopedantic.m
  test/FixIt/fixit-format-ios.m
  test/SemaObjC/format-size-spec-nsinteger.m

Index: test/SemaObjC/format-size-spec-nsinteger.m
===
--- /dev/null
+++ test/SemaObjC/format-size-spec-nsinteger.m
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 -triple thumbv7-apple-ios -Wno-objc-root-class -fsyntax-only -verify -Wformat %s
+// RUN: %clang_cc1 -triple thumbv7-apple-ios -Wno-objc-root-class -fsyntax-only -verify -Wformat-pedantic -DPEDANTIC %s
+
+#if !defined(PEDANTIC)
+// expected-no-diagnostics
+#endif
+
+#if __LP64__
+typedef unsigned long NSUInteger;
+typedef long NSInteger;
+#else
+typedef unsigned int NSUInteger;
+typedef int NSInteger;
+#endif
+
+@class NSString;
+
+extern void NSLog(NSString *format, ...);
+
+void testSizeSpecifier() {
+  NSInteger i = 0;
+  NSUInteger j = 0;
+  NSLog(@"max NSInteger = %zi", i);
+  NSLog(@"max NSUinteger = %zu", j);
+
+#if defined(PEDANTIC)
+  // expected-warning@-4 {{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}
+  // expected-warning@-4 {{values of type 'NSUInteger' should not be used as format arguments; add an explicit cast to 'unsigned long' instead}}
+#endif
+}
Index: test/FixIt/fixit-format-ios.m
===
--- test/FixIt/fixit-format-ios.m
+++ test/FixIt/fixit-format-ios.m
@@ -1,5 +1,5 @@
 // RUN: cp %s %t
-// RUN: %clang_cc1 -triple thumbv7-apple-ios8.0.0 -fsyntax-only -Wformat -fixit %t
+// RUN: %clang_cc1 -triple thumbv7-apple-ios8.0.0 -fsyntax-only -Wformat-pedantic -fixit %t
 // RUN: grep -v CHECK %t | FileCheck %s
 
 int printf(const char * restrict, ...);
Index: test/FixIt/fixit-format-ios-nopedantic.m
===
--- /dev/null
+++ test/FixIt/fixit-format-ios-nopedantic.m
@@ -0,0 +1,21 @@
+// RUN: cp %s %t
+// RUN: %clang_cc1 -triple thumbv7-apple-ios8.0.0 -fsyntax-only -Wformat -Werror -fixit %t
+
+int printf(const char *restrict, ...);
+typedef unsigned int NSUInteger;
+typedef int NSInteger;
+NSUInteger getNSUInteger();
+NSInteger getNSInteger();
+
+void test() {
+  // For thumbv7-apple-ios8.0.0 the underlying type of ssize_t is long
+  // and the underlying type of size_t is unsigned long.
+
+  printf("test 1: %zu", getNSUInteger());
+
+  printf("test 2: %zu %zu", getNSUInteger(), getNSUInteger());
+
+  printf("test 3: %zd", getNSInteger());
+
+  printf("test 4: %zd %zd", getNSInteger(), getNSInteger());
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -6594,11 +6594,11 @@
 ExprTy = TET->getUnderlyingExpr()->getType();
   }
 
-  analyze_printf::ArgType::MatchKind match = AT.matchesType(S.Context, ExprTy);
-
-  if (match == analyze_printf::ArgType::Match) {
+  const analyze_printf::ArgType::MatchKind match =
+  AT.matchesType(S.Context, ExprTy);
+  bool pedantic = match == analyze_printf::ArgType::NoMatchPedantic;
+  if (match == analyze_printf::ArgType::Match)
 return true;
-  }
 
   // Look through argument promotions for our error message's reported type.
   // This includes the integral and floating promotions, but excludes array
@@ -6674,6 +6674,11 @@
 QualType CastTy;
 std::tie(CastTy, CastTyName) = shouldNotPrintDirectly(S.Context, IntendedTy, E);
 if (!CastTy.isNull()) {
+  // %zi/%zu are OK to use for NSInteger/NSUInteger of type int
+  // (long in ASTContext). Only complain to pedants.
+  if ((CastTyName == "NSInteger" || CastTyName == "NSUInteger") &&
+  AT.isSizeT() && AT.matchesType(S.Context, CastTy))
+pedantic = true;
   IntendedTy = CastTy;
   ShouldNotPrintDirectly = true;
 }
@@ -6693,10 +6698,10 @@
 CharSourceRange SpecRange = getSpecifierRange(StartSpecifier, SpecifierLen);
 
 if (IntendedTy == ExprTy && !ShouldNotPrintDirectly) {
-  unsigned diag = diag::warn_format_conversion_argument_type_mismatch;
-  if (match == analyze_format_string::ArgType::NoMatchPedantic) {
-diag = diag::warn_format_conversion_argument_type_mismatch_pedantic;
-  }
+  unsigned diag =
+  pedantic
+  ? diag::warn_format_conversion_argument_type_mismatch_pedantic
+  : diag::warn_format_conversion_argument_type_mismatch;
   // In this case, the specifier is wrong and should be changed to match
   // the argument.
   EmitFormatDiagnostic(S.PDiag(diag)
@@ -6752,9 +6757,11 @@
   

[PATCH] D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin

[JF Bastien via Phabricator via cfe-commits]

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



Comment at: test/SemaObjC/format-size-spec-nsinteger.m:18
+  NSUInteger j = 0;
+  NSLog(@"max NSInteger = %zi", i);  // CHECK: values of type 'NSInteger' 
should not be used as format arguments; add an explicit cast to 'long' instead
+  NSLog(@"max NSUinteger = %zu", j); // CHECK: values of type 'NSUInteger' 
should not be used as format arguments; add an explicit cast to 'unsigned long' 
instead

ahatanak wrote:
> This test looks identical to 
> test/SemaObjC/format-size-spec-nsinteger-nopedantic.m except for the CHECK 
> lines. You can probably merge the two files if you separate the CHECK lines 
> from the lines that call NSLog (see test/Sema/format-strings-darwin.c for 
> example).
Cute, I didn't know about this pattern.


Repository:
  rC Clang

https://reviews.llvm.org/D47290



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

[PATCH] D47229: Make atomic non-member functions as nonnull

[JF Bastien via Phabricator via cfe-commits]

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

- Address nit.
- Change suggested by Richard


Repository:
  rC Clang

https://reviews.llvm.org/D47229

Files:
  lib/Sema/SemaChecking.cpp
  test/Sema/atomic-ops.c

Index: test/Sema/atomic-ops.c
===
--- test/Sema/atomic-ops.c
+++ test/Sema/atomic-ops.c
@@ -531,8 +531,80 @@
 }
 
 void nullPointerWarning(_Atomic(int) *Ap, int *p, int val) {
-  // The 'expected' pointer shouldn't be NULL.
-  (void)__c11_atomic_compare_exchange_strong(Ap, NULL, val, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
-  (void)atomic_compare_exchange_weak(Ap, ((void*)0), val); // expected-warning {{null passed to a callee that requires a non-null argument}}
-  (void)__atomic_compare_exchange_n(p, NULL, val, 0, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  volatile _Atomic(int) vai;
+  _Atomic(int) ai;
+  volatile int vi = 42;
+  int i = 42;
+
+  __c11_atomic_init((volatile _Atomic(int)*)0, 42); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __c11_atomic_init((_Atomic(int)*)0, 42); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __c11_atomic_store((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __c11_atomic_store((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_load((volatile _Atomic(int)*)0, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_load((_Atomic(int)*)0, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_exchange((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_exchange((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_weak((volatile _Atomic(int)*)0, , 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_weak((_Atomic(int)*)0, , 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_weak(, (int*)0, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_weak(, (int*)0, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_strong((volatile _Atomic(int)*)0, , 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_strong((_Atomic(int)*)0, , 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_strong(, (int*)0, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_strong(, (int*)0, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_add((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_add((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_sub((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_sub((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_and((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_and((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_or((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning 

[PATCH] D47225: Add nonnull; use it for atomics

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Ping! clang side landed in https://reviews.llvm.org/rL333246


Repository:
  rCXX libc++

https://reviews.llvm.org/D47225



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

[PATCH] D47229: Make atomic non-member functions as nonnull

[JF Bastien via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL333246: Make atomic non-member functions as nonnull 
(authored by jfb, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D47229

Files:
  cfe/trunk/lib/Sema/SemaChecking.cpp
  cfe/trunk/test/Sema/atomic-ops.c

Index: cfe/trunk/test/Sema/atomic-ops.c
===
--- cfe/trunk/test/Sema/atomic-ops.c
+++ cfe/trunk/test/Sema/atomic-ops.c
@@ -531,8 +531,80 @@
 }
 
 void nullPointerWarning(_Atomic(int) *Ap, int *p, int val) {
-  // The 'expected' pointer shouldn't be NULL.
-  (void)__c11_atomic_compare_exchange_strong(Ap, NULL, val, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
-  (void)atomic_compare_exchange_weak(Ap, ((void*)0), val); // expected-warning {{null passed to a callee that requires a non-null argument}}
-  (void)__atomic_compare_exchange_n(p, NULL, val, 0, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  volatile _Atomic(int) vai;
+  _Atomic(int) ai;
+  volatile int vi = 42;
+  int i = 42;
+
+  __c11_atomic_init((volatile _Atomic(int)*)0, 42); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __c11_atomic_init((_Atomic(int)*)0, 42); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __c11_atomic_store((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __c11_atomic_store((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_load((volatile _Atomic(int)*)0, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_load((_Atomic(int)*)0, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_exchange((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_exchange((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_weak((volatile _Atomic(int)*)0, , 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_weak((_Atomic(int)*)0, , 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_weak(, (int*)0, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_weak(, (int*)0, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_strong((volatile _Atomic(int)*)0, , 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_strong((_Atomic(int)*)0, , 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_strong(, (int*)0, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_strong(, (int*)0, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_add((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_add((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_sub((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_sub((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_and((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_and((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a 

[PATCH] D47229: Make atomic non-member functions as nonnull

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: lib/Sema/SemaChecking.cpp:3497
+else if (Form == Copy || Form == Xchg) {
+  if (!IsC11 && !IsN)
+// The value pointer is always dereferenced, a nullptr is 
undefined.

rsmith wrote:
> arphaman wrote:
> > Nit: might make more sense to check if `ByValType` is a pointer here 
> > instead of duplicating the `if` condition from above.
> I think the suggestion was to check `ByValType->isPointerType()` here. But... 
> that doesn't work, because we could have a pointer value being passed by 
> value (where a null is allowed), rather than a value being passed by address 
> (where a null is disallowed). I think this comparison is actually less clear 
> than the `!IsC11 && !IsN` check; factoring out a `bool IsPassedByAddress` or 
> similar instead would aid readability here.
I went with the `bool` as you suggested.


Repository:
  rC Clang

https://reviews.llvm.org/D47229



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

[PATCH] D47096: CodeGen: block capture shouldn't ICE

[JF Bastien via Phabricator via cfe-commits]

jfb created this revision.
jfb added a reviewer: rjmccall.
Herald added subscribers: cfe-commits, aheejin.

When a lambda capture captures a __block in the same statement, the compiler 
asserts out because isCapturedBy assumes that an Expr can only be a BlockExpr, 
StmtExpr, or if it's a Stmt then all the statement's children are expressions. 
That's wrong, we need to visit all sub-statements even if they're not 
expressions to see if they also capture.

Fix this issue by pulling out the isCapturedBy logic to use RecursiveASTVisitor.

rdar://problem/39926584


Repository:
  rC Clang

https://reviews.llvm.org/D47096

Files:
  lib/CodeGen/CGDecl.cpp
  test/CodeGen/block-capture.cpp

Index: test/CodeGen/block-capture.cpp
===
--- /dev/null
+++ test/CodeGen/block-capture.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -x c++ -std=c++11 -fblocks -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: %struct.__block_byref_baz = type { i8*, %struct.__block_byref_baz*, i32, i32, i32 }
+// CHECK: [[baz:%[0-9a-z_]*]] = alloca %struct.__block_byref_baz
+// CHECK: [[bazref:%[0-9a-z_\.]*]] = getelementptr inbounds %struct.__block_byref_baz, %struct.__block_byref_baz* [[baz]], i32 0, i32 1
+// CHECK: store %struct.__block_byref_baz* [[baz]], %struct.__block_byref_baz** [[bazref]]
+// CHECK: [[disposable:%[0-9a-z_]*]] = bitcast %struct.__block_byref_baz* [[baz]] to i8*
+// CHECK: call void @_Block_object_dispose(i8* [[disposable]]
+
+int main() {
+  __block int baz = [&]() { return 0; }();
+  return 0;
+}
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -26,6 +26,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclOpenMP.h"
+#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
@@ -1244,53 +1245,65 @@
   return emission;
 }
 
-/// Determines whether the given __block variable is potentially
-/// captured by the given expression.
-static bool isCapturedBy(const VarDecl , const Expr *e) {
-  // Skip the most common kinds of expressions that make
-  // hierarchy-walking expensive.
-  e = e->IgnoreParenCasts();
+static bool isCapturedBy(const VarDecl &, const Expr *);
 
-  if (const BlockExpr *be = dyn_cast(e)) {
-const BlockDecl *block = be->getBlockDecl();
-for (const auto  : block->captures()) {
-  if (I.getVariable() == )
-return true;
-}
+class IsCapturedBy : public RecursiveASTVisitor {
+  bool IsCaptured = false;
+  const VarDecl 
+  bool setCaptured() {
+IsCaptured = true;
+// End visitation early, we know it's captured.
+return false;
+  }
 
+public:
+  IsCapturedBy(const VarDecl ) : Var(Var) {}
+  bool isCaptured() const { return IsCaptured; }
+
+  bool TraverseBlockExpr(BlockExpr *BE) {
+const BlockDecl *Block = BE->getBlockDecl();
+for (const auto  : Block->captures())
+  if (I.getVariable() == )
+return setCaptured();
 // No need to walk into the subexpressions.
 return false;
   }
 
-  if (const StmtExpr *SE = dyn_cast(e)) {
+  bool TraverseStmtExpr(StmtExpr *SE) {
 const CompoundStmt *CS = SE->getSubStmt();
 for (const auto *BI : CS->body())
   if (const auto *E = dyn_cast(BI)) {
-if (isCapturedBy(var, E))
-return true;
-  }
-  else if (const auto *DS = dyn_cast(BI)) {
-  // special case declarations
-  for (const auto *I : DS->decls()) {
-  if (const auto *VD = dyn_cast((I))) {
-const Expr *Init = VD->getInit();
-if (Init && isCapturedBy(var, Init))
-  return true;
-  }
+if (isCapturedBy(Var, E))
+  return setCaptured();
+  } else if (const auto *DS = dyn_cast(BI)) {
+// special case declarations
+for (const auto *I : DS->decls()) {
+  if (const auto *VD = dyn_cast((I))) {
+const Expr *Init = VD->getInit();
+if (Init && isCapturedBy(Var, Init))
+  return setCaptured();
   }
+}
   }
   else
-// FIXME. Make safe assumption assuming arbitrary statements cause capturing.
-// Later, provide code to poke into statements for capture analysis.
-return true;
-return false;
+// FIXME. Make safe assumption assuming arbitrary statements cause
+// capturing. Later, provide code to poke into statements for capture
+// analysis.
+return setCaptured();
+return true;
   }
+};
 
-  for (const Stmt *SubStmt : e->children())
-if (isCapturedBy(var, cast(SubStmt)))
-  return true;
+/// Determines whether the given __block variable is potentially
+/// captured by the given expression.
+static bool isCapturedBy(const VarDecl , const Expr *E) {
+  // Skip the most common 

[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Based on the cfe-dev discussion 
 we'll want to 
handle the case of NSInteger with `%z` format on Darwin separately from other 
attempts at portability warnings in printf formats. I'll therefore re-post this 
patch as-is and CC all of you.


Repository:
  rC Clang

https://reviews.llvm.org/D42933



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

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

> Actually, scratch that. We will be enabling it since GCC does. Will update 
> this and other relevant C++ related code appropriately.

Could you also add tests which mix _Accum with volatile, _Atomic, _Complex, 
constexpr, inline?


Repository:
  rC Clang

https://reviews.llvm.org/D46084



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

[PATCH] D47096: CodeGen: block capture shouldn't ICE

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In https://reviews.llvm.org/D47096#1105368, @rjmccall wrote:

> RecursiveASTVisitor instantiations are huge.  Can you just make the function 
> take a Stmt and then do the first few checks if it happens to be an Expr?


I'm not super-familiar with the code, so I might be doing something silly.

I did something like this initially (leave the top of the function as-is, and 
instead of cast do dyn_cast to Expr and if that fails to CompoundStmt, 
recursively iterating all the children of the CompoundStmt). My worry was that 
I wasn't traversing all actual children (just CompountStmt's children), and 
AFAICT there's no easy way to say "take any Stmt, and visit its children if it 
has such a method". I could hard-code more Stmt derivatives but that seems 
brittle, I could use the "detection idiom" but that's silly if there's already 
a visitor which does The Right Thing through tablegen magic.

What I can do is what I did earlier, and conservatively say it was captured if 
it's neither an Expr nor a CompoundStmt? Or should I special-case other things 
as well?


Repository:
  rC Clang

https://reviews.llvm.org/D47096



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

[PATCH] D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In https://reviews.llvm.org/D47290#1126443, @aaron.ballman wrote:

> In https://reviews.llvm.org/D47290#1125028, @aaron.ballman wrote:
>
> > Okay, that's fair, but the vendor-specific type for my Windows example is 
> > spelled `DWORD`. I'm really worried that this special case will become a 
> > precedent and we'll wind up with -Wformat being relaxed for everything 
> > based on the same rationale. If that's how the community wants -Wformat to 
> > work, cool, but I'd like to know if we're intending to change (what I see 
> > as) the design of this warning.
>
>
> I spoke with @jfb offline and am less concerned about this patch now. He's 
> welcome to correct me if I misrepresent anything, but the precedent this sets 
> is that a platform "owner" (someone with authority, not Joe Q Random-User) 
> can relax -Wformat as in this patch, but this is not a precedent for a 
> blanket change to -Wformat just because the UB happens to work and we "know" 
> it.


Thanks for asking these questions Aaron, it's helped answer everyone's concerns 
and explain our respective positions. You've certainly summarized what I was 
thinking.

It sounds like you're both OK moving forward with this patch?


Repository:
  rC Clang

https://reviews.llvm.org/D47290



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

[PATCH] D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin

[JF Bastien via Phabricator via cfe-commits]

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

In https://reviews.llvm.org/D47290#1113365, @aaron.ballman wrote:

> This is relaxing `-Wformat` and making users instead write 
> `-Wformat-pedantic` to get the strong guarantees, which is the opposite of 
> what I thought the consensus was. Have I misunderstood something?


From Shoaib's email:

> Based on all the discussion so far, I think the consensus is that we don't 
> want to relax -Wformat by default, but an additional relaxed option would be 
> fine. That works for me (where I can just switch my codebases to use the new 
> warning option), but it wouldn't handle JF Bastien's use case, so he would 
> still want to pursue the relaxations specific to Apple platforms.

This patch is specific to the Darwin platform, as proposed in Alex' original 
patch and as I promised to do in the cfe-dev thread. Shoaib's follow-up would 
be different and do what I think you're referring to.


Repository:
  rC Clang

https://reviews.llvm.org/D47290



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

[PATCH] D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin

[JF Bastien via Phabricator via cfe-commits]

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

- Fix variable capitalization.


Repository:
  rC Clang

https://reviews.llvm.org/D47290

Files:
  include/clang/Analysis/Analyses/FormatString.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Analysis/PrintfFormatString.cpp
  lib/Sema/SemaChecking.cpp
  test/FixIt/fixit-format-ios-nopedantic.m
  test/FixIt/fixit-format-ios.m
  test/SemaObjC/format-size-spec-nsinteger.m

Index: test/SemaObjC/format-size-spec-nsinteger.m
===
--- /dev/null
+++ test/SemaObjC/format-size-spec-nsinteger.m
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 -triple thumbv7-apple-ios -Wno-objc-root-class -fsyntax-only -verify -Wformat %s
+// RUN: %clang_cc1 -triple thumbv7-apple-ios -Wno-objc-root-class -fsyntax-only -verify -Wformat-pedantic -DPEDANTIC %s
+
+#if !defined(PEDANTIC)
+// expected-no-diagnostics
+#endif
+
+#if __LP64__
+typedef unsigned long NSUInteger;
+typedef long NSInteger;
+#else
+typedef unsigned int NSUInteger;
+typedef int NSInteger;
+#endif
+
+@class NSString;
+
+extern void NSLog(NSString *format, ...);
+
+void testSizeSpecifier() {
+  NSInteger i = 0;
+  NSUInteger j = 0;
+  NSLog(@"max NSInteger = %zi", i);
+  NSLog(@"max NSUinteger = %zu", j);
+
+#if defined(PEDANTIC)
+  // expected-warning@-4 {{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}
+  // expected-warning@-4 {{values of type 'NSUInteger' should not be used as format arguments; add an explicit cast to 'unsigned long' instead}}
+#endif
+}
Index: test/FixIt/fixit-format-ios.m
===
--- test/FixIt/fixit-format-ios.m
+++ test/FixIt/fixit-format-ios.m
@@ -1,5 +1,5 @@
 // RUN: cp %s %t
-// RUN: %clang_cc1 -triple thumbv7-apple-ios8.0.0 -fsyntax-only -Wformat -fixit %t
+// RUN: %clang_cc1 -triple thumbv7-apple-ios8.0.0 -fsyntax-only -Wformat-pedantic -fixit %t
 // RUN: grep -v CHECK %t | FileCheck %s
 
 int printf(const char * restrict, ...);
Index: test/FixIt/fixit-format-ios-nopedantic.m
===
--- /dev/null
+++ test/FixIt/fixit-format-ios-nopedantic.m
@@ -0,0 +1,21 @@
+// RUN: cp %s %t
+// RUN: %clang_cc1 -triple thumbv7-apple-ios8.0.0 -fsyntax-only -Wformat -Werror -fixit %t
+
+int printf(const char *restrict, ...);
+typedef unsigned int NSUInteger;
+typedef int NSInteger;
+NSUInteger getNSUInteger();
+NSInteger getNSInteger();
+
+void test() {
+  // For thumbv7-apple-ios8.0.0 the underlying type of ssize_t is long
+  // and the underlying type of size_t is unsigned long.
+
+  printf("test 1: %zu", getNSUInteger());
+
+  printf("test 2: %zu %zu", getNSUInteger(), getNSUInteger());
+
+  printf("test 3: %zd", getNSInteger());
+
+  printf("test 4: %zd %zd", getNSInteger(), getNSInteger());
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -6610,11 +6610,11 @@
 ExprTy = TET->getUnderlyingExpr()->getType();
   }
 
-  analyze_printf::ArgType::MatchKind match = AT.matchesType(S.Context, ExprTy);
-
-  if (match == analyze_printf::ArgType::Match) {
+  const analyze_printf::ArgType::MatchKind Match =
+  AT.matchesType(S.Context, ExprTy);
+  bool Pedantic = Match == analyze_printf::ArgType::NoMatchPedantic;
+  if (Match == analyze_printf::ArgType::Match)
 return true;
-  }
 
   // Look through argument promotions for our error message's reported type.
   // This includes the integral and floating promotions, but excludes array
@@ -6690,32 +6690,37 @@
 QualType CastTy;
 std::tie(CastTy, CastTyName) = shouldNotPrintDirectly(S.Context, IntendedTy, E);
 if (!CastTy.isNull()) {
+  // %zi/%zu are OK to use for NSInteger/NSUInteger of type int
+  // (long in ASTContext). Only complain to pedants.
+  if ((CastTyName == "NSInteger" || CastTyName == "NSUInteger") &&
+  AT.isSizeT() && AT.matchesType(S.Context, CastTy))
+Pedantic = true;
   IntendedTy = CastTy;
   ShouldNotPrintDirectly = true;
 }
   }
 
   // We may be able to offer a FixItHint if it is a supported type.
   PrintfSpecifier fixedFS = FS;
-  bool success =
+  bool Success =
   fixedFS.fixType(IntendedTy, S.getLangOpts(), S.Context, isObjCContext());
 
-  if (success) {
+  if (Success) {
 // Get the fix string from the fixed format specifier
 SmallString<16> buf;
 llvm::raw_svector_ostream os(buf);
 fixedFS.toString(os);
 
 CharSourceRange SpecRange = getSpecifierRange(StartSpecifier, SpecifierLen);
 
 if (IntendedTy == ExprTy && !ShouldNotPrintDirectly) {
-  unsigned diag = diag::warn_format_conversion_argument_type_mismatch;
-  if (match == analyze_format_string::ArgType::NoMatchPedantic) {
-diag = 

[PATCH] D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Addressed comments.


Repository:
  rC Clang

https://reviews.llvm.org/D47290



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

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: include/clang/Basic/TokenKinds.def:393
+// ISO/IEC JTC1 SC22 WG14 N1169 Extension
+KEYWORD(_Accum  , KEYALL)
+

ebevhan wrote:
> I believe that having KEYALL will enable the keyword even if -fno-fixed-point 
> is given. Is this desired? It will mean that `_Accum` will not be a valid 
> identifier in standard C regardless of the flag.
That seems fine: identifiers starting with underscore and a capital letter 
already aren't valid identifiers in C and C++ because they're reserved for the 
implementation.


Repository:
  rC Clang

https://reviews.llvm.org/D46084



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

[PATCH] D47557: Filesystem tests: un-confuse write time

[JF Bastien via Phabricator via cfe-commits]

jfb created this revision.
jfb added reviewers: EricWF, mclow.lists, aemerson.
Herald added subscribers: cfe-commits, christof.

The filesystem test was confused about access versus write / modification time. 
The spec says:

  file_time_type last_write_time(const path& p, error_code& ec) noexcept;
  Returns: The time of last data modification of p, determined as if by the 
value of the POSIX stat structure member st_mtime obtained as if by POSIX 
stat(). The signature with argument ec returns file_time_type::min() if an 
error occurs.

The test was looking at st_atime, not st_mtime, when comparing the result from 
last_write_time. That was probably due to using a pair instead of naming things 
nicely or using types. I opted to rename things so it's clearer.

This caused test bot failures.


Repository:
  rCXX libc++

https://reviews.llvm.org/D47557

Files:
  
test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp


Index: 
test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
===
--- 
test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
+++ 
test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
@@ -32,7 +32,9 @@
 
 using namespace fs;
 
-std::pair GetTimes(path const& p) {
+struct Times { std::time_t access, write; };
+
+Times GetTimes(path const& p) {
 using Clock = file_time_type::clock;
 struct ::stat st;
 if (::stat(p.c_str(), ) == -1) {
@@ -48,11 +50,11 @@
 }
 
 std::time_t LastAccessTime(path const& p) {
-return GetTimes(p).first;
+return GetTimes(p).access;
 }
 
 std::time_t LastWriteTime(path const& p) {
-return GetTimes(p).second;
+return GetTimes(p).write;
 }
 
 std::pair GetSymlinkTimes(path const& p) {
@@ -228,11 +230,10 @@
 const path dir = env.create_dir("dir");
 
 const auto file_times = GetTimes(file);
-const std::time_t file_access_time = file_times.first;
-const std::time_t file_write_time = file_times.second;
+const std::time_t file_write_time = file_times.write;
 const auto dir_times = GetTimes(dir);
-const std::time_t dir_access_time = dir_times.first;
-const std::time_t dir_write_time = dir_times.second;
+const std::time_t dir_access_time = dir_times.access;
+const std::time_t dir_write_time = dir_times.write;
 
 file_time_type ftime = last_write_time(file);
 TEST_CHECK(Clock::to_time_t(ftime) == file_write_time);
@@ -253,8 +254,8 @@
 
 TEST_CHECK(ftime2 > ftime);
 TEST_CHECK(dtime2 > dtime);
-TEST_CHECK(LastAccessTime(file) == file_access_time ||
-   LastAccessTime(file) == Clock::to_time_t(ftime2));
+TEST_CHECK(LastWriteTime(file) == file_write_time ||
+   LastWriteTime(file) == Clock::to_time_t(ftime2));
 TEST_CHECK(LastAccessTime(dir) == dir_access_time);
 }
 
@@ -301,7 +302,7 @@
 };
 for (const auto& TC : cases) {
 const auto old_times = GetTimes(TC.p);
-file_time_type old_time(Sec(old_times.second));
+file_time_type old_time(Sec(old_times.write));
 
 std::error_code ec = GetTestEC();
 last_write_time(TC.p, TC.new_time, ec);
@@ -318,7 +319,7 @@
 TEST_CHECK(got_time <= TC.new_time + Sec(1));
 TEST_CHECK(got_time >= TC.new_time - Sec(1));
 }
-TEST_CHECK(LastAccessTime(TC.p) == old_times.first);
+TEST_CHECK(LastAccessTime(TC.p) == old_times.access);
 }
 }
 }
@@ -348,10 +349,10 @@
 file_time_type  got_time = last_write_time(sym);
 std::time_t got_time_t = Clock::to_time_t(got_time);
 
-TEST_CHECK(got_time_t != old_times.second);
+TEST_CHECK(got_time_t != old_times.write);
 TEST_CHECK(got_time_t == new_time_t);
 TEST_CHECK(LastWriteTime(file) == new_time_t);
-TEST_CHECK(LastAccessTime(sym) == old_times.first);
+TEST_CHECK(LastAccessTime(sym) == old_times.access);
 TEST_CHECK(GetSymlinkTimes(sym) == old_sym_times);
 }
 


Index: test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
===
--- test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
+++ test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
@@ -32,7 +32,9 @@
 
 using namespace fs;
 
-std::pair GetTimes(path const& p) {
+struct Times { std::time_t access, write; };
+
+Times GetTimes(path const& p) {
 using Clock = file_time_type::clock;
 struct ::stat st;
 if (::stat(p.c_str(), ) == -1) {
@@ -48,11 +50,11 @@
 }
 
 std::time_t LastAccessTime(path const& p) {
-return GetTimes(p).first;
+return GetTimes(p).access;
 }
 
 std::time_t LastWriteTime(path const& p) {
-return GetTimes(p).second;
+return GetTimes(p).write;
 }
 
 std::pair 

[PATCH] D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In https://reviews.llvm.org/D47290#1113422, @aaron.ballman wrote:

> In https://reviews.llvm.org/D47290#1113412, @jfb wrote:
>
> > In https://reviews.llvm.org/D47290#1113365, @aaron.ballman wrote:
> >
> > > This is relaxing `-Wformat` and making users instead write 
> > > `-Wformat-pedantic` to get the strong guarantees, which is the opposite 
> > > of what I thought the consensus was. Have I misunderstood something?
> >
> >
> > From Shoaib's email:
> >
> > > Based on all the discussion so far, I think the consensus is that we 
> > > don't want to relax -Wformat by default, but an additional relaxed option 
> > > would be fine. That works for me (where I can just switch my codebases to 
> > > use the new warning option), but it wouldn't handle JF Bastien's use 
> > > case, so he would still want to pursue the relaxations specific to Apple 
> > > platforms.
> >
> > This patch is specific to the Darwin platform, as proposed in Alex' 
> > original patch and as I promised to do in the cfe-dev thread. Shoaib's 
> > follow-up would be different and do what I think you're referring to.
>
>
> There were several people on the thread asking to not relax -Wformat for any 
> target, but instead wanted a separate warning flag to perform a more relaxed 
> check.
>
> http://lists.llvm.org/pipermail/cfe-dev/2018-May/057938.html


I don't think this one is talking about platform-specific warnings, I therefore 
don't think it applies to this patch.

> http://lists.llvm.org/pipermail/cfe-dev/2018-May/058030.html
>  http://lists.llvm.org/pipermail/cfe-dev/2018-May/058039.html

Hans' comment and your +1 are on-topic here, and are the only ones AFAIK 
(others don't care or explicitly agree with this patch). What I'd like to 
emphasize is that the new stricter warnings are causing issues for developers 
on our platform. The warnings are new, and they don't help developers because 
the platform guarantees that their code is fine. That frustrates developers 
because they *know* that we know better (they might even find this discussion 
when searching! ), yet we chose to come in and warn them anyways. I have yet 
to hear a reason why we're doing them a service by warning in this case. It 
really has the feel of a pedantic warning, and that's what this patch makes it.

I'll quote James  
slightly out of context:

> No, this actually has been a thorn in the side of some people at google,
>  causing a good deal of angst (only for a very small number of people,
>  granted). I'd not truly put the blame on the warning, but rather on printf
>  itself -- and that we're still USING printf (and other functions that
>  ultimately wrap printf) all over the place. The long-term plan is certainly
>  to switch to better APIs. But, it'd be nice to be able to reduce the issue
>  in the meantime.
> 
> The problem we have is that Google has an internal "int64" typedef, which
>  predates the availability of C99 "int64_t". We'd like to eliminate this,
>  and switch everything over to using the standard int64_t. (Well, really
>  we'd've liked to have done that years ago...) However, "int64" is defined
>  as "long long", while "int64_t" is defined as "long" on 64-bit linux. A
>  bunch of printf specifiers all throughout the codebase use %lld. This
>  mismatching type-specifier makes it quite difficult to change the
>  type. Automatically updating all the format strings has not been found to
>  be easy.
> 
> If we could eliminate the warning on mismatch of "%lld" vs "long", on a
>  platform where they're the same, that could make things a whole lot easier.

It seems Hans' comment came from the sentiment "warning is the right thing to 
do, and from Google's vast experience it's not a problem". James provides a 
counter-point in line with ours: we receive exactly that feedback for NSInteger 
and `%z`. The code exists, it works fine, the platform guarantees it'll keep 
working fine, and now a tonne of "useless" warnings come in. That's frustrating 
because it means developers have these options:

- Useless code churn in code that otherwise won't change, ever
- Silence the warning, potentially silencing useful warnings later (because 
-Wformat-relaxed could start doing more things)

That's frustrating when you update compilers, especially if your update is 
incremental because only some developers see the warning. It means people are 
reluctant to update, because the first thing they see is us being pedants, not 
the cool new features we bring in. If they saw a flood of really useful 
warnings instead they'd be happy,  but even there they get drowned by these 
format warnings... Kind of a shit sandwich if you'll pardon the graphic 
description.

Hopefully this makes sense? I'm not sure I summarize my context very well. I'm 
happy to talk about it in-person next week too.


Repository:
  rC Clang

https://reviews.llvm.org/D47290

[PATCH] D47557: Filesystem tests: un-confuse write time

[JF Bastien via Phabricator via cfe-commits]

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

- Add back directory test


Repository:
  rCXX libc++

https://reviews.llvm.org/D47557

Files:
  
test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp


Index: 
test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
===
--- 
test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
+++ 
test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
@@ -32,7 +32,9 @@
 
 using namespace fs;
 
-std::pair GetTimes(path const& p) {
+struct Times { std::time_t access, write; };
+
+Times GetTimes(path const& p) {
 using Clock = file_time_type::clock;
 struct ::stat st;
 if (::stat(p.c_str(), ) == -1) {
@@ -48,11 +50,11 @@
 }
 
 std::time_t LastAccessTime(path const& p) {
-return GetTimes(p).first;
+return GetTimes(p).access;
 }
 
 std::time_t LastWriteTime(path const& p) {
-return GetTimes(p).second;
+return GetTimes(p).write;
 }
 
 std::pair GetSymlinkTimes(path const& p) {
@@ -228,11 +230,9 @@
 const path dir = env.create_dir("dir");
 
 const auto file_times = GetTimes(file);
-const std::time_t file_access_time = file_times.first;
-const std::time_t file_write_time = file_times.second;
+const std::time_t file_write_time = file_times.write;
 const auto dir_times = GetTimes(dir);
-const std::time_t dir_access_time = dir_times.first;
-const std::time_t dir_write_time = dir_times.second;
+const std::time_t dir_write_time = dir_times.write;
 
 file_time_type ftime = last_write_time(file);
 TEST_CHECK(Clock::to_time_t(ftime) == file_write_time);
@@ -253,9 +253,8 @@
 
 TEST_CHECK(ftime2 > ftime);
 TEST_CHECK(dtime2 > dtime);
-TEST_CHECK(LastAccessTime(file) == file_access_time ||
-   LastAccessTime(file) == Clock::to_time_t(ftime2));
-TEST_CHECK(LastAccessTime(dir) == dir_access_time);
+TEST_CHECK(LastWriteTime(file) == Clock::to_time_t(ftime2));
+TEST_CHECK(LastWriteTime(dir) == Clock::to_time_t(dtime2));
 }
 
 
@@ -301,7 +300,7 @@
 };
 for (const auto& TC : cases) {
 const auto old_times = GetTimes(TC.p);
-file_time_type old_time(Sec(old_times.second));
+file_time_type old_time(Sec(old_times.write));
 
 std::error_code ec = GetTestEC();
 last_write_time(TC.p, TC.new_time, ec);
@@ -318,7 +317,7 @@
 TEST_CHECK(got_time <= TC.new_time + Sec(1));
 TEST_CHECK(got_time >= TC.new_time - Sec(1));
 }
-TEST_CHECK(LastAccessTime(TC.p) == old_times.first);
+TEST_CHECK(LastAccessTime(TC.p) == old_times.access);
 }
 }
 }
@@ -348,10 +347,10 @@
 file_time_type  got_time = last_write_time(sym);
 std::time_t got_time_t = Clock::to_time_t(got_time);
 
-TEST_CHECK(got_time_t != old_times.second);
+TEST_CHECK(got_time_t != old_times.write);
 TEST_CHECK(got_time_t == new_time_t);
 TEST_CHECK(LastWriteTime(file) == new_time_t);
-TEST_CHECK(LastAccessTime(sym) == old_times.first);
+TEST_CHECK(LastAccessTime(sym) == old_times.access);
 TEST_CHECK(GetSymlinkTimes(sym) == old_sym_times);
 }
 


Index: test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
===
--- test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
+++ test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
@@ -32,7 +32,9 @@
 
 using namespace fs;
 
-std::pair GetTimes(path const& p) {
+struct Times { std::time_t access, write; };
+
+Times GetTimes(path const& p) {
 using Clock = file_time_type::clock;
 struct ::stat st;
 if (::stat(p.c_str(), ) == -1) {
@@ -48,11 +50,11 @@
 }
 
 std::time_t LastAccessTime(path const& p) {
-return GetTimes(p).first;
+return GetTimes(p).access;
 }
 
 std::time_t LastWriteTime(path const& p) {
-return GetTimes(p).second;
+return GetTimes(p).write;
 }
 
 std::pair GetSymlinkTimes(path const& p) {
@@ -228,11 +230,9 @@
 const path dir = env.create_dir("dir");
 
 const auto file_times = GetTimes(file);
-const std::time_t file_access_time = file_times.first;
-const std::time_t file_write_time = file_times.second;
+const std::time_t file_write_time = file_times.write;
 const auto dir_times = GetTimes(dir);
-const std::time_t dir_access_time = dir_times.first;
-const std::time_t dir_write_time = dir_times.second;
+const std::time_t dir_write_time = dir_times.write;
 
 file_time_type ftime = last_write_time(file);
 TEST_CHECK(Clock::to_time_t(ftime) == file_write_time);
@@ -253,9 +253,8 @@
 
 TEST_CHECK(ftime2 > ftime);
 TEST_CHECK(dtime2 > dtime);
-

[PATCH] D47618: __c11_atomic_load's _Atomic can be const

[JF Bastien via Phabricator via cfe-commits]

jfb created this revision.
jfb added a reviewer: rsmith.
Herald added a subscriber: cfe-commits.

C++11 onwards specs the non-member functions atomic_load and 
atomic_load_explicit as taking the atomic by const (potentially volatile) 
pointer. C11, in its infinite wisdom, decided to drop the const, and C17 will 
fix this with DR459 (the current draft forgot to fix B.16, but that’s not the 
normative part).

clang’s lib/Headers/stdatomic.h implements these as #define to the __c11_* 
equivalent, which are builtins with custom typecheck. Fix the typecheck.

https://reviews.llvm.org/D47613 takes care of the libc++ side.

Discussion: http://lists.llvm.org/pipermail/cfe-dev/2018-May/058129.html

rdar://problem/27426936


Repository:
  rC Clang

https://reviews.llvm.org/D47618

Files:
  lib/Sema/SemaChecking.cpp
  test/Sema/atomic-ops.c
  test/SemaOpenCL/atomic-ops.cl


Index: test/SemaOpenCL/atomic-ops.cl
===
--- test/SemaOpenCL/atomic-ops.cl
+++ test/SemaOpenCL/atomic-ops.cl
@@ -58,7 +58,7 @@
   __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group); // 
expected-error {{address argument to atomic operation must be a pointer to 
non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic 
_Atomic(int) *') invalid)}}
+  __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group);
 
   __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_group);
@@ -94,7 +94,7 @@
 
   __opencl_atomic_init(ci, 0); // expected-error {{address argument to atomic 
operation must be a pointer to non-const _Atomic type ('const __generic 
atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
   __opencl_atomic_store(ci, 0, memory_order_release, memory_scope_work_group); 
// expected-error {{address argument to atomic operation must be a pointer to 
non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic 
_Atomic(int) *') invalid)}}
-  __opencl_atomic_load(ci, memory_order_acquire, memory_scope_work_group); // 
expected-error {{address argument to atomic operation must be a pointer to 
non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic 
_Atomic(int) *') invalid)}}
+  __opencl_atomic_load(ci, memory_order_acquire, memory_scope_work_group);
 
   __opencl_atomic_init(, 456);
   __opencl_atomic_init(, (void*)0); // expected-warning{{incompatible 
pointer to integer conversion passing '__generic void *' to parameter of type 
'int'}}
Index: test/Sema/atomic-ops.c
===
--- test/Sema/atomic-ops.c
+++ test/Sema/atomic-ops.c
@@ -115,7 +115,7 @@
   __c11_atomic_load(i, memory_order_seq_cst);
   __c11_atomic_load(p, memory_order_seq_cst);
   __c11_atomic_load(d, memory_order_seq_cst);
-  __c11_atomic_load(ci, memory_order_seq_cst); // expected-error {{address 
argument to atomic operation must be a pointer to non-const _Atomic type 
('const _Atomic(int) *' invalid)}}
+  __c11_atomic_load(ci, memory_order_seq_cst);
 
   int load_n_1 = __atomic_load_n(I, memory_order_relaxed);
   int *load_n_2 = __atomic_load_n(P, memory_order_relaxed);
@@ -222,7 +222,7 @@
 
   __c11_atomic_init(ci, 0); // expected-error {{address argument to atomic 
operation must be a pointer to non-const _Atomic type ('const _Atomic(int) *' 
invalid)}}
   __c11_atomic_store(ci, 0, memory_order_release); // expected-error {{address 
argument to atomic operation must be a pointer to non-const _Atomic type 
('const _Atomic(int) *' invalid)}}
-  __c11_atomic_load(ci, memory_order_acquire); // expected-error {{address 
argument to atomic operation must be a pointer to non-const _Atomic type 
('const _Atomic(int) *' invalid)}}
+  __c11_atomic_load(ci, memory_order_acquire);
 
   // Ensure the  macros behave appropriately.
   atomic_int n = ATOMIC_VAR_INIT(123);
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -3357,7 +3357,7 @@
 << Ptr->getType() << Ptr->getSourceRange();
   return ExprError();
 }
-if (AtomTy.isConstQualified() ||
+if ((Form != Load && Form != LoadCopy && AtomTy.isConstQualified()) ||
 AtomTy.getAddressSpace() == LangAS::opencl_constant) {
   Diag(DRE->getLocStart(), diag::err_atomic_op_needs_non_const_atomic)
   << (AtomTy.isConstQualified() ? 0 : 1) << Ptr->getType()


Index: test/SemaOpenCL/atomic-ops.cl
===
--- test/SemaOpenCL/atomic-ops.cl
+++ test/SemaOpenCL/atomic-ops.cl
@@ -58,7 +58,7 @@
   

[PATCH] D47618: __c11_atomic_load's _Atomic can be const

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Note that I don't touch the OpenCL __constant stuff, because the separate 
address space means this can be actually read-only, which means you can't 
cmpxchg for very wide reads.


Repository:
  rC Clang

https://reviews.llvm.org/D47618



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

[PATCH] D47557: Filesystem tests: un-confuse write time

[JF Bastien via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL333723: Filesystem tests: un-confuse write time (authored by 
jfb, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D47557

Files:
  
libcxx/trunk/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp


Index: 
libcxx/trunk/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
===
--- 
libcxx/trunk/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
+++ 
libcxx/trunk/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
@@ -32,7 +32,9 @@
 
 using namespace fs;
 
-std::pair GetTimes(path const& p) {
+struct Times { std::time_t access, write; };
+
+Times GetTimes(path const& p) {
 using Clock = file_time_type::clock;
 struct ::stat st;
 if (::stat(p.c_str(), ) == -1) {
@@ -48,11 +50,11 @@
 }
 
 std::time_t LastAccessTime(path const& p) {
-return GetTimes(p).first;
+return GetTimes(p).access;
 }
 
 std::time_t LastWriteTime(path const& p) {
-return GetTimes(p).second;
+return GetTimes(p).write;
 }
 
 std::pair GetSymlinkTimes(path const& p) {
@@ -228,11 +230,9 @@
 const path dir = env.create_dir("dir");
 
 const auto file_times = GetTimes(file);
-const std::time_t file_access_time = file_times.first;
-const std::time_t file_write_time = file_times.second;
+const std::time_t file_write_time = file_times.write;
 const auto dir_times = GetTimes(dir);
-const std::time_t dir_access_time = dir_times.first;
-const std::time_t dir_write_time = dir_times.second;
+const std::time_t dir_write_time = dir_times.write;
 
 file_time_type ftime = last_write_time(file);
 TEST_CHECK(Clock::to_time_t(ftime) == file_write_time);
@@ -253,9 +253,8 @@
 
 TEST_CHECK(ftime2 > ftime);
 TEST_CHECK(dtime2 > dtime);
-TEST_CHECK(LastAccessTime(file) == file_access_time ||
-   LastAccessTime(file) == Clock::to_time_t(ftime2));
-TEST_CHECK(LastAccessTime(dir) == dir_access_time);
+TEST_CHECK(LastWriteTime(file) == Clock::to_time_t(ftime2));
+TEST_CHECK(LastWriteTime(dir) == Clock::to_time_t(dtime2));
 }
 
 
@@ -301,7 +300,7 @@
 };
 for (const auto& TC : cases) {
 const auto old_times = GetTimes(TC.p);
-file_time_type old_time(Sec(old_times.second));
+file_time_type old_time(Sec(old_times.write));
 
 std::error_code ec = GetTestEC();
 last_write_time(TC.p, TC.new_time, ec);
@@ -318,7 +317,7 @@
 TEST_CHECK(got_time <= TC.new_time + Sec(1));
 TEST_CHECK(got_time >= TC.new_time - Sec(1));
 }
-TEST_CHECK(LastAccessTime(TC.p) == old_times.first);
+TEST_CHECK(LastAccessTime(TC.p) == old_times.access);
 }
 }
 }
@@ -348,10 +347,10 @@
 file_time_type  got_time = last_write_time(sym);
 std::time_t got_time_t = Clock::to_time_t(got_time);
 
-TEST_CHECK(got_time_t != old_times.second);
+TEST_CHECK(got_time_t != old_times.write);
 TEST_CHECK(got_time_t == new_time_t);
 TEST_CHECK(LastWriteTime(file) == new_time_t);
-TEST_CHECK(LastAccessTime(sym) == old_times.first);
+TEST_CHECK(LastAccessTime(sym) == old_times.access);
 TEST_CHECK(GetSymlinkTimes(sym) == old_sym_times);
 }
 


Index: libcxx/trunk/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
===
--- libcxx/trunk/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
+++ libcxx/trunk/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
@@ -32,7 +32,9 @@
 
 using namespace fs;
 
-std::pair GetTimes(path const& p) {
+struct Times { std::time_t access, write; };
+
+Times GetTimes(path const& p) {
 using Clock = file_time_type::clock;
 struct ::stat st;
 if (::stat(p.c_str(), ) == -1) {
@@ -48,11 +50,11 @@
 }
 
 std::time_t LastAccessTime(path const& p) {
-return GetTimes(p).first;
+return GetTimes(p).access;
 }
 
 std::time_t LastWriteTime(path const& p) {
-return GetTimes(p).second;
+return GetTimes(p).write;
 }
 
 std::pair GetSymlinkTimes(path const& p) {
@@ -228,11 +230,9 @@
 const path dir = env.create_dir("dir");
 
 const auto file_times = GetTimes(file);
-const std::time_t file_access_time = file_times.first;
-const std::time_t file_write_time = file_times.second;
+const std::time_t file_write_time = file_times.write;
 const auto dir_times = GetTimes(dir);
-const std::time_t dir_access_time = dir_times.first;
-const std::time_t dir_write_time = dir_times.second;
+const std::time_t dir_write_time = dir_times.write;
 
 

[PATCH] D47613: Mark __c11_atomic_load as const

[JF Bastien via Phabricator via cfe-commits]

jfb created this revision.
jfb added reviewers: EricWF, mclow.lists.
Herald added subscribers: cfe-commits, christof.

C++11 onwards specs the non-member functions atomic_load and 
atomic_load_explicit as taking the atomic by const (potentially volatile) 
pointer. C11, in its infinite wisdom, decided to drop the const, and C17 will 
fix this with DR459 (the current draft forgot to fix B.16, but that’s not the 
normative part).

This patch fixes the libc++ version of the __c11_atomic_load builtins defined 
for GCC's compatibility sake.

Discussion: http://lists.llvm.org/pipermail/cfe-dev/2018-May/058129.html

rdar://problem/27426936


Repository:
  rCXX libc++

https://reviews.llvm.org/D47613

Files:
  include/atomic


Index: include/atomic
===
--- include/atomic
+++ include/atomic
@@ -698,16 +698,16 @@
 }
 
 template 
-static inline _Tp __c11_atomic_load(volatile _Atomic(_Tp)* __a,
+static inline _Tp __c11_atomic_load(const volatile _Atomic(_Tp)* __a,
 memory_order __order) {
   _Tp __ret;
   __atomic_load(&__a->__a_value, &__ret,
 __gcc_atomic::__to_gcc_order(__order));
   return __ret;
 }
 
 template 
-static inline _Tp __c11_atomic_load(_Atomic(_Tp)* __a, memory_order __order) {
+static inline _Tp __c11_atomic_load(const _Atomic(_Tp)* __a, memory_order 
__order) {
   _Tp __ret;
   __atomic_load(&__a->__a_value, &__ret,
 __gcc_atomic::__to_gcc_order(__order));


Index: include/atomic
===
--- include/atomic
+++ include/atomic
@@ -698,16 +698,16 @@
 }
 
 template 
-static inline _Tp __c11_atomic_load(volatile _Atomic(_Tp)* __a,
+static inline _Tp __c11_atomic_load(const volatile _Atomic(_Tp)* __a,
 memory_order __order) {
   _Tp __ret;
   __atomic_load(&__a->__a_value, &__ret,
 __gcc_atomic::__to_gcc_order(__order));
   return __ret;
 }
 
 template 
-static inline _Tp __c11_atomic_load(_Atomic(_Tp)* __a, memory_order __order) {
+static inline _Tp __c11_atomic_load(const _Atomic(_Tp)* __a, memory_order __order) {
   _Tp __ret;
   __atomic_load(&__a->__a_value, &__ret,
 __gcc_atomic::__to_gcc_order(__order));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47557: Filesystem tests: un-confuse write time

[JF Bastien via Phabricator via cfe-commits]

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

- Remove access time checks, simplify existing check, after talking to EricWF 
on IRC.


Repository:
  rCXX libc++

https://reviews.llvm.org/D47557

Files:
  
test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp


Index: 
test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
===
--- 
test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
+++ 
test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
@@ -32,7 +32,9 @@
 
 using namespace fs;
 
-std::pair GetTimes(path const& p) {
+struct Times { std::time_t access, write; };
+
+Times GetTimes(path const& p) {
 using Clock = file_time_type::clock;
 struct ::stat st;
 if (::stat(p.c_str(), ) == -1) {
@@ -48,11 +50,11 @@
 }
 
 std::time_t LastAccessTime(path const& p) {
-return GetTimes(p).first;
+return GetTimes(p).access;
 }
 
 std::time_t LastWriteTime(path const& p) {
-return GetTimes(p).second;
+return GetTimes(p).write;
 }
 
 std::pair GetSymlinkTimes(path const& p) {
@@ -228,11 +230,9 @@
 const path dir = env.create_dir("dir");
 
 const auto file_times = GetTimes(file);
-const std::time_t file_access_time = file_times.first;
-const std::time_t file_write_time = file_times.second;
+const std::time_t file_write_time = file_times.write;
 const auto dir_times = GetTimes(dir);
-const std::time_t dir_access_time = dir_times.first;
-const std::time_t dir_write_time = dir_times.second;
+const std::time_t dir_write_time = dir_times.write;
 
 file_time_type ftime = last_write_time(file);
 TEST_CHECK(Clock::to_time_t(ftime) == file_write_time);
@@ -253,9 +253,7 @@
 
 TEST_CHECK(ftime2 > ftime);
 TEST_CHECK(dtime2 > dtime);
-TEST_CHECK(LastAccessTime(file) == file_access_time ||
-   LastAccessTime(file) == Clock::to_time_t(ftime2));
-TEST_CHECK(LastAccessTime(dir) == dir_access_time);
+TEST_CHECK(LastWriteTime(file) == Clock::to_time_t(ftime2));
 }
 
 
@@ -301,7 +299,7 @@
 };
 for (const auto& TC : cases) {
 const auto old_times = GetTimes(TC.p);
-file_time_type old_time(Sec(old_times.second));
+file_time_type old_time(Sec(old_times.write));
 
 std::error_code ec = GetTestEC();
 last_write_time(TC.p, TC.new_time, ec);
@@ -318,7 +316,7 @@
 TEST_CHECK(got_time <= TC.new_time + Sec(1));
 TEST_CHECK(got_time >= TC.new_time - Sec(1));
 }
-TEST_CHECK(LastAccessTime(TC.p) == old_times.first);
+TEST_CHECK(LastAccessTime(TC.p) == old_times.access);
 }
 }
 }
@@ -348,10 +346,10 @@
 file_time_type  got_time = last_write_time(sym);
 std::time_t got_time_t = Clock::to_time_t(got_time);
 
-TEST_CHECK(got_time_t != old_times.second);
+TEST_CHECK(got_time_t != old_times.write);
 TEST_CHECK(got_time_t == new_time_t);
 TEST_CHECK(LastWriteTime(file) == new_time_t);
-TEST_CHECK(LastAccessTime(sym) == old_times.first);
+TEST_CHECK(LastAccessTime(sym) == old_times.access);
 TEST_CHECK(GetSymlinkTimes(sym) == old_sym_times);
 }
 


Index: test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
===
--- test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
+++ test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
@@ -32,7 +32,9 @@
 
 using namespace fs;
 
-std::pair GetTimes(path const& p) {
+struct Times { std::time_t access, write; };
+
+Times GetTimes(path const& p) {
 using Clock = file_time_type::clock;
 struct ::stat st;
 if (::stat(p.c_str(), ) == -1) {
@@ -48,11 +50,11 @@
 }
 
 std::time_t LastAccessTime(path const& p) {
-return GetTimes(p).first;
+return GetTimes(p).access;
 }
 
 std::time_t LastWriteTime(path const& p) {
-return GetTimes(p).second;
+return GetTimes(p).write;
 }
 
 std::pair GetSymlinkTimes(path const& p) {
@@ -228,11 +230,9 @@
 const path dir = env.create_dir("dir");
 
 const auto file_times = GetTimes(file);
-const std::time_t file_access_time = file_times.first;
-const std::time_t file_write_time = file_times.second;
+const std::time_t file_write_time = file_times.write;
 const auto dir_times = GetTimes(dir);
-const std::time_t dir_access_time = dir_times.first;
-const std::time_t dir_write_time = dir_times.second;
+const std::time_t dir_write_time = dir_times.write;
 
 file_time_type ftime = last_write_time(file);
 TEST_CHECK(Clock::to_time_t(ftime) == file_write_time);
@@ -253,9 +253,7 @@
 
 TEST_CHECK(ftime2 > ftime);
 TEST_CHECK(dtime2 > dtime);
-

[PATCH] D47613: Mark __c11_atomic_load as const

[JF Bastien via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL333776: Mark __c11_atomic_load as const (authored by jfb, 
committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D47613

Files:
  libcxx/trunk/include/atomic


Index: libcxx/trunk/include/atomic
===
--- libcxx/trunk/include/atomic
+++ libcxx/trunk/include/atomic
@@ -698,16 +698,16 @@
 }
 
 template 
-static inline _Tp __c11_atomic_load(volatile _Atomic(_Tp)* __a,
+static inline _Tp __c11_atomic_load(const volatile _Atomic(_Tp)* __a,
 memory_order __order) {
   _Tp __ret;
   __atomic_load(&__a->__a_value, &__ret,
 __gcc_atomic::__to_gcc_order(__order));
   return __ret;
 }
 
 template 
-static inline _Tp __c11_atomic_load(_Atomic(_Tp)* __a, memory_order __order) {
+static inline _Tp __c11_atomic_load(const _Atomic(_Tp)* __a, memory_order 
__order) {
   _Tp __ret;
   __atomic_load(&__a->__a_value, &__ret,
 __gcc_atomic::__to_gcc_order(__order));


Index: libcxx/trunk/include/atomic
===
--- libcxx/trunk/include/atomic
+++ libcxx/trunk/include/atomic
@@ -698,16 +698,16 @@
 }
 
 template 
-static inline _Tp __c11_atomic_load(volatile _Atomic(_Tp)* __a,
+static inline _Tp __c11_atomic_load(const volatile _Atomic(_Tp)* __a,
 memory_order __order) {
   _Tp __ret;
   __atomic_load(&__a->__a_value, &__ret,
 __gcc_atomic::__to_gcc_order(__order));
   return __ret;
 }
 
 template 
-static inline _Tp __c11_atomic_load(_Atomic(_Tp)* __a, memory_order __order) {
+static inline _Tp __c11_atomic_load(const _Atomic(_Tp)* __a, memory_order __order) {
   _Tp __ret;
   __atomic_load(&__a->__a_value, &__ret,
 __gcc_atomic::__to_gcc_order(__order));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46024: [clang-format] Add SpaceBeforeCpp11BracedList option.

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In https://reviews.llvm.org/D46024#1121242, @rkirsling wrote:

> FWIW, please note that this space-before-brace style is not specific to 
> WebKit; CppCoreGuidelines exhibits it as well:
>  
> http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es23-prefer-the--initializer-syntax


The main point of clang-format, for me , is to not argue about style. Can we... 
not argue about WebKit's and others' choice? :)


Repository:
  rC Clang

https://reviews.llvm.org/D46024



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

[PATCH] D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin

[JF Bastien via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL335393: [Sema] -Wformat-pedantic only for 
NSInteger/NSUInteger %zu/%zi on Darwin (authored by jfb, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D47290

Files:
  cfe/trunk/include/clang/Analysis/Analyses/FormatString.h
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/Analysis/PrintfFormatString.cpp
  cfe/trunk/lib/Sema/SemaChecking.cpp
  cfe/trunk/test/FixIt/fixit-format-ios-nopedantic.m
  cfe/trunk/test/FixIt/fixit-format-ios.m
  cfe/trunk/test/SemaObjC/format-size-spec-nsinteger.m

Index: cfe/trunk/include/clang/Analysis/Analyses/FormatString.h
===
--- cfe/trunk/include/clang/Analysis/Analyses/FormatString.h
+++ cfe/trunk/include/clang/Analysis/Analyses/FormatString.h
@@ -256,26 +256,34 @@
 private:
   const Kind K;
   QualType T;
-  const char *Name;
-  bool Ptr;
+  const char *Name = nullptr;
+  bool Ptr = false, IsSizeT = false;
+
 public:
-  ArgType(Kind k = UnknownTy, const char *n = nullptr)
-  : K(k), Name(n), Ptr(false) {}
-  ArgType(QualType t, const char *n = nullptr)
-  : K(SpecificTy), T(t), Name(n), Ptr(false) {}
-  ArgType(CanQualType t) : K(SpecificTy), T(t), Name(nullptr), Ptr(false) {}
+  ArgType(Kind K = UnknownTy, const char *N = nullptr) : K(K), Name(N) {}
+  ArgType(QualType T, const char *N = nullptr) : K(SpecificTy), T(T), Name(N) {}
+  ArgType(CanQualType T) : K(SpecificTy), T(T) {}
 
   static ArgType Invalid() { return ArgType(InvalidTy); }
   bool isValid() const { return K != InvalidTy; }
 
+  bool isSizeT() const { return IsSizeT; }
+
   /// Create an ArgType which corresponds to the type pointer to A.
   static ArgType PtrTo(const ArgType& A) {
 assert(A.K >= InvalidTy && "ArgType cannot be pointer to invalid/unknown");
 ArgType Res = A;
 Res.Ptr = true;
 return Res;
   }
 
+  /// Create an ArgType which corresponds to the size_t/ssize_t type.
+  static ArgType makeSizeT(const ArgType ) {
+ArgType Res = A;
+Res.IsSizeT = true;
+return Res;
+  }
+
   MatchKind matchesType(ASTContext , QualType argTy) const;
 
   QualType getRepresentativeType(ASTContext ) const;
Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7719,6 +7719,10 @@
   "%select{values of type|enum values with underlying type}2 '%0' should not "
   "be used as format arguments; add an explicit cast to %1 instead">,
   InGroup;
+def warn_format_argument_needs_cast_pedantic : Warning<
+  "%select{values of type|enum values with underlying type}2 '%0' should not "
+  "be used as format arguments; add an explicit cast to %1 instead">,
+  InGroup, DefaultIgnore;
 def warn_printf_positional_arg_exceeds_data_args : Warning <
   "data argument position '%0' exceeds the number of data arguments (%1)">,
   InGroup;
Index: cfe/trunk/test/FixIt/fixit-format-ios.m
===
--- cfe/trunk/test/FixIt/fixit-format-ios.m
+++ cfe/trunk/test/FixIt/fixit-format-ios.m
@@ -1,5 +1,5 @@
 // RUN: cp %s %t
-// RUN: %clang_cc1 -triple thumbv7-apple-ios8.0.0 -fsyntax-only -Wformat -fixit %t
+// RUN: %clang_cc1 -triple thumbv7-apple-ios8.0.0 -fsyntax-only -Wformat-pedantic -fixit %t
 // RUN: grep -v CHECK %t | FileCheck %s
 
 int printf(const char * restrict, ...);
Index: cfe/trunk/test/FixIt/fixit-format-ios-nopedantic.m
===
--- cfe/trunk/test/FixIt/fixit-format-ios-nopedantic.m
+++ cfe/trunk/test/FixIt/fixit-format-ios-nopedantic.m
@@ -0,0 +1,21 @@
+// RUN: cp %s %t
+// RUN: %clang_cc1 -triple thumbv7-apple-ios8.0.0 -fsyntax-only -Wformat -Werror -fixit %t
+
+int printf(const char *restrict, ...);
+typedef unsigned int NSUInteger;
+typedef int NSInteger;
+NSUInteger getNSUInteger();
+NSInteger getNSInteger();
+
+void test() {
+  // For thumbv7-apple-ios8.0.0 the underlying type of ssize_t is long
+  // and the underlying type of size_t is unsigned long.
+
+  printf("test 1: %zu", getNSUInteger());
+
+  printf("test 2: %zu %zu", getNSUInteger(), getNSUInteger());
+
+  printf("test 3: %zd", getNSInteger());
+
+  printf("test 4: %zd %zd", getNSInteger(), getNSInteger());
+}
Index: cfe/trunk/test/SemaObjC/format-size-spec-nsinteger.m
===
--- cfe/trunk/test/SemaObjC/format-size-spec-nsinteger.m
+++ cfe/trunk/test/SemaObjC/format-size-spec-nsinteger.m
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 -triple thumbv7-apple-ios -Wno-objc-root-class -fsyntax-only -verify -Wformat %s
+// RUN: %clang_cc1 -triple thumbv7-apple-ios -Wno-objc-root-class -fsyntax-only -verify -Wformat-pedantic -DPEDANTIC %s
+
+#if 

[PATCH] D46024: [clang-format] Add SpaceBeforeCpp11BracedList option.

[JF Bastien via Phabricator via cfe-commits]

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

On the WebKit side this lgtm. Let's leave some time for clang-format folks to 
chime in.


Repository:
  rC Clang

https://reviews.llvm.org/D46024



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

[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: lib/Sema/SemaChecking.cpp:3098
+  case AtomicExpr::AO__atomic_fetch_umax:
+IsMinMax = true;
+Form = Arithmetic;

Should `__sync_fetch_and_min` and others also set `IsMinMax`?


Repository:
  rC Clang

https://reviews.llvm.org/D46386



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

[PATCH] D45470: Emit an error when mixing and

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

This isn't bad, so I'd go with it, but separately I imagine that we could 
implement the suggestion in http://wg21.link/p0943 and expose it even before 
C++20? Not sure we do this much, but I'd argue that before that fix stdatomic.h 
is just useless anyways, so it's a fine breakage. I imagine that the 
stdatomic.h where it's implemented would be the one injected by clang, not the 
libc++ one?


Repository:
  rC Clang

https://reviews.llvm.org/D45470



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

[PATCH] D47618: __c11_atomic_load's _Atomic can be const

[JF Bastien via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL338743: __c11_atomic_loads _Atomic can be const 
(authored by jfb, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D47618

Files:
  cfe/trunk/lib/Sema/SemaChecking.cpp
  cfe/trunk/test/Sema/atomic-ops.c
  cfe/trunk/test/SemaOpenCL/atomic-ops.cl


Index: cfe/trunk/test/SemaOpenCL/atomic-ops.cl
===
--- cfe/trunk/test/SemaOpenCL/atomic-ops.cl
+++ cfe/trunk/test/SemaOpenCL/atomic-ops.cl
@@ -58,7 +58,8 @@
   __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group); // 
expected-error {{address argument to atomic operation must be a pointer to 
non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic 
_Atomic(int) *') invalid)}}
+  __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_load(i_c, memory_order_seq_cst, memory_scope_work_group); // 
expected-error {{address argument to atomic operation must be a pointer to 
non-constant _Atomic type ('__constant atomic_int *' (aka '__constant 
_Atomic(int) *') invalid)}}
 
   __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_group);
@@ -94,7 +95,7 @@
 
   __opencl_atomic_init(ci, 0); // expected-error {{address argument to atomic 
operation must be a pointer to non-const _Atomic type ('const __generic 
atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
   __opencl_atomic_store(ci, 0, memory_order_release, memory_scope_work_group); 
// expected-error {{address argument to atomic operation must be a pointer to 
non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic 
_Atomic(int) *') invalid)}}
-  __opencl_atomic_load(ci, memory_order_acquire, memory_scope_work_group); // 
expected-error {{address argument to atomic operation must be a pointer to 
non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic 
_Atomic(int) *') invalid)}}
+  __opencl_atomic_load(ci, memory_order_acquire, memory_scope_work_group);
 
   __opencl_atomic_init(, 456);
   __opencl_atomic_init(, (void*)0); // expected-warning{{incompatible 
pointer to integer conversion passing '__generic void *' to parameter of type 
'int'}}
Index: cfe/trunk/test/Sema/atomic-ops.c
===
--- cfe/trunk/test/Sema/atomic-ops.c
+++ cfe/trunk/test/Sema/atomic-ops.c
@@ -115,7 +115,7 @@
   __c11_atomic_load(i, memory_order_seq_cst);
   __c11_atomic_load(p, memory_order_seq_cst);
   __c11_atomic_load(d, memory_order_seq_cst);
-  __c11_atomic_load(ci, memory_order_seq_cst); // expected-error {{address 
argument to atomic operation must be a pointer to non-const _Atomic type 
('const _Atomic(int) *' invalid)}}
+  __c11_atomic_load(ci, memory_order_seq_cst);
 
   int load_n_1 = __atomic_load_n(I, memory_order_relaxed);
   int *load_n_2 = __atomic_load_n(P, memory_order_relaxed);
@@ -222,7 +222,7 @@
 
   __c11_atomic_init(ci, 0); // expected-error {{address argument to atomic 
operation must be a pointer to non-const _Atomic type ('const _Atomic(int) *' 
invalid)}}
   __c11_atomic_store(ci, 0, memory_order_release); // expected-error {{address 
argument to atomic operation must be a pointer to non-const _Atomic type 
('const _Atomic(int) *' invalid)}}
-  __c11_atomic_load(ci, memory_order_acquire); // expected-error {{address 
argument to atomic operation must be a pointer to non-const _Atomic type 
('const _Atomic(int) *' invalid)}}
+  __c11_atomic_load(ci, memory_order_acquire);
 
   // Ensure the  macros behave appropriately.
   atomic_int n = ATOMIC_VAR_INIT(123);
Index: cfe/trunk/lib/Sema/SemaChecking.cpp
===
--- cfe/trunk/lib/Sema/SemaChecking.cpp
+++ cfe/trunk/lib/Sema/SemaChecking.cpp
@@ -4347,7 +4347,7 @@
 << Ptr->getType() << Ptr->getSourceRange();
   return ExprError();
 }
-if (AtomTy.isConstQualified() ||
+if ((Form != Load && Form != LoadCopy && AtomTy.isConstQualified()) ||
 AtomTy.getAddressSpace() == LangAS::opencl_constant) {
   Diag(DRE->getLocStart(), diag::err_atomic_op_needs_non_const_atomic)
   << (AtomTy.isConstQualified() ? 0 : 1) << Ptr->getType()


Index: cfe/trunk/test/SemaOpenCL/atomic-ops.cl
===
--- cfe/trunk/test/SemaOpenCL/atomic-ops.cl
+++ cfe/trunk/test/SemaOpenCL/atomic-ops.cl
@@ -58,7 +58,8 @@
   __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(p, 

[PATCH] D49771: CodeGen: use non-zero memset when possible for automatic variables

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In https://reviews.llvm.org/D49771#1183641, @mehdi_amini wrote:

> > I'm worried, however, about generating a bunch more code than needed from 
> > clang in the hopes that the compiler will clean it up later.
>
> Isn't a strong design component of clang/LLVM? Clang does not try to generate 
> "smart" code and leave it up to LLVM to clean it up.


The code around this one, and lack of code in LLVM, seem to disagree. :-)


Repository:
  rL LLVM

https://reviews.llvm.org/D49771



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

[PATCH] D49771: CodeGen: use non-zero memset when possible for automatic variables

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In https://reviews.llvm.org/D49771#1181008, @mehdi_amini wrote:

> I'm curious: isn't the kind of optimization we should expect LLVM to provide?


Maybe? It seems obvious to do here since we know we'll probably want to be 
doing it, and I have another patch I'm working on which will make it that much 
more obviously useful to have here. The middle-end can definitely figure it out 
but it just seems like more work, later, so in the meantime we'd be looking at 
more stuff.


Repository:
  rL LLVM

https://reviews.llvm.org/D49771



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

[PATCH] D49771: CodeGen: use non-zero memset when possible for automatic variables

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In https://reviews.llvm.org/D49771#1183562, @mehdi_amini wrote:

> In https://reviews.llvm.org/D49771#1183380, @jfb wrote:
>
> > In https://reviews.llvm.org/D49771#1181008, @mehdi_amini wrote:
> >
> > > I'm curious: isn't the kind of optimization we should expect LLVM to 
> > > provide?
> >
> >
> > Maybe? It seems obvious to do here since we know we'll probably want to be 
> > doing it, and I have another patch I'm working on which will make it that 
> > much more obviously useful to have here. The middle-end can definitely 
> > figure it out but it just seems like more work, later, so in the meantime 
> > we'd be looking at more stuff.
>
>
> I'm not asking where is it easier to do, but where does it make the most 
> sense :)


What I mean by "easy" is: we know we're likely to want this type of code, 
there's not much pattern recognition needed on our part here. Were we to wait 
we'd need to do more work. I believe this statement will become truer over time.

> Doing such in LLVM in general means catching more patterns (i.e. after 
> inlining, etc.), and also catching it from multiple frontend. So in general 
> I'm worried when I see optimizations implemented in the frontend  instead of 
> the middle end.

Agreed, LLVM could also do it, and it would likely be useful to do so. I'm 
worried, however, about generating a bunch more code than needed from clang in 
the hopes that the compiler will clean it up later.


Repository:
  rL LLVM

https://reviews.llvm.org/D49771



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

[PATCH] D47618: __c11_atomic_load's _Atomic can be const

[JF Bastien via Phabricator via cfe-commits]

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

- Add constant AS pointer test.


Repository:
  rC Clang

https://reviews.llvm.org/D47618

Files:
  lib/Sema/SemaChecking.cpp
  test/Sema/atomic-ops.c
  test/SemaOpenCL/atomic-ops.cl


Index: test/SemaOpenCL/atomic-ops.cl
===
--- test/SemaOpenCL/atomic-ops.cl
+++ test/SemaOpenCL/atomic-ops.cl
@@ -58,7 +58,8 @@
   __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group); // 
expected-error {{address argument to atomic operation must be a pointer to 
non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic 
_Atomic(int) *') invalid)}}
+  __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_load(i_c, memory_order_seq_cst, memory_scope_work_group); // 
expected-error {{address argument to atomic operation must be a pointer to 
non-constant _Atomic type ('__constant atomic_int *' (aka '__constant 
_Atomic(int) *') invalid)}}
 
   __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_group);
@@ -94,7 +95,7 @@
 
   __opencl_atomic_init(ci, 0); // expected-error {{address argument to atomic 
operation must be a pointer to non-const _Atomic type ('const __generic 
atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
   __opencl_atomic_store(ci, 0, memory_order_release, memory_scope_work_group); 
// expected-error {{address argument to atomic operation must be a pointer to 
non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic 
_Atomic(int) *') invalid)}}
-  __opencl_atomic_load(ci, memory_order_acquire, memory_scope_work_group); // 
expected-error {{address argument to atomic operation must be a pointer to 
non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic 
_Atomic(int) *') invalid)}}
+  __opencl_atomic_load(ci, memory_order_acquire, memory_scope_work_group);
 
   __opencl_atomic_init(, 456);
   __opencl_atomic_init(, (void*)0); // expected-warning{{incompatible 
pointer to integer conversion passing '__generic void *' to parameter of type 
'int'}}
Index: test/Sema/atomic-ops.c
===
--- test/Sema/atomic-ops.c
+++ test/Sema/atomic-ops.c
@@ -115,7 +115,7 @@
   __c11_atomic_load(i, memory_order_seq_cst);
   __c11_atomic_load(p, memory_order_seq_cst);
   __c11_atomic_load(d, memory_order_seq_cst);
-  __c11_atomic_load(ci, memory_order_seq_cst); // expected-error {{address 
argument to atomic operation must be a pointer to non-const _Atomic type 
('const _Atomic(int) *' invalid)}}
+  __c11_atomic_load(ci, memory_order_seq_cst);
 
   int load_n_1 = __atomic_load_n(I, memory_order_relaxed);
   int *load_n_2 = __atomic_load_n(P, memory_order_relaxed);
@@ -222,7 +222,7 @@
 
   __c11_atomic_init(ci, 0); // expected-error {{address argument to atomic 
operation must be a pointer to non-const _Atomic type ('const _Atomic(int) *' 
invalid)}}
   __c11_atomic_store(ci, 0, memory_order_release); // expected-error {{address 
argument to atomic operation must be a pointer to non-const _Atomic type 
('const _Atomic(int) *' invalid)}}
-  __c11_atomic_load(ci, memory_order_acquire); // expected-error {{address 
argument to atomic operation must be a pointer to non-const _Atomic type 
('const _Atomic(int) *' invalid)}}
+  __c11_atomic_load(ci, memory_order_acquire);
 
   // Ensure the  macros behave appropriately.
   atomic_int n = ATOMIC_VAR_INIT(123);
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -4347,7 +4347,7 @@
 << Ptr->getType() << Ptr->getSourceRange();
   return ExprError();
 }
-if (AtomTy.isConstQualified() ||
+if ((Form != Load && Form != LoadCopy && AtomTy.isConstQualified()) ||
 AtomTy.getAddressSpace() == LangAS::opencl_constant) {
   Diag(DRE->getLocStart(), diag::err_atomic_op_needs_non_const_atomic)
   << (AtomTy.isConstQualified() ? 0 : 1) << Ptr->getType()


Index: test/SemaOpenCL/atomic-ops.cl
===
--- test/SemaOpenCL/atomic-ops.cl
+++ test/SemaOpenCL/atomic-ops.cl
@@ -58,7 +58,8 @@
   __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-const 

[PATCH] D47618: __c11_atomic_load's _Atomic can be const

[JF Bastien via Phabricator via cfe-commits]

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

Give the comments, I think this is ready to commit.


Repository:
  rC Clang

https://reviews.llvm.org/D47618



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

[PATCH] D44671: [libcxx] Enable static libcxxabi linking on Darwin

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Were the concerns expressed in https://reviews.llvm.org/D8017 addressed?


Repository:
  rCXX libc++

https://reviews.llvm.org/D44671



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

[PATCH] D45639: [Driver] Support default libc++ library location on Darwin

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In https://reviews.llvm.org/D45639#1192849, @beanz wrote:

> Adding @jfb since this is his domain now too.


@ldionne is the libc++ expert :-)


Repository:
  rC Clang

https://reviews.llvm.org/D45639



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

[PATCH] D50361: [NFC] Test automatic variable initialization

[JF Bastien via Phabricator via cfe-commits]

jfb created this revision.
Herald added subscribers: cfe-commits, dexonsmith.

r337887 started using memset for automatic variable initialization where 
sensible. A follow-up discussion leads me to believe that we should better test 
automatic variable initialization, and that there are probably follow-up 
patches in clang and LLVM to improve codegen. It’ll be important to measure -O0 
compile time, and figure out which transforms should be in the frontend versus 
the backend.

This patch is just a test of the current behavior, no questions asked. 
Follow-up patches will tune the code generation.

rdar://problem/42981573


Repository:
  rC Clang

https://reviews.llvm.org/D50361

Files:
  test/CodeGenCXX/auto-var-init.cpp

Index: test/CodeGenCXX/auto-var-init.cpp
===
--- /dev/null
+++ test/CodeGenCXX/auto-var-init.cpp
@@ -0,0 +1,999 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fblocks -ftrivial-auto-var-init=pattern %s -emit-llvm -o - | FileCheck %s
+
+template void used(T &) noexcept;
+
+#define TEST_UNINIT(NAME, TYPE) \
+  using type_##NAME = TYPE; \
+  void test_##NAME##_uninit() { \
+type_##NAME uninit; \
+used(uninit);   \
+  }
+
+// Value initialization on scalars, aggregate initialization on aggregates.
+#define TEST_BRACES(NAME, TYPE) \
+  using type_##NAME = TYPE; \
+  void test_##NAME##_braces() { \
+type_##NAME braces = {};\
+used(braces);   \
+  }
+
+#define TEST_CUSTOM(NAME, TYPE, ...)\
+  using type_##NAME = TYPE; \
+  void test_##NAME##_custom() { \
+type_##NAME custom __VA_ARGS__; \
+used(custom);   \
+  }
+
+struct empty {};
+struct small { char c; };
+struct smallinit { char c = 42; };
+struct smallpartinit { char c = 42, d; };
+struct nullinit { char* null = nullptr; };
+struct padded { char c; int i; };
+struct paddednullinit { char c = 0; int i = 0; };
+struct bitfield { int i : 4; int j : 2; };
+struct bitfieldaligned { int i : 4; int : 0; int j : 2; };
+struct big { unsigned a, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p, q, r, s, t, u, v, w, x, y, z; };
+struct arraytail { int i; int arr[]; };
+struct tailpad { short s; char c; };
+struct notlockfree { long long a[4]; };
+struct semivolatile { int i; volatile int vi; };
+struct semivolatileinit { int i = 0x; volatile int vi = 0x; };
+struct base { virtual ~base(); };
+struct derived : public base {};
+struct virtualderived : public virtual base, public virtual derived {};
+union matching { int i; float f; };
+union matchingreverse { float f; int i; };
+union unmatched { char c; int i; };
+union unmatchedreverse { int i; char c; };
+union unmatchedfp { float f; double d; };
+enum emptyenum {};
+enum smallenum { VALUE };
+
+extern "C" {
+
+TEST_UNINIT(char, char);
+// CHECK-LABEL: @test_char_uninit()
+// CHECK:   %uninit = alloca i8, align 1
+// CHECK-NEXT:  call void @{{.*}}used{{.*}}%uninit)
+
+TEST_BRACES(char, char);
+// CHECK-LABEL: @test_char_braces()
+// CHECK:   %braces = alloca i8, align 1
+// CHECK-NEXT:  store i8 0, i8* %braces, align 1
+// CHECK-NEXT:  call void @{{.*}}used{{.*}}%braces)
+
+TEST_UNINIT(uchar, unsigned char);
+// CHECK-LABEL: @test_uchar_uninit()
+// CHECK:   %uninit = alloca i8, align 1
+// CHECK-NEXT:  call void @{{.*}}used{{.*}}%uninit)
+
+TEST_BRACES(uchar, unsigned char);
+// CHECK-LABEL: @test_uchar_braces()
+// CHECK:   %braces = alloca i8, align 1
+// CHECK-NEXT:  store i8 0, i8* %braces, align 1
+// CHECK-NEXT:  call void @{{.*}}used{{.*}}%braces)
+
+TEST_UNINIT(schar, signed char);
+// CHECK-LABEL: @test_schar_uninit()
+// CHECK:   %uninit = alloca i8, align 1
+// CHECK-NEXT:  call void @{{.*}}used{{.*}}%uninit)
+
+TEST_BRACES(schar, signed char);
+// CHECK-LABEL: @test_schar_braces()
+// CHECK:   %braces = alloca i8, align 1
+// CHECK-NEXT:  store i8 0, i8* %braces, align 1
+// CHECK-NEXT:  call void @{{.*}}used{{.*}}%braces)
+
+TEST_UNINIT(wchar_t, wchar_t);
+// CHECK-LABEL: @test_wchar_t_uninit()
+// CHECK:   %uninit = alloca i32, align 4
+// CHECK-NEXT:  call void @{{.*}}used{{.*}}%uninit)
+
+TEST_BRACES(wchar_t, wchar_t);
+// CHECK-LABEL: @test_wchar_t_braces()
+// CHECK:   %braces = alloca i32, align 4
+// CHECK-NEXT:  store i32 0, i32* %braces, align 4
+// CHECK-NEXT:  call void @{{.*}}used{{.*}}%braces)
+
+TEST_UNINIT(short, short);
+// CHECK-LABEL: @test_short_uninit()
+// CHECK:   %uninit = alloca i16, align 2
+// CHECK-NEXT:  call void @{{.*}}used{{.*}}%uninit)
+
+TEST_BRACES(short, short);
+// CHECK-LABEL: @test_short_braces()
+// CHECK:   %braces = alloca i16, align 2
+// CHECK-NEXT:  store i16 0, i16* %braces, align 2
+// CHECK-NEXT:  call void 

[PATCH] D50361: [NFC] Test automatic variable initialization

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

I'm honestly not sure there's anything to review here since it's just showing 
us what the current behavior is. LMK if I'm not testing something that I should.

I'd much rather test current behavior as one patch first, because then the 
follow-ups show a clear before / after diff instead of leaving you guessing as 
to what the previous behavior was.


Repository:
  rC Clang

https://reviews.llvm.org/D50361



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

[PATCH] D50361: [NFC] Test automatic variable initialization

[JF Bastien via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC339089: [NFC] Test automatic variable initialization 
(authored by jfb, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D50361?vs=159397=159448#toc

Repository:
  rC Clang

https://reviews.llvm.org/D50361

Files:
  test/CodeGenCXX/auto-var-init.cpp

Index: test/CodeGenCXX/auto-var-init.cpp
===
--- test/CodeGenCXX/auto-var-init.cpp
+++ test/CodeGenCXX/auto-var-init.cpp
@@ -0,0 +1,999 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fblocks -ftrivial-auto-var-init=pattern %s -emit-llvm -o - | FileCheck %s
+
+template void used(T &) noexcept;
+
+#define TEST_UNINIT(NAME, TYPE) \
+  using type_##NAME = TYPE; \
+  void test_##NAME##_uninit() { \
+type_##NAME uninit; \
+used(uninit);   \
+  }
+
+// Value initialization on scalars, aggregate initialization on aggregates.
+#define TEST_BRACES(NAME, TYPE) \
+  using type_##NAME = TYPE; \
+  void test_##NAME##_braces() { \
+type_##NAME braces = {};\
+used(braces);   \
+  }
+
+#define TEST_CUSTOM(NAME, TYPE, ...)\
+  using type_##NAME = TYPE; \
+  void test_##NAME##_custom() { \
+type_##NAME custom __VA_ARGS__; \
+used(custom);   \
+  }
+
+struct empty {};
+struct small { char c; };
+struct smallinit { char c = 42; };
+struct smallpartinit { char c = 42, d; };
+struct nullinit { char* null = nullptr; };
+struct padded { char c; int i; };
+struct paddednullinit { char c = 0; int i = 0; };
+struct bitfield { int i : 4; int j : 2; };
+struct bitfieldaligned { int i : 4; int : 0; int j : 2; };
+struct big { unsigned a, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p, q, r, s, t, u, v, w, x, y, z; };
+struct arraytail { int i; int arr[]; };
+struct tailpad { short s; char c; };
+struct notlockfree { long long a[4]; };
+struct semivolatile { int i; volatile int vi; };
+struct semivolatileinit { int i = 0x; volatile int vi = 0x; };
+struct base { virtual ~base(); };
+struct derived : public base {};
+struct virtualderived : public virtual base, public virtual derived {};
+union matching { int i; float f; };
+union matchingreverse { float f; int i; };
+union unmatched { char c; int i; };
+union unmatchedreverse { int i; char c; };
+union unmatchedfp { float f; double d; };
+enum emptyenum {};
+enum smallenum { VALUE };
+
+extern "C" {
+
+TEST_UNINIT(char, char);
+// CHECK-LABEL: @test_char_uninit()
+// CHECK:   %uninit = alloca i8, align 1
+// CHECK-NEXT:  call void @{{.*}}used{{.*}}%uninit)
+
+TEST_BRACES(char, char);
+// CHECK-LABEL: @test_char_braces()
+// CHECK:   %braces = alloca i8, align 1
+// CHECK-NEXT:  store i8 0, i8* %braces, align 1
+// CHECK-NEXT:  call void @{{.*}}used{{.*}}%braces)
+
+TEST_UNINIT(uchar, unsigned char);
+// CHECK-LABEL: @test_uchar_uninit()
+// CHECK:   %uninit = alloca i8, align 1
+// CHECK-NEXT:  call void @{{.*}}used{{.*}}%uninit)
+
+TEST_BRACES(uchar, unsigned char);
+// CHECK-LABEL: @test_uchar_braces()
+// CHECK:   %braces = alloca i8, align 1
+// CHECK-NEXT:  store i8 0, i8* %braces, align 1
+// CHECK-NEXT:  call void @{{.*}}used{{.*}}%braces)
+
+TEST_UNINIT(schar, signed char);
+// CHECK-LABEL: @test_schar_uninit()
+// CHECK:   %uninit = alloca i8, align 1
+// CHECK-NEXT:  call void @{{.*}}used{{.*}}%uninit)
+
+TEST_BRACES(schar, signed char);
+// CHECK-LABEL: @test_schar_braces()
+// CHECK:   %braces = alloca i8, align 1
+// CHECK-NEXT:  store i8 0, i8* %braces, align 1
+// CHECK-NEXT:  call void @{{.*}}used{{.*}}%braces)
+
+TEST_UNINIT(wchar_t, wchar_t);
+// CHECK-LABEL: @test_wchar_t_uninit()
+// CHECK:   %uninit = alloca i32, align 4
+// CHECK-NEXT:  call void @{{.*}}used{{.*}}%uninit)
+
+TEST_BRACES(wchar_t, wchar_t);
+// CHECK-LABEL: @test_wchar_t_braces()
+// CHECK:   %braces = alloca i32, align 4
+// CHECK-NEXT:  store i32 0, i32* %braces, align 4
+// CHECK-NEXT:  call void @{{.*}}used{{.*}}%braces)
+
+TEST_UNINIT(short, short);
+// CHECK-LABEL: @test_short_uninit()
+// CHECK:   %uninit = alloca i16, align 2
+// CHECK-NEXT:  call void @{{.*}}used{{.*}}%uninit)
+
+TEST_BRACES(short, short);
+// CHECK-LABEL: @test_short_braces()
+// CHECK:   %braces = alloca i16, align 2
+// CHECK-NEXT:  store i16 0, i16* %braces, align 2
+// CHECK-NEXT:  call void @{{.*}}used{{.*}}%braces)
+
+TEST_UNINIT(ushort, unsigned short);
+// CHECK-LABEL: @test_ushort_uninit()
+// CHECK:   %uninit = alloca i16, align 2
+// CHECK-NEXT:  call void @{{.*}}used{{.*}}%uninit)
+
+TEST_BRACES(ushort, unsigned short);
+// CHECK-LABEL: @test_ushort_braces()
+// CHECK:   %braces = alloca i16, align 2
+// 

[PATCH] D50361: [NFC] Test automatic variable initialization

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Further fixes in r339090 and r339093.


Repository:
  rC Clang

https://reviews.llvm.org/D50361



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

[PATCH] D50719: [libc++] Fix incorrect definition of TEST_HAS_C11_FEATURES

[JF Bastien via Phabricator via cfe-commits]

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

From `__ISO_C_VISIBLE >= 2011` it looks like this tries to test C11 features 
regardless of the C++ version. That's probably fine, but we're walking this 
line where only C++17 really guarantees C11 support, so this entire thing is 
weird and brittle to me. You're not changing the brittle-ness, just removing 
one kink, so from that perspective this change LGTM.




Comment at: libcxx/test/support/test_macros.h:127
 // Note that at this time (July 2018), MacOS X and iOS do NOT.
-#if __ISO_C_VISIBLE >= 2011
+#if __ISO_C_VISIBLE >= 2011 || __cplusplus >= 201103L
 #  if defined(__FreeBSD__)

Should this be `TEST_STD_VER >= 11`?


Repository:
  rCXX libc++

https://reviews.llvm.org/D50719



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

[PATCH] D50549: [libcxx] [test] Repair thread unsafety in thread tests

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

CC some sanitizer folks.


https://reviews.llvm.org/D50549



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

[PATCH] D50994: Add a new flag and attributes to control static destructor registration

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: clang/include/clang/AST/Decl.h:1472
 
+  /// Do we need to emit an exit-time destructor for this variable?
+  bool isNoDestroy(const ASTContext &) const;

This is only valid for static variables, right? It's probably better to 
document the function name that way, e.g. `isStaticWithNoDestroy`.



Comment at: clang/include/clang/Basic/Attr.td:2999
+
+def NoDestroy : InheritableAttr {
+  let Spellings = [Clang<"no_destroy", 0>];

Ditto for these, I think the names of the variables should clarify that it's 
only for `static`. Note that I wouldn't rename the attributes themselves! Usage 
in context is unambiguous, and the docs should be clear. I just think the 
variable names don't provide the required context.



Comment at: clang/include/clang/Basic/AttrDocs.td:3494
+The ``always_destroy`` attribute specifies that a variable with static or 
thread
+storage duration should have it's exit-time destructor run. This attribute does
+nothing unless clang was invoked with -fno-c++-static-destructors.

"its"



Comment at: clang/include/clang/Basic/AttrDocs.td:3495
+storage duration should have it's exit-time destructor run. This attribute does
+nothing unless clang was invoked with -fno-c++-static-destructors.
+  }];

"does nothing" is more "is the default".



Comment at: clang/include/clang/Basic/AttrDocs.td:3503
+The ``no_destroy`` attribute specifies that a variable with static or thread
+storage duration shouldn't have it's exit-time destructor run. Annotating every
+static or thread duration variable with this attribute is equalivant to 
invoking

"its"



Comment at: clang/include/clang/Basic/AttrDocs.td:3504
+storage duration shouldn't have it's exit-time destructor run. Annotating every
+static or thread duration variable with this attribute is equalivant to 
invoking
+clang with -fno-c++-static-destructors.

"static and thread"



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5931
+handleSimpleAttributeWithExclusions(S, 
D, A);
+  }
+}

This allows a variable with both attributes :)
Looking at the code above it seems like having both attributes is equivalent to 
no-destroy.


Repository:
  rC Clang

https://reviews.llvm.org/D50994



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

[PATCH] D50994: Add a new flag and attributes to control static destructor registration

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: clang/include/clang/AST/Decl.h:1472
 
+  /// Do we need to emit an exit-time destructor for this variable?
+  bool isNoDestroy(const ASTContext &) const;

rsmith wrote:
> jfb wrote:
> > This is only valid for static variables, right? It's probably better to 
> > document the function name that way, e.g. `isStaticWithNoDestroy`.
> I think the question (and hence current function name) is meaningful for any 
> variable, but it just happens that the answer will always be "no" for 
> non-static variables (for now, at least). Are you concerned that people will 
> think calls to this function are missing from codepaths that only deal with 
> automatic storage duration variables with the current name?
Yeah it seems like "this variable has no destructor". I guess even trivial 
automatic variables have a (trivial) destructor, so maybe it's not ambiguous? 
Up to y'all :)


Repository:
  rC Clang

https://reviews.llvm.org/D50994



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

[PATCH] D50994: Add a new flag and attributes to control static destructor registration

[JF Bastien via Phabricator via cfe-commits]

jfb accepted this revision.
jfb added inline comments.



Comment at: clang/include/clang/AST/Decl.h:1472
 
+  /// Do we need to emit an exit-time destructor for this variable?
+  bool isNoDestroy(const ASTContext &) const;

erik.pilkington wrote:
> jfb wrote:
> > rsmith wrote:
> > > jfb wrote:
> > > > This is only valid for static variables, right? It's probably better to 
> > > > document the function name that way, e.g. `isStaticWithNoDestroy`.
> > > I think the question (and hence current function name) is meaningful for 
> > > any variable, but it just happens that the answer will always be "no" for 
> > > non-static variables (for now, at least). Are you concerned that people 
> > > will think calls to this function are missing from codepaths that only 
> > > deal with automatic storage duration variables with the current name?
> > Yeah it seems like "this variable has no destructor". I guess even trivial 
> > automatic variables have a (trivial) destructor, so maybe it's not 
> > ambiguous? Up to y'all :)
> If anyone has a strong preference I'd be happy either way!
Considering what Richard pointed out below w.r.t. naming convention, this is 
good with me.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5931
+handleSimpleAttributeWithExclusions(S, 
D, A);
+  }
+}

erik.pilkington wrote:
> jfb wrote:
> > This allows a variable with both attributes :)
> > Looking at the code above it seems like having both attributes is 
> > equivalent to no-destroy.
> `handleSimpleAttributeWithExclusions` handles that automagically, i.e.:
> ```
> $ cat t.cpp
> [[clang::no_destroy]] [[clang::always_destroy]] int x;
> $ ~/dbg-bld/bin/clang t.cpp
> t.cpp:1:25: error: 'always_destroy' and 'no_destroy' attributes are not 
> compatible
> [[clang::no_destroy]] [[clang::always_destroy]] int x;
> ^
> t.cpp:1:3: note: conflicting attribute is here
> [[clang::no_destroy]] [[clang::always_destroy]] int x;
>   ^
> 1 error generated.
> ```
> I'll add a test to prove this though...
Ah nice!


https://reviews.llvm.org/D50994



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

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Isn't this duplicating code in lib/Support/Unix/Threading.inc with a different 
implementation?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50993



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

[PATCH] D51084: Implement -Watomic-implicit-seq-cst

[JF Bastien via Phabricator via cfe-commits]

jfb created this revision.
jfb added a reviewer: rjmccall.
Herald added subscribers: cfe-commits, dexonsmith.

_Atomic and __sync_* operations are implicitly sequentially-consistent. Some
codebases want to force explicit usage of memory order instead. This warning
allows them to know where implicit sequentially-consistent memory order is used.
The warning isn't on by default because _Atomic was purposefully designed to
have seq_cst as the default: the idea was that it's the right thing to use most
of the time. This warning allows developers who disagree to enforce explicit
usage instead.

A follow-up patch will take care of C++'s std::atomic. It'll be different enough
from this patch that I think it should be separate: for C++ the atomic
operations all have a memory order parameter (or two), but it's defaulted. I
believe this warning should trigger when the default is used, but not when
seq_cst is used explicitly (or implicitly as the failure order for cmpxchg).

rdar://problem/28172966


Repository:
  rC Clang

https://reviews.llvm.org/D51084

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaChecking.cpp
  test/Sema/atomic-implicit-seq_cst.c
  test/Sema/sync-implicit-seq_cst.c

Index: test/Sema/sync-implicit-seq_cst.c
===
--- /dev/null
+++ test/Sema/sync-implicit-seq_cst.c
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 %s -verify -ffreestanding -fsyntax-only -triple=i686-linux-gnu -std=c11 -Watomic-implicit-seq-cst -Wno-sync-fetch-and-nand-semantics-changed
+
+// __sync_* operations are implicitly sequentially-consistent. Some codebases
+// want to force explicit usage of memory order instead.
+
+void fetch_and_add(int *ptr, int val) { __sync_fetch_and_add(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void fetch_and_sub(int *ptr, int val) { __sync_fetch_and_sub(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void fetch_and_or(int *ptr, int val) { __sync_fetch_and_or(ptr, val); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void fetch_and_and(int *ptr, int val) { __sync_fetch_and_and(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void fetch_and_xor(int *ptr, int val) { __sync_fetch_and_xor(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void fetch_and_nand(int *ptr, int val) { __sync_fetch_and_nand(ptr, val); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+
+void add_and_fetch(int *ptr, int val) { __sync_add_and_fetch(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void sub_and_fetch(int *ptr, int val) { __sync_sub_and_fetch(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void or_and_fetch(int *ptr, int val) { __sync_or_and_fetch(ptr, val); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void and_and_fetch(int *ptr, int val) { __sync_and_and_fetch(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void xor_and_fetch(int *ptr, int val) { __sync_xor_and_fetch(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void nand_and_fetch(int *ptr, int val) { __sync_nand_and_fetch(ptr, val); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+
+void bool_compare_and_swap(int *ptr, int oldval, int newval) { __sync_bool_compare_and_swap(ptr, oldval, newval); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void val_compare_and_swap(int *ptr, int oldval, int newval) { __sync_val_compare_and_swap(ptr, oldval, newval); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+
+void synchronize(void) { __sync_synchronize(); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+
+void lock_test_and_set(int *ptr, int val) { __sync_lock_test_and_set(ptr, val); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void lock_release(int *ptr) { __sync_lock_release(ptr); }  

[PATCH] D51170: [libc++] Remove race condition in std::async

[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: libcxx/include/future:556
 bool __has_value() const
 {return (__state_ & __constructed) || (__exception_ != nullptr);}
 

I'm not auditing everything, but it seems like code above can still access 
__state_ without holding __mut_? Like in the dtor.

Generally this patch lgtm because it's a step forward, but maybe we should 
separately refactor the code to make it so that accesses to __state_ require 
passing in a reference to lock_guard to show we actually hold __mut_. It would 
ignore that reference, but that's a way to enforce, in the type system, that 
__state_ is only touched when the lock is held.

WDYT?


Repository:
  rCXX libc++

https://reviews.llvm.org/D51170



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

[PATCH] D51084: Implement -Watomic-implicit-seq-cst

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Updated.


Repository:
  rC Clang

https://reviews.llvm.org/D51084



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

[PATCH] D51084: Implement -Watomic-implicit-seq-cst

[JF Bastien via Phabricator via cfe-commits]

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

- Address John's comments: diagnose at beginning, and don't check isIgnored 
manually.


Repository:
  rC Clang

https://reviews.llvm.org/D51084

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaChecking.cpp
  test/Sema/atomic-implicit-seq_cst.c
  test/Sema/sync-implicit-seq_cst.c

Index: test/Sema/sync-implicit-seq_cst.c
===
--- /dev/null
+++ test/Sema/sync-implicit-seq_cst.c
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 %s -verify -ffreestanding -fsyntax-only -triple=i686-linux-gnu -std=c11 -Watomic-implicit-seq-cst -Wno-sync-fetch-and-nand-semantics-changed
+
+// __sync_* operations are implicitly sequentially-consistent. Some codebases
+// want to force explicit usage of memory order instead.
+
+void fetch_and_add(int *ptr, int val) { __sync_fetch_and_add(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void fetch_and_sub(int *ptr, int val) { __sync_fetch_and_sub(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void fetch_and_or(int *ptr, int val) { __sync_fetch_and_or(ptr, val); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void fetch_and_and(int *ptr, int val) { __sync_fetch_and_and(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void fetch_and_xor(int *ptr, int val) { __sync_fetch_and_xor(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void fetch_and_nand(int *ptr, int val) { __sync_fetch_and_nand(ptr, val); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+
+void add_and_fetch(int *ptr, int val) { __sync_add_and_fetch(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void sub_and_fetch(int *ptr, int val) { __sync_sub_and_fetch(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void or_and_fetch(int *ptr, int val) { __sync_or_and_fetch(ptr, val); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void and_and_fetch(int *ptr, int val) { __sync_and_and_fetch(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void xor_and_fetch(int *ptr, int val) { __sync_xor_and_fetch(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void nand_and_fetch(int *ptr, int val) { __sync_nand_and_fetch(ptr, val); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+
+void bool_compare_and_swap(int *ptr, int oldval, int newval) { __sync_bool_compare_and_swap(ptr, oldval, newval); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void val_compare_and_swap(int *ptr, int oldval, int newval) { __sync_val_compare_and_swap(ptr, oldval, newval); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+
+void synchronize(void) { __sync_synchronize(); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+
+void lock_test_and_set(int *ptr, int val) { __sync_lock_test_and_set(ptr, val); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void lock_release(int *ptr) { __sync_lock_release(ptr); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
Index: test/Sema/atomic-implicit-seq_cst.c
===
--- /dev/null
+++ test/Sema/atomic-implicit-seq_cst.c
@@ -0,0 +1,319 @@
+// RUN: %clang_cc1 %s -verify -ffreestanding -fsyntax-only -triple=i686-linux-gnu -std=c11 -Watomic-implicit-seq-cst
+
+// _Atomic operations are implicitly sequentially-consistent. Some codebases
+// want to force explicit usage of memory order instead.
+
+_Atomic(int) atom;
+void gimme_int(int);
+
+void bad_pre_inc(void) {
+  ++atom; // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+}
+
+void bad_pre_dec(void) {
+  

[PATCH] D51084: Implement -Watomic-implicit-seq-cst

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: lib/Sema/SemaChecking.cpp:10668
+  if (Source->isAtomicType() || Target->isAtomicType())
+S.Diag(E->getBeginLoc(), diag::warn_atomic_implicit_seq_cst);
+

rjmccall wrote:
> jfb wrote:
> > rjmccall wrote:
> > > Why would the target be an atomic type?  And if it is an atomic type, 
> > > isn't that an initialization of a temporary?  In what situation does it 
> > > make sense to order the initialization of a temporary?
> > In this case:
> > 
> > ```
> > void bad_assign_1(int i) {
> >   atom = i; // expected-warning {{implicit use of sequentially-consistent 
> > atomic may incur stronger memory barriers than necessary}}
> > }
> > ```
> > 
> > We want to warn on assignment to atomics being implicitly `seq_cst`.
> > 
> > Though you're right, initialization shouldn't warn because it isn't atomic. 
> > The issue is that `ATOMIC_VAR_INIT` is what needs to get used, and that's 
> > weird to test. I'll add a test that just assigns (which is what 
> > `ATOMIC_VAR_INIT` expands to for clang), and I'll need to update the code 
> > to detect that pattern and avoid warning in that case. I guess I have to 
> > look at the `Expr` and figure out if the LHS is a `Decl` or something like 
> > that.
> Do we really implicitly convert the RHS of that assignment to atomic type?  
> That seems wrong; `_Atomic` is really a type qualifier, and the RHS should 
> not be converted to `_Atomic(T)` any more than it would be converted to 
> `volatile T` for an assignment into a `volatile` l-value.
I don't make the rules man! I just enforce (and try to warn) on them!

C17:

> 6.5.16.1 Simple assignment
> **Constraints**
> One of the following shall hold: 114)
> — the left operand has atomic, qualified, or unqualified arithmetic type, and 
> the right has arithmetic type;
> — the left operand has an atomic, qualified, or unqualified version of a 
> structure or union type compatible with the type of the right;
> — the left operand has atomic, qualified, or unqualified pointer type, and 
> (considering the type the left operand would have after lvalue conversion) 
> both operands are pointers to qualified or unqualified versions of compatible 
> types, and the type pointed to by the left has all the qualifiers of the type 
> pointed to by the right;
> — the left operand has atomic, qualified, or unqualified pointer type, and 
> (considering the type the left operand would have after lvalue conversion) 
> one operand is a pointer to an object type, and the other is a pointer to a 
> qualified or unqualified version of void, and the type pointed to by the left 
> has all the qualifiers of the type pointed to by the right;
> — the left operand is an atomic, qualified, or unqualified pointer, and the 
> right is a null pointer constant; or
> — the left operand has type atomic, qualified, or unqualified _Bool, and the 
> right is a pointer.
>
> **Semantics**
> In simple assignment (=), the value of the right operand is converted to the 
> type of the assignment expression and replaces the value stored in the object 
> designated by the left operand.
>
> 114)The asymmetric appearance of these constraints with respect to type 
> qualifiers is due to the conversion (specified in 6.3.2.1) that changes 
> lvalues to "the value of the expression" and thus removes any type qualifiers 
> that were applied to the 
> typecategoryoftheexpression(forexample,itremovesconstbutnotvolatilefromthetypeint
>  volatile * const).


Repository:
  rC Clang

https://reviews.llvm.org/D51084



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

[PATCH] D51084: Implement -Watomic-implicit-seq-cst

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: lib/Sema/SemaChecking.cpp:10668
+  if (Source->isAtomicType() || Target->isAtomicType())
+S.Diag(E->getBeginLoc(), diag::warn_atomic_implicit_seq_cst);
+

rjmccall wrote:
> Why would the target be an atomic type?  And if it is an atomic type, isn't 
> that an initialization of a temporary?  In what situation does it make sense 
> to order the initialization of a temporary?
In this case:

```
void bad_assign_1(int i) {
  atom = i; // expected-warning {{implicit use of sequentially-consistent 
atomic may incur stronger memory barriers than necessary}}
}
```

We want to warn on assignment to atomics being implicitly `seq_cst`.

Though you're right, initialization shouldn't warn because it isn't atomic. The 
issue is that `ATOMIC_VAR_INIT` is what needs to get used, and that's weird to 
test. I'll add a test that just assigns (which is what `ATOMIC_VAR_INIT` 
expands to for clang), and I'll need to update the code to detect that pattern 
and avoid warning in that case. I guess I have to look at the `Expr` and figure 
out if the LHS is a `Decl` or something like that.


Repository:
  rC Clang

https://reviews.llvm.org/D51084



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

[PATCH] D51314: Parse compile commands lazily in InterpolatingCompilationDatabase

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:124
+// A CompileCommand that can be applied to another file. Any instance of this
+// object is invalid after std::move() from it.
 struct TransferableCommand {

This comment about `move` isn't really saying anything. Also, it's valid but 
unspecified (in the case of STL things). I'd drop it.



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:128
+  : OriginalCmd(std::move(C)),
+TraitsComputed(llvm::make_unique()) {}
 

The `once_flag` should just be a static, don't allocate it.


Repository:
  rC Clang

https://reviews.llvm.org/D51314



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

[PATCH] D51314: Parse compile commands lazily in InterpolatingCompilationDatabase

[JF Bastien via Phabricator via cfe-commits]

jfb requested changes to this revision.
jfb added inline comments.
This revision now requires changes to proceed.



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:201
+
+  CommandTraits computeTraits() const {
+CommandTraits Result;

It's not clear to me that this entire function is safe to call from multiple 
threads at the same time. Even if it's safe now, I'm willing to bet it won't 
always be that way. `getTraits` should therefore use a magic static or 
`call_once` and avoid the headache entirely.



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:258
+  /// Null before Traits were computed, non-null otherwise. All accesses to 
this
+  /// must be atomic.
+  mutable std::shared_ptr Traits;

The comment should say why accesses need to be atomic. Or better yet, this 
should only be usable "the right way".


Repository:
  rC Clang

https://reviews.llvm.org/D51314



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

[PATCH] D47618: __c11_atomic_load's _Atomic can be const

[JF Bastien via Phabricator via cfe-commits]

jfb added a subscriber: yaxunl.
jfb added inline comments.



Comment at: lib/Sema/SemaChecking.cpp:3361
+if ((Form != Load && Form != LoadCopy && AtomTy.isConstQualified()) ||
 AtomTy.getAddressSpace() == LangAS::opencl_constant) {
   Diag(DRE->getLocStart(), diag::err_atomic_op_needs_non_const_atomic)

rsmith wrote:
> We also need to figure out what to do about this -- should an atomic load 
> from a constant address space be valid? (It seems a little pointless to use 
> an *atomic* load here, but not obviously wrong.)
I think it's mostly harmless except when the address space is actually constant 
(not C++ constant) because in these cases a wide atomic might need cmpxchg to 
perform a read, and that will fail writing. Maybe @Anastasia can chime in for 
OpenCL?



Comment at: lib/Sema/SemaChecking.cpp:3368-3374
   } else if (Form != Load && Form != LoadCopy) {
 if (ValType.isConstQualified()) {
   Diag(DRE->getLocStart(), diag::err_atomic_op_needs_non_const_pointer)
 << Ptr->getType() << Ptr->getSourceRange();
   return ExprError();
 }
   }

rsmith wrote:
> It would be a little nicer to change this `else if` to a plain `if` and 
> conditionalize the diagnostic instead.
> 
> Can you track down whoever added the address space check to the C11 atomic 
> path and ask them if they really meant for it to not apply to the GNU atomic 
> builtins?
It was @yaxunl in https://reviews.llvm.org/D28691
Nobody asked about GNU atomic builtins with OpenCL in that review. I'll let 
@yaxunl chime in.


Repository:
  rC Clang

https://reviews.llvm.org/D47618



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

[PATCH] D49458: Support implicit _Atomic struct load / store

[JF Bastien via Phabricator via cfe-commits]

jfb created this revision.
jfb added a reviewer: dexonsmith.
Herald added a subscriber: cfe-commits.

Using _Atomic to do implicit load / store is just a seq_cst atomic_load / 
atomic_store. Stores currently assert in Sema::ImpCastExprToType with 'can't 
implicitly cast lvalue to rvalue with this cast kind', but that's erroneous. 
The codegen is fine as the test shows.

While investigating I found that Richard had found the problem here: 
https://reviews.llvm.org/D46112#1113557

rdar://problem/40347123


Repository:
  rC Clang

https://reviews.llvm.org/D49458

Files:
  lib/Sema/Sema.cpp
  test/CodeGen/atomic-ops.c


Index: test/CodeGen/atomic-ops.c
===
--- test/CodeGen/atomic-ops.c
+++ test/CodeGen/atomic-ops.c
@@ -183,6 +183,18 @@
   double x;
 };
 
+void implicit_store(_Atomic(struct S) *a, struct S s) {
+  // CHECK-LABEL: @implicit_store(
+  // CHECK: store atomic i64 %{{.*}}, i64* %{{.*}} seq_cst, align 8
+  *a = s;
+}
+
+struct S implicit_load(_Atomic(struct S) *a) {
+  // CHECK-LABEL: @implicit_load(
+  // CHECK: load atomic i64, i64* %{{.*}} seq_cst, align 8
+  return *a;
+}
+
 struct S fd1(struct S *a) {
   // CHECK-LABEL: @fd1
   // CHECK: [[RETVAL:%.*]] = alloca %struct.S, align 4
Index: lib/Sema/Sema.cpp
===
--- lib/Sema/Sema.cpp
+++ lib/Sema/Sema.cpp
@@ -481,6 +481,7 @@
 case CK_ArrayToPointerDecay:
 case CK_FunctionToPointerDecay:
 case CK_ToVoid:
+case CK_NonAtomicToAtomic:
   break;
 }
   }


Index: test/CodeGen/atomic-ops.c
===
--- test/CodeGen/atomic-ops.c
+++ test/CodeGen/atomic-ops.c
@@ -183,6 +183,18 @@
   double x;
 };
 
+void implicit_store(_Atomic(struct S) *a, struct S s) {
+  // CHECK-LABEL: @implicit_store(
+  // CHECK: store atomic i64 %{{.*}}, i64* %{{.*}} seq_cst, align 8
+  *a = s;
+}
+
+struct S implicit_load(_Atomic(struct S) *a) {
+  // CHECK-LABEL: @implicit_load(
+  // CHECK: load atomic i64, i64* %{{.*}} seq_cst, align 8
+  return *a;
+}
+
 struct S fd1(struct S *a) {
   // CHECK-LABEL: @fd1
   // CHECK: [[RETVAL:%.*]] = alloca %struct.S, align 4
Index: lib/Sema/Sema.cpp
===
--- lib/Sema/Sema.cpp
+++ lib/Sema/Sema.cpp
@@ -481,6 +481,7 @@
 case CK_ArrayToPointerDecay:
 case CK_FunctionToPointerDecay:
 case CK_ToVoid:
+case CK_NonAtomicToAtomic:
   break;
 }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46112: Allow _Atomic to be specified on incomplete types

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In https://reviews.llvm.org/D46112#1113557, @aaron.ballman wrote:

> In https://reviews.llvm.org/D46112#1098243, @rsmith wrote:
>
> > In https://reviews.llvm.org/D46112#1096893, @aaron.ballman wrote:
> >
> > > In https://reviews.llvm.org/D46112#1091981, @efriedma wrote:
> > >
> > > > Also needs some test coverage for atomic operations which aren't calls, 
> > > > like "typedef struct S S; void f(_Atomic S *s, _Atomic S *s2) { *s = 
> > > > *s2; };".
> > >
> > >
> > > Thank you for pointing this out -- that uncovered an issue where we were 
> > > not properly diagnosing the types as being incomplete. I've added a new 
> > > test case and rolled the contents of Sema\atomic-type.cpp (which I added 
> > > in an earlier patch) into SemaCXX\atomic-type.cpp (which already existed 
> > > and I missed it).
> >
> >
> > I don't think this has sufficiently addressed the comment. Specifically, 
> > for a case like:
> >
> >   struct NotTriviallyCopyable { NotTriviallyCopyable(const 
> > NotTriviallyCopyable&); };
> >   void f(_Atomic NotTriviallyCopyable *p) { *p = *p; }
> >
> >
> > ... we should reject the assignment, because it would perform an atomic 
> > operation on a non-trivially-copyable type. It would probably suffice to 
> > check the places where we form an `AtomicToNonAtomic` or 
> > `NonAtomicToAtomic` conversion.
> >
> > In passing, I noticed that this crashes Clang (because we fail to create an 
> > lvalue-to-rvalue conversion for `x`):
> >
> >   struct X {};  
> >   void f(_Atomic X *p, X x) { *p = x; }
> >
> >
> > This will likely get in the way of your test cases unless you fix it :)
>
>
> It only gets in the way for C++ whereas my primary concern for this patch is 
> C. I tried spending a few hours on this today and got nowhere -- we have a 
> lot of FIXME comments surrounding atomic type qualifiers in C++. I've run out 
> of time to be able to work on this patch and may have to abandon it. I'd hate 
> to lose the forward progress already made, so I'm wondering if the C bits are 
> sufficiently baked that even more FIXMEs around atomics in C++ would suffice?


FYI I ran into this here: https://reviews.llvm.org/D49458


https://reviews.llvm.org/D46112



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

[PATCH] D49458: Support implicit _Atomic struct load / store

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

I'm sure there are other bugs around `_Atomic`, please file a bug an CC me if 
that's the case. I'll commit this fix for now.


Repository:
  rC Clang

https://reviews.llvm.org/D49458



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

[PATCH] D49458: Support implicit _Atomic struct load / store

[JF Bastien via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL337410: Support implicit _Atomic struct load / store 
(authored by jfb, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D49458

Files:
  cfe/trunk/lib/Sema/Sema.cpp
  cfe/trunk/test/CodeGen/atomic-ops.c


Index: cfe/trunk/test/CodeGen/atomic-ops.c
===
--- cfe/trunk/test/CodeGen/atomic-ops.c
+++ cfe/trunk/test/CodeGen/atomic-ops.c
@@ -183,6 +183,18 @@
   double x;
 };
 
+void implicit_store(_Atomic(struct S) *a, struct S s) {
+  // CHECK-LABEL: @implicit_store(
+  // CHECK: store atomic i64 %{{.*}}, i64* %{{.*}} seq_cst, align 8
+  *a = s;
+}
+
+struct S implicit_load(_Atomic(struct S) *a) {
+  // CHECK-LABEL: @implicit_load(
+  // CHECK: load atomic i64, i64* %{{.*}} seq_cst, align 8
+  return *a;
+}
+
 struct S fd1(struct S *a) {
   // CHECK-LABEL: @fd1
   // CHECK: [[RETVAL:%.*]] = alloca %struct.S, align 4
Index: cfe/trunk/lib/Sema/Sema.cpp
===
--- cfe/trunk/lib/Sema/Sema.cpp
+++ cfe/trunk/lib/Sema/Sema.cpp
@@ -481,6 +481,7 @@
 case CK_ArrayToPointerDecay:
 case CK_FunctionToPointerDecay:
 case CK_ToVoid:
+case CK_NonAtomicToAtomic:
   break;
 }
   }


Index: cfe/trunk/test/CodeGen/atomic-ops.c
===
--- cfe/trunk/test/CodeGen/atomic-ops.c
+++ cfe/trunk/test/CodeGen/atomic-ops.c
@@ -183,6 +183,18 @@
   double x;
 };
 
+void implicit_store(_Atomic(struct S) *a, struct S s) {
+  // CHECK-LABEL: @implicit_store(
+  // CHECK: store atomic i64 %{{.*}}, i64* %{{.*}} seq_cst, align 8
+  *a = s;
+}
+
+struct S implicit_load(_Atomic(struct S) *a) {
+  // CHECK-LABEL: @implicit_load(
+  // CHECK: load atomic i64, i64* %{{.*}} seq_cst, align 8
+  return *a;
+}
+
 struct S fd1(struct S *a) {
   // CHECK-LABEL: @fd1
   // CHECK: [[RETVAL:%.*]] = alloca %struct.S, align 4
Index: cfe/trunk/lib/Sema/Sema.cpp
===
--- cfe/trunk/lib/Sema/Sema.cpp
+++ cfe/trunk/lib/Sema/Sema.cpp
@@ -481,6 +481,7 @@
 case CK_ArrayToPointerDecay:
 case CK_FunctionToPointerDecay:
 case CK_ToVoid:
+case CK_NonAtomicToAtomic:
   break;
 }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49771: CodeGen: use non-zero memset when possible for automatic variables

[JF Bastien via Phabricator via cfe-commits]

jfb created this revision.
jfb added a reviewer: dexonsmith.
Herald added a subscriber: cfe-commits.

Right now automatic variables are either initialized with bzero followed by a 
few stores, or memcpy'd from a synthesized global. We end up encountering a 
fair amount of code where memcpy of non-zero byte patterns would be better than 
memcpy from a global because it touches less memory and generates a smaller 
binary. The optimizer could reason about this, but it's not really worth it 
when clang already knows.

This code could definitely be more clever but I'm not sure it's worth it. In 
particular we could track a histogram of bytes seen and figure out (as we do 
with bzero) if a memset could be followed by a handful of stores. Similarly, we 
could tune the heuristics for GlobalSize, but using the same as for bzero seems 
conservatively OK for now.

rdar://problem/42563091


Repository:
  rC Clang

https://reviews.llvm.org/D49771

Files:
  lib/CodeGen/CGDecl.cpp
  test/CodeGen/init.c

Index: test/CodeGen/init.c
===
--- test/CodeGen/init.c
+++ test/CodeGen/init.c
@@ -140,6 +140,72 @@
   // CHECK: call void @bar
 }
 
+void nonzeroMemseti8() {
+  char arr[33] = { 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, };
+  // CHECK-LABEL: @nonzeroMemseti8(
+  // CHECK-NOT: store
+  // CHECK-NOT: memcpy
+  // CHECK: call void @llvm.memset.p0i8.i32(i8* {{.*}}, i8 42, i32 33, i1 false)
+}
+
+void nonzeroMemseti16() {
+  unsigned short arr[17] = { 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, };
+  // CHECK-LABEL: @nonzeroMemseti16(
+  // CHECK-NOT: store
+  // CHECK-NOT: memcpy
+  // CHECK: call void @llvm.memset.p0i8.i32(i8* {{.*}}, i8 66, i32 34, i1 false)
+}
+
+void nonzeroMemseti32() {
+  unsigned arr[9] = { 0xF0F0F0F0, 0xF0F0F0F0, 0xF0F0F0F0, 0xF0F0F0F0, 0xF0F0F0F0, 0xF0F0F0F0, 0xF0F0F0F0, 0xF0F0F0F0, 0xF0F0F0F0, };
+  // CHECK-LABEL: @nonzeroMemseti32(
+  // CHECK-NOT: store
+  // CHECK-NOT: memcpy
+  // CHECK: call void @llvm.memset.p0i8.i32(i8* {{.*}}, i8 -16, i32 36, i1 false)
+}
+
+void nonzeroMemseti64() {
+  unsigned long long arr[7] = { 0x, 0x, 0x, 0x, 0x,  0x,  0x,  };
+  // CHECK-LABEL: @nonzeroMemseti64(
+  // CHECK-NOT: store
+  // CHECK-NOT: memcpy
+  // CHECK: call void @llvm.memset.p0i8.i32(i8* {{.*}}, i8 -86, i32 56, i1 false)
+}
+
+void nonzeroMemsetf32() {
+  float arr[9] = { 0x1.cacacap+75, 0x1.cacacap+75, 0x1.cacacap+75, 0x1.cacacap+75, 0x1.cacacap+75, 0x1.cacacap+75, 0x1.cacacap+75, 0x1.cacacap+75, 0x1.cacacap+75, };
+  // CHECK-LABEL: @nonzeroMemsetf32(
+  // CHECK-NOT: store
+  // CHECK-NOT: memcpy
+  // CHECK: call void @llvm.memset.p0i8.i32(i8* {{.*}}, i8 101, i32 36, i1 false)
+}
+
+void nonzeroMemsetf64() {
+  double arr[7] = { 0x1.4p+69, 0x1.4p+69, 0x1.4p+69, 0x1.4p+69, 0x1.4p+69, 0x1.4p+69, 0x1.4p+69, };
+  // CHECK-LABEL: @nonzeroMemsetf64(
+  // CHECK-NOT: store
+  // CHECK-NOT: memcpy
+  // CHECK: call void @llvm.memset.p0i8.i32(i8* {{.*}}, i8 68, i32 56, i1 false)
+}
+
+void nonzeroPaddedUnionMemset() {
+  union U { char c; int i; };
+  union U arr[9] = { 0xF0, 0xF0, 0xF0, 0xF0, 0xF0, 0xF0, 0xF0, 0xF0, 0xF0, };
+  // CHECK-LABEL: @nonzeroPaddedUnionMemset(
+  // CHECK-NOT: store
+  // CHECK-NOT: memcpy
+  // CHECK: call void @llvm.memset.p0i8.i32(i8* {{.*}}, i8 -16, i32 36, i1 false)
+}
+
+void nonzeroNestedMemset() {
+  union U { char c; int i; };
+  struct S { union U u; int i; };
+  struct S arr[5] = { { {0xF0}, 0xF0F0F0F0 }, { {0xF0}, 0xF0F0F0F0 }, { {0xF0}, 0xF0F0F0F0 }, { {0xF0}, 0xF0F0F0F0 }, { {0xF0}, 0xF0F0F0F0 }, };
+  // CHECK-LABEL: @nonzeroNestedMemset(
+  // CHECK-NOT: store
+  // CHECK-NOT: memcpy
+  // CHECK: call void @llvm.memset.p0i8.i32(i8* {{.*}}, i8 -16, i32 40, i1 false)
+}
 
 // PR9257
 struct test11S {
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -948,6 +948,113 @@
  canEmitInitWithFewStoresAfterBZero(Init, StoreBudget);
 }
 
+/// A byte pattern.
+///
+/// Can be "any" pattern if the value was padding or known to be undef.
+/// Can be "none" pattern if a sequence doesn't exist.
+class BytePattern {
+  uint8_t Val;
+  enum class ValueType { Specific, Any, None } Type;
+  BytePattern(ValueType Type) : Type(Type) {}
+
+public:
+  BytePattern(uint8_t Value) : Val(Value), Type(ValueType::Specific) {}
+  static BytePattern Any() { return BytePattern(ValueType::Any); }
+  static BytePattern None() { return BytePattern(ValueType::None); }
+  bool isAny() const { return Type == ValueType::Any; }
+  bool isNone() const { 

[PATCH] D49771: CodeGen: use non-zero memset when possible for automatic variables

[JF Bastien via Phabricator via cfe-commits]

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

- Use short to test padding between array elements.
- Define enum class storage type; swap order of if / else to make it more 
readable.


Repository:
  rC Clang

https://reviews.llvm.org/D49771

Files:
  lib/CodeGen/CGDecl.cpp
  test/CodeGen/init.c

Index: test/CodeGen/init.c
===
--- test/CodeGen/init.c
+++ test/CodeGen/init.c
@@ -140,6 +140,72 @@
   // CHECK: call void @bar
 }
 
+void nonzeroMemseti8() {
+  char arr[33] = { 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, };
+  // CHECK-LABEL: @nonzeroMemseti8(
+  // CHECK-NOT: store
+  // CHECK-NOT: memcpy
+  // CHECK: call void @llvm.memset.p0i8.i32(i8* {{.*}}, i8 42, i32 33, i1 false)
+}
+
+void nonzeroMemseti16() {
+  unsigned short arr[17] = { 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, 0x4242, };
+  // CHECK-LABEL: @nonzeroMemseti16(
+  // CHECK-NOT: store
+  // CHECK-NOT: memcpy
+  // CHECK: call void @llvm.memset.p0i8.i32(i8* {{.*}}, i8 66, i32 34, i1 false)
+}
+
+void nonzeroMemseti32() {
+  unsigned arr[9] = { 0xF0F0F0F0, 0xF0F0F0F0, 0xF0F0F0F0, 0xF0F0F0F0, 0xF0F0F0F0, 0xF0F0F0F0, 0xF0F0F0F0, 0xF0F0F0F0, 0xF0F0F0F0, };
+  // CHECK-LABEL: @nonzeroMemseti32(
+  // CHECK-NOT: store
+  // CHECK-NOT: memcpy
+  // CHECK: call void @llvm.memset.p0i8.i32(i8* {{.*}}, i8 -16, i32 36, i1 false)
+}
+
+void nonzeroMemseti64() {
+  unsigned long long arr[7] = { 0x, 0x, 0x, 0x, 0x,  0x,  0x,  };
+  // CHECK-LABEL: @nonzeroMemseti64(
+  // CHECK-NOT: store
+  // CHECK-NOT: memcpy
+  // CHECK: call void @llvm.memset.p0i8.i32(i8* {{.*}}, i8 -86, i32 56, i1 false)
+}
+
+void nonzeroMemsetf32() {
+  float arr[9] = { 0x1.cacacap+75, 0x1.cacacap+75, 0x1.cacacap+75, 0x1.cacacap+75, 0x1.cacacap+75, 0x1.cacacap+75, 0x1.cacacap+75, 0x1.cacacap+75, 0x1.cacacap+75, };
+  // CHECK-LABEL: @nonzeroMemsetf32(
+  // CHECK-NOT: store
+  // CHECK-NOT: memcpy
+  // CHECK: call void @llvm.memset.p0i8.i32(i8* {{.*}}, i8 101, i32 36, i1 false)
+}
+
+void nonzeroMemsetf64() {
+  double arr[7] = { 0x1.4p+69, 0x1.4p+69, 0x1.4p+69, 0x1.4p+69, 0x1.4p+69, 0x1.4p+69, 0x1.4p+69, };
+  // CHECK-LABEL: @nonzeroMemsetf64(
+  // CHECK-NOT: store
+  // CHECK-NOT: memcpy
+  // CHECK: call void @llvm.memset.p0i8.i32(i8* {{.*}}, i8 68, i32 56, i1 false)
+}
+
+void nonzeroPaddedUnionMemset() {
+  union U { char c; int i; };
+  union U arr[9] = { 0xF0, 0xF0, 0xF0, 0xF0, 0xF0, 0xF0, 0xF0, 0xF0, 0xF0, };
+  // CHECK-LABEL: @nonzeroPaddedUnionMemset(
+  // CHECK-NOT: store
+  // CHECK-NOT: memcpy
+  // CHECK: call void @llvm.memset.p0i8.i32(i8* {{.*}}, i8 -16, i32 36, i1 false)
+}
+
+void nonzeroNestedMemset() {
+  union U { char c; int i; };
+  struct S { union U u; short i; };
+  struct S arr[5] = { { {0xF0}, 0xF0F0 }, { {0xF0}, 0xF0F0 }, { {0xF0}, 0xF0F0 }, { {0xF0}, 0xF0F0 }, { {0xF0}, 0xF0F0 }, };
+  // CHECK-LABEL: @nonzeroNestedMemset(
+  // CHECK-NOT: store
+  // CHECK-NOT: memcpy
+  // CHECK: call void @llvm.memset.p0i8.i32(i8* {{.*}}, i8 -16, i32 40, i1 false)
+}
 
 // PR9257
 struct test11S {
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -948,6 +948,113 @@
  canEmitInitWithFewStoresAfterBZero(Init, StoreBudget);
 }
 
+/// A byte pattern.
+///
+/// Can be "any" pattern if the value was padding or known to be undef.
+/// Can be "none" pattern if a sequence doesn't exist.
+class BytePattern {
+  uint8_t Val;
+  enum class ValueType : uint8_t { Specific, Any, None } Type;
+  BytePattern(ValueType Type) : Type(Type) {}
+
+public:
+  BytePattern(uint8_t Value) : Val(Value), Type(ValueType::Specific) {}
+  static BytePattern Any() { return BytePattern(ValueType::Any); }
+  static BytePattern None() { return BytePattern(ValueType::None); }
+  bool isAny() const { return Type == ValueType::Any; }
+  bool isNone() const { return Type == ValueType::None; }
+  bool isValued() const { return Type == ValueType::Specific; }
+  uint8_t getValue() const {
+assert(isValued());
+return Val;
+  }
+  BytePattern merge(const BytePattern Other) const {
+if (isNone() || Other.isNone())
+  return None();
+if (isAny())
+  return Other;
+if (Other.isAny())
+  return *this;
+if (getValue() == Other.getValue())
+  return *this;
+return None();
+  }
+};
+
+/// Figures out whether the constant can be initialized with memset.
+static BytePattern constantIsRepeatedBytePattern(llvm::Constant *C) {
+  if (isa(C) || isa(C))
+return 

[PATCH] D49771: CodeGen: use non-zero memset when possible for automatic variables

[JF Bastien via Phabricator via cfe-commits]

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

Addressed all comments.




Comment at: lib/CodeGen/CGDecl.cpp:956-957
+class BytePattern {
+  uint8_t Val;
+  enum class ValueType { Specific, Any, None } Type;
+  BytePattern(ValueType Type) : Type(Type) {}

bogner wrote:
> Probably makes sense to swap the order of these or give the enum class a 
> smaller underlying type than int.
I defined the enum class' storage type as `uint8_t`.


Repository:
  rC Clang

https://reviews.llvm.org/D49771



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

[PATCH] D49771: CodeGen: use non-zero memset when possible for automatic variables

[JF Bastien via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL337887: CodeGen: use non-zero memset when possible for 
automatic variables (authored by jfb, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D49771

Files:
  cfe/trunk/lib/CodeGen/CGDecl.cpp
  cfe/trunk/test/CodeGen/init.c

Index: cfe/trunk/lib/CodeGen/CGDecl.cpp
===
--- cfe/trunk/lib/CodeGen/CGDecl.cpp
+++ cfe/trunk/lib/CodeGen/CGDecl.cpp
@@ -948,6 +948,113 @@
  canEmitInitWithFewStoresAfterBZero(Init, StoreBudget);
 }
 
+/// A byte pattern.
+///
+/// Can be "any" pattern if the value was padding or known to be undef.
+/// Can be "none" pattern if a sequence doesn't exist.
+class BytePattern {
+  uint8_t Val;
+  enum class ValueType : uint8_t { Specific, Any, None } Type;
+  BytePattern(ValueType Type) : Type(Type) {}
+
+public:
+  BytePattern(uint8_t Value) : Val(Value), Type(ValueType::Specific) {}
+  static BytePattern Any() { return BytePattern(ValueType::Any); }
+  static BytePattern None() { return BytePattern(ValueType::None); }
+  bool isAny() const { return Type == ValueType::Any; }
+  bool isNone() const { return Type == ValueType::None; }
+  bool isValued() const { return Type == ValueType::Specific; }
+  uint8_t getValue() const {
+assert(isValued());
+return Val;
+  }
+  BytePattern merge(const BytePattern Other) const {
+if (isNone() || Other.isNone())
+  return None();
+if (isAny())
+  return Other;
+if (Other.isAny())
+  return *this;
+if (getValue() == Other.getValue())
+  return *this;
+return None();
+  }
+};
+
+/// Figures out whether the constant can be initialized with memset.
+static BytePattern constantIsRepeatedBytePattern(llvm::Constant *C) {
+  if (isa(C) || isa(C))
+return BytePattern(0x00);
+  if (isa(C))
+return BytePattern::Any();
+
+  if (isa(C)) {
+auto *Int = cast(C);
+if (Int->getBitWidth() % 8 != 0)
+  return BytePattern::None();
+const llvm::APInt  = Int->getValue();
+if (Value.isSplat(8))
+  return BytePattern(Value.getLoBits(8).getLimitedValue());
+return BytePattern::None();
+  }
+
+  if (isa(C)) {
+auto *FP = cast(C);
+llvm::APInt Bits = FP->getValueAPF().bitcastToAPInt();
+if (Bits.getBitWidth() % 8 != 0)
+  return BytePattern::None();
+if (!Bits.isSplat(8))
+  return BytePattern::None();
+return BytePattern(Bits.getLimitedValue() & 0xFF);
+  }
+
+  if (isa(C)) {
+llvm::Constant *Splat = cast(C)->getSplatValue();
+if (Splat)
+  return constantIsRepeatedBytePattern(Splat);
+return BytePattern::None();
+  }
+
+  if (isa(C) || isa(C)) {
+BytePattern Pattern(BytePattern::Any());
+for (unsigned I = 0, E = C->getNumOperands(); I != E; ++I) {
+  llvm::Constant *Elt = cast(C->getOperand(I));
+  Pattern = Pattern.merge(constantIsRepeatedBytePattern(Elt));
+  if (Pattern.isNone())
+return Pattern;
+}
+return Pattern;
+  }
+
+  if (llvm::ConstantDataSequential *CDS =
+  dyn_cast(C)) {
+BytePattern Pattern(BytePattern::Any());
+for (unsigned I = 0, E = CDS->getNumElements(); I != E; ++I) {
+  llvm::Constant *Elt = CDS->getElementAsConstant(I);
+  Pattern = Pattern.merge(constantIsRepeatedBytePattern(Elt));
+  if (Pattern.isNone())
+return Pattern;
+}
+return Pattern;
+  }
+
+  // BlockAddress, ConstantExpr, and everything else is scary.
+  return BytePattern::None();
+}
+
+/// Decide whether we should use memset to initialize a local variable instead
+/// of using a memcpy from a constant global. Assumes we've already decided to
+/// not user bzero.
+/// FIXME We could be more clever, as we are for bzero above, and generate
+///   memset followed by stores. It's unclear that's worth the effort.
+static BytePattern shouldUseMemSetToInitialize(llvm::Constant *Init,
+   uint64_t GlobalSize) {
+  uint64_t SizeLimit = 32;
+  if (GlobalSize <= SizeLimit)
+return BytePattern::None();
+  return constantIsRepeatedBytePattern(Init);
+}
+
 /// EmitAutoVarDecl - Emit code and set up an entry in LocalDeclMap for a
 /// variable declaration with auto, register, or no storage class specifier.
 /// These turn into simple stack objects, or GlobalValues depending on target.
@@ -1401,41 +1508,49 @@
   if (Loc.getType() != BP)
 Loc = Builder.CreateBitCast(Loc, BP);
 
-  // If the initializer is all or mostly zeros, codegen with bzero then do a
-  // few stores afterward.
-  if (shouldUseBZeroPlusStoresToInitialize(
-  constant,
-  CGM.getDataLayout().getTypeAllocSize(constant->getType( {
+  // If the initializer is all or mostly the same, codegen with bzero / memset
+  // then do a few stores afterward.
+  uint64_t ConstantSize =
+  CGM.getDataLayout().getTypeAllocSize(constant->getType());
+  if 

[PATCH] D51084: Implement -Watomic-implicit-seq-cst

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In https://reviews.llvm.org/D51084#1216913, @rjmccall wrote:

> It says the type of the assignment expression, not the type of the LHS.
>
> C11 [6.5.16]p2: "The type of an assignment expression is the type the left 
> operand would have after lvalue conversion."
>
> C11 [6.3.2.1]p2: "...this is called lvalue conversion. If the lvalue has 
> qualified type, the value has the unqualified version of the type of the 
> lvalue; additionally, if the lvalue has atomic type, the value has the 
> non-atomic version of the type of the lvalue; otherwise, the value has the 
> type of the lvalue."
>
> The RHS is not converted to have atomic type.


Right that would be nonsensical because you'd need to load it atomically. The C 
`_Atomic` semantics match what C++ `std::atomic` does:

  _Atomic(int) atom;
  void ass(int i) {
  atom = i;
  }

is the same as:

  #include 
  std::atomic atom;
  void ass(int i) {
  atom = i;
  }

They're meant to be interchangeable.

This warning is meant to warn on C's implicit `seq_cst`. When I get to writing 
the C++ version of the warning I'll warn about C++'s implicit `seq_cst`.


Repository:
  rC Clang

https://reviews.llvm.org/D51084



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

[PATCH] D51084: Implement -Watomic-implicit-seq-cst

[JF Bastien via Phabricator via cfe-commits]

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

- Don't diagnose initialization, only assignment.


Repository:
  rC Clang

https://reviews.llvm.org/D51084

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaChecking.cpp
  test/Sema/atomic-implicit-seq_cst.c
  test/Sema/sync-implicit-seq_cst.c

Index: test/Sema/sync-implicit-seq_cst.c
===
--- /dev/null
+++ test/Sema/sync-implicit-seq_cst.c
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 %s -verify -ffreestanding -fsyntax-only -triple=i686-linux-gnu -std=c11 -Watomic-implicit-seq-cst -Wno-sync-fetch-and-nand-semantics-changed
+
+// __sync_* operations are implicitly sequentially-consistent. Some codebases
+// want to force explicit usage of memory order instead.
+
+void fetch_and_add(int *ptr, int val) { __sync_fetch_and_add(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void fetch_and_sub(int *ptr, int val) { __sync_fetch_and_sub(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void fetch_and_or(int *ptr, int val) { __sync_fetch_and_or(ptr, val); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void fetch_and_and(int *ptr, int val) { __sync_fetch_and_and(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void fetch_and_xor(int *ptr, int val) { __sync_fetch_and_xor(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void fetch_and_nand(int *ptr, int val) { __sync_fetch_and_nand(ptr, val); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+
+void add_and_fetch(int *ptr, int val) { __sync_add_and_fetch(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void sub_and_fetch(int *ptr, int val) { __sync_sub_and_fetch(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void or_and_fetch(int *ptr, int val) { __sync_or_and_fetch(ptr, val); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void and_and_fetch(int *ptr, int val) { __sync_and_and_fetch(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void xor_and_fetch(int *ptr, int val) { __sync_xor_and_fetch(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void nand_and_fetch(int *ptr, int val) { __sync_nand_and_fetch(ptr, val); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+
+void bool_compare_and_swap(int *ptr, int oldval, int newval) { __sync_bool_compare_and_swap(ptr, oldval, newval); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void val_compare_and_swap(int *ptr, int oldval, int newval) { __sync_val_compare_and_swap(ptr, oldval, newval); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+
+void synchronize(void) { __sync_synchronize(); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+
+void lock_test_and_set(int *ptr, int val) { __sync_lock_test_and_set(ptr, val); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void lock_release(int *ptr) { __sync_lock_release(ptr); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
Index: test/Sema/atomic-implicit-seq_cst.c
===
--- /dev/null
+++ test/Sema/atomic-implicit-seq_cst.c
@@ -0,0 +1,325 @@
+// RUN: %clang_cc1 %s -verify -ffreestanding -fsyntax-only -triple=i686-linux-gnu -std=c11 -Watomic-implicit-seq-cst
+
+// _Atomic operations are implicitly sequentially-consistent. Some codebases
+// want to force explicit usage of memory order instead.
+
+_Atomic(int) atom;
+void gimme_int(int);
+
+void bad_pre_inc(void) {
+  ++atom; // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+}
+
+void bad_pre_dec(void) {
+  --atom; // expected-warning {{implicit use of sequentially-consistent 

[PATCH] D51084: Implement -Watomic-implicit-seq-cst

[JF Bastien via Phabricator via cfe-commits]

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

- Address comments.


Repository:
  rC Clang

https://reviews.llvm.org/D51084

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaChecking.cpp
  test/Sema/atomic-implicit-seq_cst.c
  test/Sema/sync-implicit-seq_cst.c

Index: test/Sema/sync-implicit-seq_cst.c
===
--- /dev/null
+++ test/Sema/sync-implicit-seq_cst.c
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 %s -verify -ffreestanding -fsyntax-only -triple=i686-linux-gnu -std=c11 -Watomic-implicit-seq-cst -Wno-sync-fetch-and-nand-semantics-changed
+
+// __sync_* operations are implicitly sequentially-consistent. Some codebases
+// want to force explicit usage of memory order instead.
+
+void fetch_and_add(int *ptr, int val) { __sync_fetch_and_add(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void fetch_and_sub(int *ptr, int val) { __sync_fetch_and_sub(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void fetch_and_or(int *ptr, int val) { __sync_fetch_and_or(ptr, val); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void fetch_and_and(int *ptr, int val) { __sync_fetch_and_and(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void fetch_and_xor(int *ptr, int val) { __sync_fetch_and_xor(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void fetch_and_nand(int *ptr, int val) { __sync_fetch_and_nand(ptr, val); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+
+void add_and_fetch(int *ptr, int val) { __sync_add_and_fetch(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void sub_and_fetch(int *ptr, int val) { __sync_sub_and_fetch(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void or_and_fetch(int *ptr, int val) { __sync_or_and_fetch(ptr, val); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void and_and_fetch(int *ptr, int val) { __sync_and_and_fetch(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void xor_and_fetch(int *ptr, int val) { __sync_xor_and_fetch(ptr, val); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void nand_and_fetch(int *ptr, int val) { __sync_nand_and_fetch(ptr, val); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+
+void bool_compare_and_swap(int *ptr, int oldval, int newval) { __sync_bool_compare_and_swap(ptr, oldval, newval); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void val_compare_and_swap(int *ptr, int oldval, int newval) { __sync_val_compare_and_swap(ptr, oldval, newval); }   // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+
+void synchronize(void) { __sync_synchronize(); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+
+void lock_test_and_set(int *ptr, int val) { __sync_lock_test_and_set(ptr, val); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+void lock_release(int *ptr) { __sync_lock_release(ptr); } // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
Index: test/Sema/atomic-implicit-seq_cst.c
===
--- /dev/null
+++ test/Sema/atomic-implicit-seq_cst.c
@@ -0,0 +1,325 @@
+// RUN: %clang_cc1 %s -verify -ffreestanding -fsyntax-only -triple=i686-linux-gnu -std=c11 -Watomic-implicit-seq-cst
+
+// _Atomic operations are implicitly sequentially-consistent. Some codebases
+// want to force explicit usage of memory order instead.
+
+_Atomic(int) atom;
+void gimme_int(int);
+
+void bad_pre_inc(void) {
+  ++atom; // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
+}
+
+void bad_pre_dec(void) {
+  --atom; // expected-warning {{implicit use of