Page MenuHomePhabricator

bcraig (Ben Craig)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 1 2015, 9:50 AM (222 w, 3 d)

Recent Activity

Oct 1 2019

bcraig added a comment to D68275: [libcxx] [test] Query the target platform, not the host one.

I think it would be a huge help, because running libc++ tests on a remote target is exactly what I'm trying to accomplish.

Oct 1 2019, 2:23 PM · Restricted Project

Feb 4 2019

bcraig added a comment to D57624: Support tests in freestanding.

I like this too, however I don't know how this interacts with tests like test/libcxx/language.support/support.dynamic/new_faligned_allocation.sh.cpp that specify the build command-line explicitly:

// RUN: %build -faligned-allocation

We'd make sure that %build links in the additional entry point?

Agreed that seems problematic, but I think we can fix it after this patch lands. :)

Feb 4 2019, 11:28 AM · Restricted Project
bcraig added a comment to D57624: Support tests in freestanding.

For what it's worth, when I ran these tests with MSVC in the Windows kernel, I just called main, and falling off the end of main happened to work. I can't say I'm thrilled with that approach though.

Feb 4 2019, 7:18 AM · Restricted Project

Oct 24 2018

bcraig added a comment to D53206: Allow padding checker to traverse simple class hierarchies.

add a comment, and it will be fine in my eyes. You'll need signoff from the code owner though.

Oct 24 2018, 9:00 AM · Restricted Project

Oct 12 2018

bcraig added inline comments to D53206: Allow padding checker to traverse simple class hierarchies.
Oct 12 2018, 7:57 PM · Restricted Project
bcraig added a comment to D53206: Allow padding checker to traverse simple class hierarchies.

You should probably add whoever the current code owner of that static analyzer to this review. I have very little involvement with Clang these days, so a "LGTM" from me doesn't carry much weight.

Oct 12 2018, 11:21 AM · Restricted Project

Apr 24 2018

bcraig added a comment to D41316: [libcxx] Allow random_device to be built optionally.

For those that would prefer random device to not exist if it isn't cryptographically secure, please write a wg21 paper. I will gladly review such a paper, and if you need a presenter, then I will present it if I am attending. I won't be at Rapperswil, but I will be at San Diego.

Apr 24 2018, 11:54 AM

Apr 4 2018

bcraig added a comment to D32411: [libcxx] Provide #include_next alternative for MSVC.

I spoke to STL on the MSVC team a while ago, and he stated that if we produced a paper describing why we need #include_next and the rational behind it, and they would pass that on to the front-end team. They didn't guarantee that they would implement it, but that seems like a good first step.

Apr 4 2018, 6:39 AM

Dec 9 2017

bcraig updated the diff for D32411: [libcxx] Provide #include_next alternative for MSVC.

Rebased

Dec 9 2017, 9:01 AM

Dec 5 2017

bcraig added a comment to D32411: [libcxx] Provide #include_next alternative for MSVC.

Talked with mclow and some Microsoft devs in Albuquerque. I'm not expecting include_next to show up in MSVC. Sometime in the next month I hope to rebase this change.

Dec 5 2017, 9:46 AM

Sep 11 2017

bcraig added a comment to D37677: [libc++] implement future synchronization using atomic_flag.

This current patch just swaps out std::mutex for a std::mutex-alike class that claims to be faster for uncontested accesses. Definitely safer than my interpretation. :) If this patch actually helps, then I would offer that the class could be provided as a reusable class std::__spin_lock in the <mutex> header instead of being hidden inside __assoc_shared_state.

Sep 11 2017, 11:02 AM
bcraig added a comment to D37677: [libc++] implement future synchronization using atomic_flag.

Is there a benchmark where this demonstrates some performance improvement? I fear that the switch to condition_variable_any will swamp any performance gains from the switch to a spin lock.

Sep 11 2017, 6:20 AM

Aug 21 2017

bcraig added a comment to D36423: [libc++] Introsort based sorting function.

LGTM. You should probably get one other person to approve though. I'm hoping that @EricWF or @mclow.lists will take a look

Aug 21 2017, 5:48 PM

Aug 15 2017

bcraig added a comment to D36423: [libc++] Introsort based sorting function.

The test headers should not be in the production include folder. They should probably be in the benchmark folder.

Aug 15 2017, 7:09 AM

Aug 11 2017

bcraig added a comment to D32411: [libcxx] Provide #include_next alternative for MSVC.

ping

Aug 11 2017, 7:49 AM
bcraig added a comment to D34170: [libcxx] Moving compiler specific test infrastructure to compiler.py.

ping

Aug 11 2017, 7:49 AM

Aug 10 2017

bcraig added a comment to D36423: [libc++] Introsort based sorting function.

If you want the performance improvements in the BM_sort_std_worst_quick cases preserved, you really need to port the benchmark from Aditya's repo into the libcxx benchmark code base. Otherwise, the next person that comes along to improve std::sort performance may very well wreck the performance gains you achieved.

Aug 10 2017, 6:28 PM

Aug 8 2017

bcraig added a comment to D36423: [libc++] Introsort based sorting function.

I like this change in general. Dinkumware has been using introsort for 10+ years, so I'm a bit surprised that libc++ wasn't already.

Aug 8 2017, 6:56 PM
bcraig added a comment to D36423: [libc++] Introsort based sorting function.

Those are interesting (and useful) results... but they don't look like they came from the same algorithms.bench.cpp that I'm looking at...
https://github.com/llvm-mirror/libcxx/blob/master/benchmarks/algorithms.bench.cpp

Aug 8 2017, 6:46 PM

Aug 7 2017

bcraig added a comment to D36423: [libc++] Introsort based sorting function.

alternatively, you could report the comparison of the old code vs. the new code with an existing benchmark, like benchmarks/algorithms.bench.cpp

Aug 7 2017, 6:54 PM
bcraig added a comment to D36423: [libc++] Introsort based sorting function.

This patch needs benchmarks that demonstrate the performance changes.

Aug 7 2017, 3:41 PM

Jul 31 2017

bcraig added inline comments to D18174: Fix libcxx build on musl.
Jul 31 2017, 5:58 AM

Jul 23 2017

bcraig added a comment to D32411: [libcxx] Provide #include_next alternative for MSVC.

ping

Jul 23 2017, 5:19 PM
bcraig added a comment to D34170: [libcxx] Moving compiler specific test infrastructure to compiler.py.

ping

Jul 23 2017, 5:19 PM

Jul 12 2017

bcraig closed D35174: [libc++] Fix unrepresentable enum for clang-cl unstable ABI.
Jul 12 2017, 11:48 AM

Jul 11 2017

bcraig committed rL307751: Fix unrepresentable enum for clang-cl unstable ABI.
Fix unrepresentable enum for clang-cl unstable ABI
Jul 11 2017, 6:45 PM

Jul 10 2017

bcraig updated the diff for D35174: [libc++] Fix unrepresentable enum for clang-cl unstable ABI.

Switched to static consts

Jul 10 2017, 7:06 AM

Jul 9 2017

bcraig created D35174: [libc++] Fix unrepresentable enum for clang-cl unstable ABI.
Jul 9 2017, 6:14 AM

Jul 5 2017

bcraig updated the diff for D34170: [libcxx] Moving compiler specific test infrastructure to compiler.py.

Rebased.
Separating out logging into it's own class.
Also tweaked the output slightly so that the language dialect under test shows up as the first line of output.

Jul 5 2017, 7:19 AM

Jun 27 2017

bcraig added a comment to D34588: Check for _MSC_VER before define _LIBCPP_MSVCRT.

_LIBCPP_MS_CRT seems fine too.

Jun 27 2017, 5:17 AM

Jun 26 2017

bcraig added a comment to D34588: Check for _MSC_VER before define _LIBCPP_MSVCRT.

LGTM, but you should probably get approval from somebody a bit more senior with the project.

Jun 26 2017, 6:57 PM

Jun 25 2017

bcraig added a comment to D32411: [libcxx] Provide #include_next alternative for MSVC.

ping

Jun 25 2017, 5:48 PM

Jun 24 2017

bcraig added inline comments to D34588: Check for _MSC_VER before define _LIBCPP_MSVCRT.
Jun 24 2017, 7:07 PM

Jun 15 2017

bcraig added a comment to D32411: [libcxx] Provide #include_next alternative for MSVC.

ping

Jun 15 2017, 2:43 PM
bcraig abandoned D20596: [libcxx] Refactor locale switching, creation, and destruction.

This is very stale at this point, and isn't blocking anything. Closing.

Jun 15 2017, 2:42 PM

Jun 14 2017

bcraig accepted D32146: PR32476: __nop_locale_mgmt.h not needed with newlib 2.5+.

LGTM.
@waltl : Do you need me to submit the changes, or will you handle that?

Jun 14 2017, 6:07 AM

Jun 13 2017

bcraig created D34170: [libcxx] Moving compiler specific test infrastructure to compiler.py.
Jun 13 2017, 1:53 PM

May 29 2017

bcraig updated the diff for D32411: [libcxx] Provide #include_next alternative for MSVC.
May 29 2017, 6:40 PM

May 25 2017

bcraig added a comment to D33082: Fix Libc++ build with MinGW64.

Are you suggesting that libc++ should stop declaring/defining these functions, at least publically?

May 25 2017, 6:47 PM

May 18 2017

bcraig added inline comments to D32146: PR32476: __nop_locale_mgmt.h not needed with newlib 2.5+.
May 18 2017, 6:55 PM

May 12 2017

bcraig added a comment to D33080: [Libc++] Use #pragma push_macro/pop_macro to better handle min/max on Windows.

I noticed that the Windows STL headers have to do this dance with new (even though they do (foo)(...) for min and max). If we're going to need
to guard against a bunch of macros I would like to use a single approach. Other than updating the #if defined(min) || defined(max) lines it's trivial to guard
against additional macros.

May 12 2017, 6:19 AM

May 11 2017

bcraig added a comment to D33082: Fix Libc++ build with MinGW64.

I agree that we need to be precise in our platform detection macros. On Windows, we currently need to worry about which compiler is being used, whether we are targeting the mingw environment or the native (?) environment, and which c-runtime is being used.

May 11 2017, 8:26 AM
bcraig added a comment to D33080: [Libc++] Use #pragma push_macro/pop_macro to better handle min/max on Windows.

I like the warning that you generate for min and max macros existing. Is the push_macro / pop_macro the right way to go though? You could throw extra parenthesis around the declaration and usages of min/max to avoid macro expansion.

May 11 2017, 6:36 AM

May 10 2017

bcraig added a comment to D32988: [libc++] Refactor Windows support headers..

LGTM

May 10 2017, 1:56 PM

May 9 2017

bcraig added inline comments to D32988: [libc++] Refactor Windows support headers..
May 9 2017, 9:12 AM

May 8 2017

bcraig committed rL302497: Fix Windows tests when __config_site is present..
Fix Windows tests when __config_site is present.
May 8 2017, 6:47 PM
bcraig committed rL302496: Revert "Fix Windows tests when __config_site is present.".
Revert "Fix Windows tests when __config_site is present."
May 8 2017, 6:39 PM
bcraig committed rL302421: Fix Windows tests when __config_site is present..
Fix Windows tests when __config_site is present.
May 8 2017, 6:28 AM

May 7 2017

bcraig added a comment to D32927: [libc++] Implement exception_ptr on Windows.

Getting the test suite green sooner rather than later seems like a good reason to temporarily pick a 1-3 year solution rather than a 5+ year solution. Also, as you mention, this isn't all throw away work, so it's still progress.

May 7 2017, 6:20 PM

May 6 2017

bcraig added a comment to D32927: [libc++] Implement exception_ptr on Windows.

libstdc++ and the Visual Studio C++ runtime have very different compatibility expectations.

May 6 2017, 6:20 PM
bcraig added inline comments to D32927: [libc++] Implement exception_ptr on Windows.
May 6 2017, 3:15 PM

May 4 2017

bcraig added a comment to D32411: [libcxx] Provide #include_next alternative for MSVC.

I'm still working on this.

May 4 2017, 7:18 PM

Apr 24 2017

bcraig added a comment to D32411: [libcxx] Provide #include_next alternative for MSVC.

BTW, the list of include files which are located in [PROGRAM_FILES]\Microsoft Visual Studio 14.0\VC\include directory is

  • stdbool.h
  • limits.h
  • stdint.h
  • setjmp.h

    The rest is in [PROGRAM_FILES]\Windows Kits\10\Include\10.0.whatever.0\ucrt directory. Which directory @_LIBCPP_INCLUDE_NEXT@ is supposed to point to?
Apr 24 2017, 6:52 AM

Apr 23 2017

bcraig created D32411: [libcxx] Provide #include_next alternative for MSVC.
Apr 23 2017, 6:58 PM

Apr 21 2017

bcraig abandoned D32147: [PR32479] Avoid newlib vasprintf, since it requires gnu extensions.
Apr 21 2017, 6:42 AM

Apr 17 2017

bcraig updated the diff for D32147: [PR32479] Avoid newlib vasprintf, since it requires gnu extensions.
Apr 17 2017, 6:28 PM
bcraig created D32147: [PR32479] Avoid newlib vasprintf, since it requires gnu extensions.
Apr 17 2017, 6:25 PM
bcraig created D32146: PR32476: __nop_locale_mgmt.h not needed with newlib 2.5+.
Apr 17 2017, 6:21 PM

Apr 11 2017

bcraig committed rL299942: [libc++] Fix unknown pragma warning on MSVC.
[libc++] Fix unknown pragma warning on MSVC
Apr 11 2017, 7:19 AM

Sep 9 2016

bcraig added a comment to D24278: [analyzer] Extend bug reports with extra notes..

Neat! I would have liked to have had this for the Excess Padding Checker. Currently, the padding checker has a very long diagnostic that recommends a new order for data members. I think a note (or fixit) would be more appropriate, but that support didn't exist previously.

Sep 9 2016, 11:31 AM
bcraig added a comment to D24374: [libc++] Avoid <memory> include in locale_win32.h.

@bcraig thanks for pointing me to that diff; there's a lot of nice cleanup going on there. Were you planning on updating and following up on it?

I also realized I forgot to adjust locale_win32.cpp for this diff. Will re-upload with that fixed.

Sep 9 2016, 11:01 AM
bcraig added a comment to D24374: [libc++] Avoid <memory> include in locale_win32.h.

This seems related:
https://reviews.llvm.org/D20596

Sep 9 2016, 6:05 AM

Sep 2 2016

bcraig added a comment to D24159: Fix PR30202 - notify_all_at_thread_exit seg faults if run from a raw pthread context..

LGTM

Sep 2 2016, 12:32 PM
bcraig added inline comments to D24159: Fix PR30202 - notify_all_at_thread_exit seg faults if run from a raw pthread context..
Sep 2 2016, 6:23 AM

Sep 1 2016

bcraig added a comment to D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation.

The "@" will do a ping through phabricator, but a direct email is probably going to be your best bet at this point.

Sep 1 2016, 1:33 PM

Aug 31 2016

bcraig added a comment to D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation.

Ping?

Aug 31 2016, 11:53 AM

Aug 25 2016

bcraig added inline comments to D20083: Add an c++ itanium demangler to llvm.
Aug 25 2016, 7:15 AM

Aug 24 2016

bcraig added inline comments to D20083: Add an c++ itanium demangler to llvm.
Aug 24 2016, 9:25 AM

Aug 19 2016

bcraig added a comment to D23719: [libc++] Use C11 for atomics check.

Are we really guaranteed that the C and C++ compiler behave the same way here? I don't see why that would necessarily be the case.

For libc++, std::atomic is implemented in terms of _Atomic. So as long as the C++ compiler doesn't butcher _Atomic, it seems that the behavior would be the same.

I don't see any good reason to assume that's the case. GCC, for instance, does not define _Atomic *at all* in C++ mode; the implementation used by libc++ in that case is completely different. Also, as far as I know, libc++ did not previously require the host to have a C11 compiler. And there's no reason to assume that the C compiler picked up by cmake is in any way related to the C++ compiler.

Bottom line: if you want to know how the C++ compiler behaves, you need to test the C++ compiler not the C compiler.

Aug 19 2016, 11:55 AM
bcraig added a comment to D23719: [libc++] Use C11 for atomics check.

@bcraig: __STDC_NO_ATOMICS__ wouldn't be defined for pre-C11 compilers either, right?

From what I understand of the original code sample, one of the purposes was to check for 64-bit atomic support on 32-bit systems (hence the use of uintmax_t and the check for __atomic_fetch_add_8). I don't really know of a portable way of accomplishing that without at least including <stdint.h>.

Aug 19 2016, 11:15 AM
bcraig added a comment to D23719: [libc++] Use C11 for atomics check.

Are we really guaranteed that the C and C++ compiler behave the same way here? I don't see why that would necessarily be the case.

Aug 19 2016, 11:11 AM
bcraig added a comment to D23719: [libc++] Use C11 for atomics check.

I like the rationale here, but can we avoid pulling in headers at all?
You could test _ _STDC_NO_ATOMICS_ _. You could also have some code like this...

Aug 19 2016, 10:50 AM

Aug 2 2016

bcraig committed rL277456: Fixing 'Aquire' typo and libcxx build..
Fixing 'Aquire' typo and libcxx build.
Aug 2 2016, 6:51 AM

Aug 1 2016

bcraig committed rL277373: Adding smart_ptr benchmark.
Adding smart_ptr benchmark
Aug 1 2016, 1:04 PM
bcraig closed D22470: [libcxx] Improve shared_ptr dtor performance.

committed rL277357: Improve shared_ptr dtor performance.

Aug 1 2016, 11:01 AM
bcraig committed rL277357: Improve shared_ptr dtor performance.
Improve shared_ptr dtor performance
Aug 1 2016, 10:59 AM
bcraig added a comment to D22470: [libcxx] Improve shared_ptr dtor performance.

I am going to submit the code changes and the tests independently. I'm having trouble getting cmake to use the right compiler for the libcxx-benchmarks target.

Aug 1 2016, 10:49 AM

Jul 21 2016

bcraig added inline comments to D22557: [libcxx] Diagnose invalid memory order arguments in <atomic>. Fixes PR21179..
Jul 21 2016, 7:13 AM

Jul 20 2016

bcraig added inline comments to D22557: [libcxx] Diagnose invalid memory order arguments in <atomic>. Fixes PR21179..
Jul 20 2016, 11:59 AM

Jul 19 2016

bcraig updated the diff for D22470: [libcxx] Improve shared_ptr dtor performance.

Added weak_ptr benchmark, as that's where the cost shifted.

Jul 19 2016, 8:10 AM

Jul 18 2016

bcraig added inline comments to D22292: [libunwind] Fix unw_getcontext for ARMv6-m.
Jul 18 2016, 12:25 PM
bcraig retitled D22470: [libcxx] Improve shared_ptr dtor performance from to [libcxx] Improve shared_ptr dtor performance.
Jul 18 2016, 11:31 AM

Jul 8 2016

bcraig added a comment to D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation.

LGTM (with a comment nit), but you'll need to get approval from @EricWF or @mclow.lists.

Jul 8 2016, 11:05 AM

Jul 6 2016

bcraig added a comment to D21968: [libcxx] Externally threaded libc++ variant - Take 2.

LGTM. As usual, you'll want to get one of the code owner's sign off first though.

Jul 6 2016, 6:36 AM

Jul 5 2016

bcraig added inline comments to D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation.
Jul 5 2016, 12:23 PM
bcraig abandoned D12535: [PR24643] Speeding up FoldingSetNodeID::AddPointer.
Jul 5 2016, 10:43 AM
bcraig added inline comments to D21968: [libcxx] Externally threaded libc++ variant - Take 2.
Jul 5 2016, 9:55 AM
bcraig added inline comments to D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation.
Jul 5 2016, 9:24 AM
bcraig added inline comments to D21991: [libunwind][ARM] Improve unwinder stack usage - Make WMMX support optional.
Jul 5 2016, 8:43 AM

Jun 30 2016

bcraig added a comment to D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation.

Also, can you add test cases for a lot of these things? I don't expect test cases for the DSO side of things, but a lot of the tricky atexit cases should be covered.

Jun 30 2016, 1:23 PM
bcraig added inline comments to D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation.
Jun 30 2016, 12:57 PM
bcraig added a comment to D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation.

On the topic of __cxa_thread_atexit, was it ever specified how it interacts with things like thread cancellation?

Jun 30 2016, 6:44 AM

Jun 29 2016

bcraig added a comment to D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation.

Hmm, maybe? If other global destructors run after ~DtorListHolder(), and they cause a thread_local to be initialized for the first time, __cxa_thread_atexit() might be called again. I was thinking that dtors would get re-initialized in that case but it appears it does not. So yeah, I think I'll need to leak the pthread_key_t.

I'm not sure how to avoid leaking the actual thread_local objects that get created in that situation. There's nothing left to trigger run_dtors() a second time.

Jun 29 2016, 1:06 PM
bcraig added a comment to D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation.

You should look at __thread_specific_ptr in libcxx's <thread>. It does a lot of these things in order to satisfy the requirements of notify_all_at_thread_exit, set_value_at_thread_exit, and make_ready_at_thread_exit.

Had a look at it. One thing that stands out is that notify_all_at_thread_exit() and friends are supposed to be invoked *after* thread_local destructors. But the order that pthread_key destructors run in is unspecified. This could be worked around by waiting for the second iteration through pthread_key destructors before triggering ~__thread_struct_imp(). It looks like libstdc++ has a similar bug if ..._impl() isn't available.

Jun 29 2016, 9:47 AM
bcraig updated subscribers of D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation.

You should look at __thread_specific_ptr in libcxx's <thread>. It does a lot of these things in order to satisfy the requirements of notify_all_at_thread_exit, set_value_at_thread_exit, and make_ready_at_thread_exit.

Jun 29 2016, 6:50 AM

Jun 23 2016

bcraig closed D21229: [Analyzer] Don't cache report generation ExplodedNodes.

Old: 106,657,666
New: 105,346,818

Jun 23 2016, 8:55 AM
bcraig committed rL273572: [Analyzer] Don't cache report generation ExplodedNodes.
[Analyzer] Don't cache report generation ExplodedNodes
Jun 23 2016, 8:54 AM
bcraig updated subscribers of D21637: [libcxx] Don't use pthread initializers in constexpr constructors.

@joerg. I think this has been fixed in newer versions of NetBSD.

Jun 23 2016, 7:27 AM

Jun 14 2016

bcraig added a comment to D21345: [libcxx] [test] Avoid huge main() functions and huge arrays..

LGTM. I don't see anything controversial here. If you want to wait for @EricWF or @mclow.lists though, that's fine.

Jun 14 2016, 2:27 PM
bcraig added a comment to D21345: [libcxx] [test] Avoid huge main() functions and huge arrays..

Even though they have artificial blocks in order to keep variables locally scoped, this gives MSVC's /analyze a headache, because it views main() as consuming an enormous amount of stack space (it doesn't reuse stack, at least for analysis). Splitting main() up into numbered test functions is slightly friendlier to humans and massively friendlier to /analyze.

Jun 14 2016, 2:20 PM