Re: [libcxx] r278147 - Update in-tree Google Benchmark to current ToT.

[Craig, Ben via cfe-commits]

This is probably more of a comment on the compare_bench.py side of 
things, but here goes anyway...


When comparing two results, both the percentage and absolute differences 
can be very important.  It is very easy to be misled by percentages.  It 
is harder to be misled with absolute differences.


For example, I think it is more insightful to see something like this 
for the comparison:


Comparing ./util_smartptr.native.out to ./util_smartptr.libcxx.out
Benchmark  Time   CPU
-
BM_SharedPtrCreateDestroy -16 ns (-26%) -16 ns (-26%)
BM_SharedPtrIncDecRef +0 ns (+0%)   +0 ns (+0%)
BM_WeakPtrIncDecRef   +6 ns (+21%)  +6 ns (+21%)

This presentation makes it easier to evaluate the significance of 
shifted costs.  The shared_ptr changes moved some costs around, and this 
benchmark shows that 16 ns left one area, and turned into 6 ns 
elsewhere.  The percentages mask that insight.


I've been in benchmarking situations where one function's time is 
reduced by 5%, and another function increases by 100%.  Some managers 
started panicking because of it.  In actuality, 5 ms was shaved off of a 
100 ms operation that was in the inner loop.  The 5 ms was moved into 
the outer loop which previously took 5 ms.



On 8/9/2016 1:56 PM, Eric Fiselier via cfe-commits wrote:

Author: ericwf
Date: Tue Aug  9 13:56:48 2016
New Revision: 278147

URL: http://llvm.org/viewvc/llvm-project?rev=278147&view=rev
Log:
Update in-tree Google Benchmark to current ToT.

I've put some work into the Google Benchmark library in order to make it easier
to benchmark libc++. These changes have already been upstreamed into
Google Benchmark and this patch applies the changes to the in-tree version.

The main improvement in the addition of a 'compare_bench.py' script which
makes it very easy to compare benchmarks. For example to compare the native
STL to libc++ you would run:

`$ compare_bench.py ./util_smartptr.native.out ./util_smartptr.libcxx.out`

And the output would look like:

RUNNING: ./util_smartptr.native.out
Benchmark  Time   CPU Iterations

BM_SharedPtrCreateDestroy 62 ns 62 ns   10937500
BM_SharedPtrIncDecRef 31 ns 31 ns   23972603
BM_WeakPtrIncDecRef   28 ns 28 ns   23648649
RUNNING: ./util_smartptr.libcxx.out
Benchmark  Time   CPU Iterations

BM_SharedPtrCreateDestroy 46 ns 46 ns   14957265
BM_SharedPtrIncDecRef 31 ns 31 ns   22435897
BM_WeakPtrIncDecRef   34 ns 34 ns   21084337
Comparing ./util_smartptr.native.out to ./util_smartptr.libcxx.out
Benchmark  Time   CPU
-
BM_SharedPtrCreateDestroy -0.26 -0.26
BM_SharedPtrIncDecRef +0.00 +0.00
BM_WeakPtrIncDecRef   +0.21 +0.21

Added:
 libcxx/trunk/utils/google-benchmark/test/multiple_ranges_test.cc
 libcxx/trunk/utils/google-benchmark/test/register_benchmark_test.cc
 libcxx/trunk/utils/google-benchmark/tools/
 libcxx/trunk/utils/google-benchmark/tools/compare_bench.py
 libcxx/trunk/utils/google-benchmark/tools/gbench/
 libcxx/trunk/utils/google-benchmark/tools/gbench/Inputs/
 libcxx/trunk/utils/google-benchmark/tools/gbench/Inputs/test1_run1.json
 libcxx/trunk/utils/google-benchmark/tools/gbench/Inputs/test1_run2.json
 libcxx/trunk/utils/google-benchmark/tools/gbench/__init__.py
 libcxx/trunk/utils/google-benchmark/tools/gbench/report.py
 libcxx/trunk/utils/google-benchmark/tools/gbench/util.py
Modified:
 libcxx/trunk/benchmarks/CMakeLists.txt
 libcxx/trunk/benchmarks/ContainerBenchmarks.hpp
 libcxx/trunk/benchmarks/algorithms.bench.cpp
 libcxx/trunk/benchmarks/unordered_set_operations.bench.cpp
 libcxx/trunk/utils/google-benchmark/AUTHORS
 libcxx/trunk/utils/google-benchmark/README.md
 libcxx/trunk/utils/google-benchmark/include/benchmark/benchmark_api.h
 libcxx/trunk/utils/google-benchmark/include/benchmark/macros.h
 libcxx/trunk/utils/google-benchmark/include/benchmark/reporter.h
 libcxx/trunk/utils/google-benchmark/src/benchmark.cc
 libcxx/trunk/utils/google-benchmark/src/colorprint.cc
 libcxx/trunk/utils/google-benchmark/src/colorprint.h
 libcxx/trunk/utils/google-benchmark/src/complexity.cc
 libcxx/trunk/utils/google-benchmark/src/console_reporter.cc
 libcxx/trunk/utils/google-benchmark/src/cycleclock.h
 libcxx/trunk/utils/google-benchmark/src/sysinfo.cc
 libcxx/trunk/utils/google-benchmark/test/CMakeLists.txt
 libcxx/trunk/utils/google-benchmark/test/basic_test.cc
 libcxx/trunk/utils/google-benchmark/test/benchm

Re: [libcxx] r260235 - Introduce a cmake module to figure out whether we need to link with libatomic.

[Craig, Ben via cfe-commits]

FYI, this change and the LLVM version have each broken my libcxx 
builds.  I cross compile from Linux x64 to Hexagon, and my host machine 
doesn't have .  The LLVM change was particularly offensive 
because I wasn't planning on building LLVM itself.


I have since worked around the issue by making my cmake invocation longer.

I'm not sure what you can do for the LLVM check, but if you can use the 
libcxx headers directly for the  check, while still using all 
the custom flags and tools that I pass on the command line, then I 
should be able to remove the explicit setting of 
"LIBCXX_HAVE_CXX_ATOMICS_WITHOUT_LIB" in the future.


On 2/16/2016 2:09 PM, Vasileios Kalintiris via cfe-commits wrote:

Hi Hans,


Or is this comment on PR26059 all that needs to be done for 3.8?

That's correct. I wrote that comment in order to clarify which bits we should 
merge in 3.8.

The latest commit:

- [libcxx] r260961 - Issue a warning instead of fatal errors when checks for 
libatomic fail."

makes sure that my changes don't break older configurations that used to work 
previously, by emitting a warning message instead of a fatal error.

I had to turn the error to warning because there are cases where the host compiler doesn't 
provide the  and  headers when bootstrapping llvm.

The correct solution would be to use the headers provided from libc++ itself. 
However, I wanted to take the safe route for the 3.8 branch and provide the 
elegant solution on the trunk after having the proper discussion with the 
libc++ devs.

Thanks,
Vasileios


From: hwennb...@google.com [hwennb...@google.com] on behalf of Hans Wennborg 
[h...@chromium.org]
Sent: 16 February 2016 19:53
To: Vasileios Kalintiris
Cc: Eric Fiselier; Daniel Sanders; mclow.li...@gmail.com; 
cfe-commits@lists.llvm.org
Subject: Re: [libcxx] r260235 - Introduce a cmake module to figure out whether 
we need to link with libatomic.

Or is this comment on PR26059 all that needs to be done for 3.8?

"Bug 26369, which has been fixed with r260961, requires the following
commits to get merged on the release branch:

- [libcxx] r260515 - Re-commit "Introduce a cmake module to figure out
whether we need to link with libatomic."
- [libcxx] r260524 - Fix r260515 - Correct typos in CMake changes
- [libcxx] r260531 - Rename CheckLibcxxAtomic.cmake variable result
names so they don't clash with LLVM
- [libcxx] r260961 - Issue a warning instead of fatal errors when
checks for libatomic fail."

Thanks,
Hans

On Tue, Feb 16, 2016 at 11:47 AM, Hans Wennborg  wrote:

Do I understand correctly that there are still issues here's that are not fixed?

(I'm trying to understand if there is something here to merge for 3.8,
but I'm having trouble following these commits.)

On Tue, Feb 16, 2016 at 6:44 AM, Vasileios Kalintiris
 wrote:

I changed the type of message from fatal_error to warning in r260961. While
the test for atomics works fine in most cases, it fails because we include
 and , and the user's host compiler might not provide them
during a bootstrap (see PR26631 and PR26622).

Does anyone have any idea how to tackle this problem? As suggested by the
bug reports, we could include the headers provided by libc++. However, I'm
not sure whether we are supposed to do that during configuration time.

- Vasileios



From: Eric Fiselier [e...@efcs.ca]
Sent: 11 February 2016 16:08
To: Daniel Sanders
Cc: Vasileios Kalintiris; h...@chromium.org; mclow.li...@gmail.com;
cfe-commits@lists.llvm.org
Subject: Re: [libcxx] r260235 - Introduce a cmake module to figure out
whether we need to link with libatomic.


  we need to rename HAVE_CXX_ATOMICS_WITH_LIB to something like
LIBCXX_HAVE_CXX_ATOMICS_WITH_LIB.

Fixed in r260531.

I think we will eventually want to merge the following commits, assuming
they don't regress the build, especially with the -gcc-toolchain option.

- [libcxx] r260515 - Re-commit "Introduce a cmake module to figure out
whether we need to link with libatomic."
- [libcxx] r260524 - Fix r260515 - Correct typos in CMake changes
- [libcxx] r260531 - Rename CheckLibcxxAtomic.cmake variable result names so
they don't clash with LLVM

@Marshall Any objections?

On Thu, Feb 11, 2016 at 2:18 AM, Daniel Sanders via cfe-commits
 wrote:

Hi,

In my latests rc2+patches build I've also found that we need to rename
HAVE_CXX_ATOMICS_WITH_LIB to something like
LIBCXX_HAVE_CXX_ATOMICS_WITH_LIB. It's currently re-using the result of
LLVM's check which doesn't include 64-bit atomics.

From: Vasileios Kalintiris
Sent: 09 February 2016 23:50
To: h...@chromium.org
Cc: cfe-commits@lists.llvm.org; Daniel Sanders; mclow.li...@gmail.com
Subject: RE: [libcxx] r260235 - Introduce a cmake module to figure out
whether we need to link with libatomic.

Hi Hans,

Please wait before merging this as it breaks LLVM bootstraps when using
the -gcc-toolchain option and the system's default gcc installation does n

Re: [PATCH] D17380: [libcxx] Split locale management out of ibm/xlocale.h. NFCI

[Craig, Ben via cfe-commits]

On 2/19/2016 8:36 AM, Joerg Sonnenberger via cfe-commits wrote:
On Thu, Feb 18, 2016 at 03:39:29PM +, Ben Craig via cfe-commits 
wrote:

Unfortunately, I have no access to an AIX machine to build with, so
this change has been made blind.

Any reason why this is not using "new" and "delete"? I think I would
prefer to not define uselocale, but that can be left until later.

Joerg
For now, I'm just shuffling functionality around.  The old code used 
calloc and free, so this rearranged code uses calloc and free.


Soon, I will be renaming uselocale to _CXX_uselocale.  On platforms that 
natively have uselocale, _CXX_uselocale will be #define'd to uselocale.  
On platforms like AIX, _CXX_uselocale will be some function.


As for changing calloc and free to new and delete... I'm in favor of 
that, but I'm not the right person to do it.  I'm already nervous enough 
doing mechanical shuffling of code without being able to compile the 
results.



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


--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project


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

Re: [PATCH] D17416: [libcxx] Reorganize locale extension fallbacks

[Craig, Ben via cfe-commits]

On 2/19/2016 8:40 AM, Joerg Sonnenberger via cfe-commits wrote:

On Thu, Feb 18, 2016 at 10:32:11PM +, Ben Craig via cfe-commits wrote:

The fallback locale functions are also useful on their own for other
lightweight platforms. Putting these fallback implementations in
support/xlocale should enable code sharing.

Shouldn't the fallback functions be using uselocale() when available?

Joerg
Soon, yes.  This change is just moving functions from one header to 
another.  Doing a move + significant change is painful to review... but 
you can see some of where that is going in this now-defunct review:

http://reviews.llvm.org/D17146

I fully intend on making each of the fallback functions call one of the 
uselocale RAII objects.  I would need to ensure that the RAII object 
inlined away on platforms without extended locale support.



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


--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

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

Re: [PATCH] D17456: [libcxx] Reorganize _LIBCPP_LOCALE__L_EXTENSIONS

[Craig, Ben via cfe-commits]

On 2/19/2016 3:32 PM, Joerg Sonnenberger via cfe-commits wrote:

On Fri, Feb 19, 2016 at 06:14:18PM +, Ben Craig via cfe-commits wrote:

Instead of checking _LIBCPP_LOCALE__L_EXTENSIONS all over, instead
check it once, and define the various *_l symbols once.

If you want to rename using macros, please use the argument form. I find
that to provide better self-documentation.

Joerg

Would the following form address your concerns?
#define __libcxx_sscanf_l(...) sscanf_l(__VA_ARGS__)

I think that getting more elaborate than that general form would be more 
effort and more buggy without significant customer benefit.



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


--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

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

Re: [libcxx] r272634 - Implement variadic lock_guard.

[Craig, Ben via cfe-commits]

Does this change (and the paper) permit declarations like the following?

lock_guard<> guard();

If that syntax is allowed, then this is also likely allowed...

lock_guard<>(guard);

I would really like the prior two examples to not compile.  Here is a 
common bug that I see in the wild...


unique_guard(some_member_mutex);

That defines a new, default constructed unique_guard named 
"some_member_mutex", that likely shadows the member variable 
some_member_mutex.  It is almost never what users want.


Is it possible to have the empty template remain undefined, and let the 
one element lock_guard be the base case of the recursion? Does that help 
any with the mangling?



On 6/14/2016 8:24 PM, Eric Fiselier via cfe-commits wrote:

Update on the bot failures:

I've spoken to the owner of the bots and they are planning to upgrade 
their Clang versions.

This will get the bots green again.

/Eric

On Mon, Jun 13, 2016 at 11:49 PM, Eric Fiselier > wrote:


This is causing some of the libc++ bots to go red.
`variadic_copy.fail.cpp` is encountering an error, which seems to
be a clang bug which temporarily existed in 3.9.
The test passes against current ToT and older clang releases and GCC.

Please do not revert this commit due to that specific failure. I
am aware of it and I am working to fix it.

On Mon, Jun 13, 2016 at 9:48 PM, Eric Fiselier via cfe-commits
mailto:cfe-commits@lists.llvm.org>>
wrote:

Author: ericwf
Date: Mon Jun 13 22:48:09 2016
New Revision: 272634

URL: http://llvm.org/viewvc/llvm-project?rev=272634&view=rev
Log:
Implement variadic lock_guard.

Summary:
This patch implements the variadic `lock_guard` paper.

Making `lock_guard` variadic is a ABI breaking change because
the specialization `lock_guard<_Mutex>` mangles differently
then when it was the primary template. This change only
provides variadic `lock_guard` in ABI V2 or when
`_LIBCPP_ABI_VARIADIC_LOCK_GUARD` is defined.

Note that in ABI V2 `lock_guard` must always be declared as a
variadic template, even in C++03, in order to keep the ABI
consistent. For this reason `lock_guard` is forward declared
as a variadic template in all standard dialects and therefore
depends on variadic templates being provided as an extension
in C++03. All supported versions of Clang and GCC provide this
extension.




Reviewers: mclow.lists

Subscribers: K-ballo, mclow.lists, cfe-commits

Differential Revision: http://reviews.llvm.org/D21260

Added:
libcxx/trunk/test/libcxx/thread/thread.mutex/thread.lock/

libcxx/trunk/test/libcxx/thread/thread.mutex/thread.lock/thread.lock.guard/

libcxx/trunk/test/libcxx/thread/thread.mutex/thread.lock/thread.lock.guard/variadic_mutex_mangling.pass.cpp

libcxx/trunk/test/std/thread/thread.mutex/thread.lock/thread.lock.guard/variadic_adopt_lock.pass.cpp

libcxx/trunk/test/std/thread/thread.mutex/thread.lock/thread.lock.guard/variadic_assign.fail.cpp

libcxx/trunk/test/std/thread/thread.mutex/thread.lock/thread.lock.guard/variadic_copy.fail.cpp

libcxx/trunk/test/std/thread/thread.mutex/thread.lock/thread.lock.guard/variadic_mutex.fail.cpp

libcxx/trunk/test/std/thread/thread.mutex/thread.lock/thread.lock.guard/variadic_mutex.pass.cpp

libcxx/trunk/test/std/thread/thread.mutex/thread.lock/thread.lock.guard/variadic_mutex_cxx03.pass.cpp

libcxx/trunk/test/std/thread/thread.mutex/thread.lock/thread.lock.guard/variadic_types.pass.cpp
Modified:
libcxx/trunk/include/__config
libcxx/trunk/include/__mutex_base
libcxx/trunk/include/mutex

libcxx/trunk/test/std/thread/thread.mutex/thread.lock/thread.lock.guard/mutex.fail.cpp
libcxx/trunk/www/cxx1z_status.html

Modified: libcxx/trunk/include/__config
URL:

http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__config?rev=272634&r1=272633&r2=272634&view=diff

==
--- libcxx/trunk/include/__config (original)
+++ libcxx/trunk/include/__config Mon Jun 13 22:48:09 2016
@@ -43,6 +43,7 @@
 #define _LIBCPP_ABI_LIST_REMOVE_NODE_POINTER_UB
 #define _LIBCPP_ABI_FORWARD_LIST_REMOVE_NODE_POINTER_UB
 #define _LIBCPP_ABI_FIX_UNORDERED_CONTAINER_SIZE_TYPE
+#define _LIBCPP_ABI_VARIADIC_LOCK_GUARD
 #endif

 #define _LIBCPP_CONCAT1(_LIBCPP_X,_LIBCPP_Y) _LIBCPP_X##_LIBCPP_Y

Modified: libcxx/trunk/include/__mutex_base
URL:

http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__mutex_base?rev=272634&r1=272633&r2=272634&view=diff

=

Re: [libcxx] r272634 - Implement variadic lock_guard.

[Craig, Ben via cfe-commits]

On 6/15/2016 1:15 PM, Eric Fiselier wrote:
On Wed, Jun 15, 2016 at 11:45 AM, Craig, Ben via cfe-commits 
mailto:cfe-commits@lists.llvm.org>> wrote:


Does this change (and the paper) permit declarations like the
following?

lock_guard<> guard();

If that syntax is allowed, then this is also likely allowed...

lock_guard<>(guard);

I would really like the prior two examples to not compile.  Here
is a common bug that I see in the wild...

unique_guard(some_member_mutex);

That defines a new, default constructed unique_guard named
"some_member_mutex", that likely shadows the member variable
some_member_mutex.  It is almost never what users want.


I had no idea that syntax did that. I would have assumed it created an 
unnamed temporary. I can see how that would cause bugs.
It's also strong rationale for deduced constructor templates. 
(http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0091r0.html)

auto guard = unique_guard(some_member_mutex);
You don't need to repeat types there, and it's very difficult to forget 
to name the guard variable.



Is it possible to have the empty template remain undefined, and
let the one element lock_guard be the base case of the recursion? 
Does that help any with the mangling?


Nothing in the spec says the empty template should be undefined. The 
default constructor on the empty template is technically implementing 
"lock_guard(MutexTypes...)" for an empty pack.
However your example provides ample motivation to make it undefined. 
I'll go ahead and make that change and I'll file a LWG defect to 
change the standard.


There is actually no recursion in the variadic lock_guard 
implementation, so the change is trivial.


As for mangling I'm not sure what you mean? It definitely doesn't 
change the fact that this change is ABI breaking. (Note this change is 
not enabled by default for that reason).
My thought regarding the mangling was that you could still provide a one 
argument lock_guard, as well as a variadic lock_guard.  The one argument 
lock_guard would have the same mangling as before.  I think some of your 
other comments have convinced me that that won't work, as I think the 
variadic lock_guard has to be made the primary template, and I think the 
primary template dictates the mangling.


I'm also going to guess that throwing inline namespaces at the problem 
won't help, as that would probably cause compile-time ambiguity.


If I'm not mistaken, this only breaks ABI for those foolish enough to 
pass a lock_guard reference or pointer as a parameter across a libcxx 
version boundary.  Does that sound accurate?


--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

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

Re: [libcxx] r272634 - Implement variadic lock_guard.

[Craig, Ben via cfe-commits]

Makes sense.  Here's hoping parameter deduction for constructors makes 
it in!


(better link) 
http://open-std.org/JTC1/SC22/WG21/docs/papers/2016/p0091r2.html



On 6/15/2016 1:54 PM, Eric Fiselier wrote:


I've had a change of heart. I think that lock_guard<> has some utility 
in generic code, and I'm not sure removing it is a good idea. For 
example a function like:


template 
void ExecuteUnderLocks(Func&& fn, Locks&... locks) {
  lock_guard g(locks...);
  fn();
}

I checked the proposal and it's clear that "lock_guard<>" is expected 
to compile and be default constructable. For this reason I'm not going 
to remove "lock_guard<>", at least not without further discussion.


On Wed, Jun 15, 2016 at 12:47 PM, Craig, Ben <mailto:ben.cr...@codeaurora.org>> wrote:


    On 6/15/2016 1:15 PM, Eric Fiselier wrote:

On Wed, Jun 15, 2016 at 11:45 AM, Craig, Ben via cfe-commits
mailto:cfe-commits@lists.llvm.org>>
wrote:

Does this change (and the paper) permit declarations like the
following?

lock_guard<> guard();

If that syntax is allowed, then this is also likely allowed...

lock_guard<>(guard);

I would really like the prior two examples to not compile. 
Here is a common bug that I see in the wild...


unique_guard(some_member_mutex);

That defines a new, default constructed unique_guard named
"some_member_mutex", that likely shadows the member variable
some_member_mutex.  It is almost never what users want.


I had no idea that syntax did that. I would have assumed it
created an unnamed temporary. I can see how that would cause bugs.

It's also strong rationale for deduced constructor templates.
(http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0091r0.html)
auto guard = unique_guard(some_member_mutex);
You don't need to repeat types there, and it's very difficult to
forget to name the guard variable.


Is it possible to have the empty template remain undefined,
and let the one element lock_guard be the base case of the
recursion?  Does that help any with the mangling?

Nothing in the spec says the empty template should be undefined.
The default constructor on the empty template is technically
implementing "lock_guard(MutexTypes...)" for an empty pack.
However your example provides ample motivation to make it
undefined. I'll go ahead and make that change and I'll file a LWG
defect to change the standard.

There is actually no recursion in the variadic lock_guard
implementation, so the change is trivial.

As for mangling I'm not sure what you mean? It definitely doesn't
change the fact that this change is ABI breaking. (Note this
change is not enabled by default for that reason).

My thought regarding the mangling was that you could still provide
a one argument lock_guard, as well as a variadic lock_guard.  The
one argument lock_guard would have the same mangling as before.  I
think some of your other comments have convinced me that that
won't work, as I think the variadic lock_guard has to be made the
primary template, and I think the primary template dictates the
mangling.


Exactly.


I'm also going to guess that throwing inline namespaces at the
problem won't help, as that would probably cause compile-time
ambiguity.

If I'm not mistaken, this only breaks ABI for those foolish enough
to pass a lock_guard reference or pointer as a parameter across a
libcxx version boundary.  Does that sound accurate?


It breaks the ABI any time "lock_guard" participates in the 
mangling of some function or type. In addition to your example this 
will also break any time "lock_guard" is used as a template 
parameter: ie


using T = MyType>;
MyFunction>();

The two different implementations are still layout compatible, so if 
mangling were not an issue I think this change would have been safe.


-- 
Employee of Qualcomm Innovation Center, Inc.

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project




--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

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

Re: [libcxx] r272634 - Implement variadic lock_guard.

[Craig, Ben via cfe-commits]

It would be awesome if this kind of shadowing warning could be put into 
-Wall.  My recollection on the last set of -Wshadow reviews is that most 
shadowing warnings are from ctor arguments being used to initialize 
members.  Here's the last discussion / review regarding shadowing 
http://reviews.llvm.org/D18271



On 6/15/2016 2:22 PM, Eric Fiselier wrote:
Maybe we should add a new warning in Clang for this. -Wshadow 
diagnosis's this but -Wshadow isn't a part of -Wall or -Wextra so it's 
of limited utility.

A separate warning for shadowing 'x' caused by "T(x)" might be useful.

Do people actually use "T(x)" in the wild to default construct 'x'?

/Eric


On Wed, Jun 15, 2016 at 1:07 PM, Craig, Ben <mailto:ben.cr...@codeaurora.org>> wrote:


Makes sense.  Here's hoping parameter deduction for constructors
makes it in!

(better link)
http://open-std.org/JTC1/SC22/WG21/docs/papers/2016/p0091r2.html


On 6/15/2016 1:54 PM, Eric Fiselier wrote:


I've had a change of heart. I think that lock_guard<> has some
utility in generic code, and I'm not sure removing it is a good
idea. For example a function like:

template 
void ExecuteUnderLocks(Func&& fn, Locks&... locks) {
  lock_guard g(locks...);
  fn();
}

I checked the proposal and it's clear that "lock_guard<>" is
expected to compile and be default constructable. For this reason
I'm not going to remove "lock_guard<>", at least not without
further discussion.

On Wed, Jun 15, 2016 at 12:47 PM, Craig, Ben
    mailto:ben.cr...@codeaurora.org>> wrote:

On 6/15/2016 1:15 PM, Eric Fiselier wrote:

On Wed, Jun 15, 2016 at 11:45 AM, Craig, Ben via cfe-commits
mailto:cfe-commits@lists.llvm.org>> wrote:

Does this change (and the paper) permit declarations
like the following?

lock_guard<> guard();

If that syntax is allowed, then this is also likely
allowed...

lock_guard<>(guard);

I would really like the prior two examples to not
compile.  Here is a common bug that I see in the wild...

unique_guard(some_member_mutex);

That defines a new, default constructed unique_guard
named "some_member_mutex", that likely shadows the
member variable some_member_mutex.  It is almost never
what users want.


I had no idea that syntax did that. I would have assumed it
created an unnamed temporary. I can see how that would cause
bugs.

It's also strong rationale for deduced constructor templates.
(http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0091r0.html)
auto guard = unique_guard(some_member_mutex);
You don't need to repeat types there, and it's very difficult
to forget to name the guard variable.


Is it possible to have the empty template remain
undefined, and let the one element lock_guard be the
base case of the recursion?  Does that help any with the
mangling?

Nothing in the spec says the empty template should be
undefined. The default constructor on the empty template is
technically implementing "lock_guard(MutexTypes...)" for an
empty pack.
However your example provides ample motivation to make it
undefined. I'll go ahead and make that change and I'll file
a LWG defect to change the standard.

There is actually no recursion in the variadic lock_guard
implementation, so the change is trivial.

As for mangling I'm not sure what you mean? It definitely
doesn't change the fact that this change is ABI breaking.
(Note this change is not enabled by default for that reason).

My thought regarding the mangling was that you could still
provide a one argument lock_guard, as well as a variadic
lock_guard.  The one argument lock_guard would have the same
mangling as before.  I think some of your other comments have
convinced me that that won't work, as I think the variadic
lock_guard has to be made the primary template, and I think
the primary template dictates the mangling.


Exactly.


I'm also going to guess that throwing inline namespaces at
the problem won't help, as that would probably cause
compile-time ambiguity.

If I'm not mistaken, this only breaks ABI for those foolish
enough to pass a lock_guard reference or pointer as a
parameter across a libcxx version boundary.  Does that sound
accurate?


It breaks the ABI any time "lock_guard" partic

Re: [PATCH] D17741: adds __FILE_BASENAME__ builtin macro

[Craig, Ben via cfe-commits]

On 3/4/2016 6:23 AM, Joerg Sonnenberger via cfe-commits wrote:

On Thu, Mar 03, 2016 at 04:50:11PM -0800, Nico Weber via cfe-commits wrote:

On Thu, Mar 3, 2016 at 4:28 PM, Joerg Sonnenberger via cfe-commits <
cfe-commits@lists.llvm.org> wrote:


On Thu, Mar 03, 2016 at 02:09:17PM -0800, Nico Weber via cfe-commits wrote:

On Thu, Mar 3, 2016 at 1:50 PM, Joerg Sonnenberger via cfe-commits <
cfe-commits@lists.llvm.org> wrote:


On Thu, Mar 03, 2016 at 07:39:04PM +, Weiming Zhao via cfe-commits
wrote:

Change the option name to -ffile-macro-prefix-to-remove

This still sounds to me like a solution for a very restricted part of a
much more generic problem...


What do you mean?

Storing only the base name of file names is a strict subset of __FILE__
transformations. As mentioned in the linked GCC PR, other uses are
creating build location independent output for larger software projects
etc. For that, reducing to the base name is not an option as it removes
too much information.


But ffile-macro-prefix-to-remove has a general prefix arg, which you can
set to your build dir. This isn't just a "get me the basename" flag (?)

It doesn't help to make sure that path names are always using /usr/src
or src/ and build/ without requiring those to be part of the build
layout. Another instance not handled is symlink trees for controlling
access, where you still want to use the original path etc.

This isn't changing the defaults.  In addition, you could have a mix of 
files in the same binary, some with ffile-macro-prefix-to-remove set, 
and some without.  In cases where the user wants to be explicit about 
full name vs. base name in the source, this patch also provides 
__CLANG_FILE_BASENAME__.


There doesn't seem to be one approach to improve all of the existing use 
cases.  This patch provides two independent tools that can be used to 
improve many of the use cases.


--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

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

Re: [PATCH] D17950: Implement is_always_lock_free

[Craig, Ben via cfe-commits]

I know that MIPS does that, and an out-of-tree implementation of hexagon 
implements 1-byte cmpxchg in terms of the 4-byte version. The emulation 
code isn't particularly small, and it seems reasonable to make it a 
libcall.  The emulation code seems sketchy from a correctness 
perspective, as you end up generating unsolicited loads and stores on 
adjacent bytes.  Take a look at the thread on cfe-dev and llvm-dev named 
"the as-if rule / perf vs. security" for some of the ramifications and 
concerns surrounding unsolicited loads and stores.


On 3/17/2016 10:05 AM, James Y Knight wrote:


> A 4 byte cmpxchg could be lock free, while a 1 byte cmpxchg may not 
be lock free.


That's not possible: if you have a 4-byte cmpxchg instruction, you can 
use it to implement a 1-byte cmpxchg, too. Many targets do this 
already. (It would be better if that was available generically so that 
code didn't need to be written separately fit each target, but that's 
not the case at the moment.)




--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

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

Re: [PATCH] D17950: Implement is_always_lock_free

[Craig, Ben via cfe-commits]

On 3/18/2016 1:50 AM, Joerg Sonnenberger via cfe-commits wrote:

On Thu, Mar 17, 2016 at 05:56:17PM +, JF Bastien via cfe-commits wrote:

C++ atomics are explicitly designed to avoid problems with touching
adjacent bytes: if `atomic` where `sizeof(T) == 1` requires a 4-byte
`cmpxchg` then it's up to the frontend to make sure `sizeof> >= 4`
(or something equivalent such as making it non-lock-free).

That's not completely relevant for the code at hand. At least the
__sync_* builtins only ever required appropiate alignment of the base
type, not necessarily extra alignment to avoid padding for stupid
codegen. I also believe that access to extra data is completely harmless
for all but one use case. If you are directly accessing memory mapped
devices using compiler atomics, you should really know what you are
doing. That's about the only case where it matters.
Some architectures support byte granularity memory protection. Accessing 
unsolicited bytes can cause a trap on those architectures.


I think it makes sense for atomic and Atomic to get turned 
into the 4 byte __atomic intrinsics.  Those 4 byte __atomic intrinsics 
can then get lowered to efficient inlined code. If someone has a regular 
char, and they use the __atomic or __sync intrinsics directly, I'm fine 
with that going to a lib call.  The lib call can then either use the 
global lock shard, or access extra data, depending on what is reasonable 
for that platform.


-- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation 
Center, Inc. is a member of Code Aurora Forum, a Linux Foundation 
Collaborative Project

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

Re: [PATCH] D17950: Implement is_always_lock_free

[Craig, Ben via cfe-commits]

On 3/18/2016 11:54 AM, JF Bastien wrote:


Some architectures support byte granularity memory protection.
Accessing unsolicited bytes can cause a trap on those architectures.

I think it makes sense for atomic and Atomic to get
turned into the 4 byte __atomic intrinsics.  Those 4 byte __atomic
intrinsics can then get lowered to efficient inlined code. If
someone has a regular char, and they use the __atomic or __sync
intrinsics directly, I'm fine with that going to a lib call.  The
lib call can then either use the global lock shard, or access
extra data, depending on what is reasonable for that platform.


That all sounds like something the frontend has to decide on based on 
what the target is.


A lot of it is a frontend decision.  What goes in the libcall feels an 
awful lot like the 386 vs 486 example that I hear a lot.  If I want one 
binary that can run on both a 386 (very limited atomic support) and a 
486 (a bit more atomic support), then I generate the binary such that it 
has library calls to atomic routines.  The library on the 386 uses the 
global lock shard, and the library on the 486 uses native instructions.  
The same kind of thing would apply for one byte atomics when only four 
byte atomics have hardware support.  The library call would use the lock 
shard as the default. If the platform doesn't have byte granularity 
memory protection a different, lockless implementation could be used 
that loads the surrounding four bytes instead and does all the masking.


--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

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

Re: [PATCH] D14506: Porting shouldVisitImplicitCode to DataRecursiveASTVisitor.

[Craig, Ben via cfe-commits]

I'm fine with this approach.  How about I leave the file in place, but 
replace the contents with a "using DataRecursiveASTVisitor = 
RecursiveASTVisitor;" and see what breaks?  That way I won't need to go 
through a large retrofit.


On 11/16/2015 3:28 PM, Richard Smith wrote:
Rather than trying to maintain the horrible duplication between 
DataRecursiveASTVisitor and RecursiveASTVisitor, can we just delete 
DataRecursiveASTVisitor? RecursiveASTVisitor is data-recursive too 
these days (and has a smarter implementation than 
DataRecursiveASTVisitor's from what I can see), but doesn't yet apply 
data recursion in so many cases.


On Mon, Nov 16, 2015 at 1:07 PM, Argyrios Kyrtzidis > wrote:


LGTM.

> On Nov 16, 2015, at 12:32 PM, Ben Craig
mailto:ben.cr...@codeaurora.org>> wrote:
>
> bcraig added a comment.
>
> Ping.  Note that the test is basically a copy / paste job, and
the new code in DataRecursiveASTVisitor.h is a very direct
translation from the 'regular' RecursiveASTVisitor.h.
>
>
> http://reviews.llvm.org/D14506
>
>
>




--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

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

Re: [PATCH] D14506: Porting shouldVisitImplicitCode to DataRecursiveASTVisitor.

[Craig, Ben via cfe-commits]

Looks good to me.  I'm not too worried about the compactness of the 
visitor class.


On 11/16/2015 7:10 PM, Richard Smith wrote:
Attached patch makes RAV fully data-recursive when visiting 
statements, except in cases where the derived class could tell the 
difference (when it falls back to a normal recursive walk). The queue 
representation is slightly less compact than before: instead of 
storing a child iterator, we now store a list of all children. This 
allows us to handle any Stmt subclass that we can traverse, not just 
those ones that finish by traversing all their children in the usual 
order.


Thoughts?

On Mon, Nov 16, 2015 at 2:28 PM, Craig, Ben via cfe-commits 
mailto:cfe-commits@lists.llvm.org>> wrote:


I'm fine with this approach.  How about I leave the file in place,
but replace the contents with a "using DataRecursiveASTVisitor =
RecursiveASTVisitor;" and see what breaks?  That way I won't need
to go through a large retrofit.


On 11/16/2015 3:28 PM, Richard Smith wrote:

Rather than trying to maintain the horrible duplication between
DataRecursiveASTVisitor and RecursiveASTVisitor, can we just
delete DataRecursiveASTVisitor? RecursiveASTVisitor is
data-recursive too these days (and has a smarter implementation
than DataRecursiveASTVisitor's from what I can see), but doesn't
yet apply data recursion in so many cases.

On Mon, Nov 16, 2015 at 1:07 PM, Argyrios Kyrtzidis
mailto:akyr...@gmail.com>> wrote:

LGTM.

> On Nov 16, 2015, at 12:32 PM, Ben Craig
mailto:ben.cr...@codeaurora.org>>
wrote:
>
> bcraig added a comment.
>
> Ping.  Note that the test is basically a copy / paste job,
and the new code in DataRecursiveASTVisitor.h is a very
direct translation from the 'regular' RecursiveASTVisitor.h.
>
>
> http://reviews.llvm.org/D14506
>
>
>




-- 
Employee of Qualcomm Innovation Center, Inc.

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project


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




--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

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

Re: [PATCH] D15539: [libcxxabi] Reducing stack usage of test

[Craig, Ben via cfe-commits]

As an alternative, would it be acceptable to heap allocate these 
objects, using the original padding numbers?


On 12/15/2015 5:11 PM, Howard Hinnant wrote:

On Dec 15, 2015, at 5:30 PM, Jonathan Roelofs  wrote:


Could these large padding things be related to the fact that the test is used 
as a performance check for the implementation? That being said, I have no idea 
who is paying attention to the numbers that come out of this test (if anyone 
even is?). Maybe @howard.hinnant knows something about it?

It’s been a few years since I wrote this code, but my best recollection is that 
the large buffers indicate my concern of getting the correct answer 
accidentally as the dynamic_cast code will add/subtract offsets to pointers.  
The implementation is wicked hard to get right, and so the tests are extra 
paranoid.

As I wrote this I only had to worry about OS X / iOS.  If you need to reduce 
these numbers for other platforms, I think that would be fine.  However I would 
not reduce them to double digits.  I would keep them as large as you can 
reasonably get away with.

There may have also been effort to make sure that the numbers involved were not 
even close to multiples of one another, or add up to each other, etc, I don’t 
remember for sure.  The main concern is that the offset arithmetic implemented 
under __dynamic_cast does not accidentally get the right answer.  And I suspect 
alignment is also factored in to the offset computation (I don’t recall for 
sure), and so one can not depend entirely on small primes (which may be rounded 
to alignment).

Handy link:

http://mentorembedded.github.io/cxx-abi/abi.html#dynamic_cast-algorithm

Normally I don’t write a lot of comments in code.  Code should be self 
explanatory.  The sheer volume of comments in private_typeinfo.cpp should serve 
as a signal that this code is very tricky.  So I recommend keeping the tests as 
paranoid as possible.

Howard



--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

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