compnerd closed this revision.
compnerd added a comment.
SVN r291333
Comment at: include/__threading_support:29-30
+#include
+#define WIN32_LEAN_AND_MEAN
+#include
+#include
majnemer wrote:
> EricWF wrote:
> > I think we agreed that we cannot use this macro.
EricWF accepted this revision.
EricWF added a comment.
This revision is now accepted and ready to land.
LGTM. I just successfully built this on Windows.
https://reviews.llvm.org/D28220
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://l
EricWF added inline comments.
Comment at: include/__threading_support:30
+#define WIN32_LEAN_AND_MEAN
+#include
+#include
EricWF wrote:
> smeenai wrote:
> > EricWF wrote:
> > > EricWF wrote:
> > > > smeenai wrote:
> > > > > EricWF wrote:
> > > > > > > Can we do
EricWF added inline comments.
Comment at: include/__threading_support:30
+#define WIN32_LEAN_AND_MEAN
+#include
+#include
smeenai wrote:
> EricWF wrote:
> > EricWF wrote:
> > > smeenai wrote:
> > > > EricWF wrote:
> > > > > > Can we do as Reid suggests and not
smeenai added inline comments.
Comment at: include/__threading_support:30
+#define WIN32_LEAN_AND_MEAN
+#include
+#include
EricWF wrote:
> EricWF wrote:
> > smeenai wrote:
> > > EricWF wrote:
> > > > > Can we do as Reid suggests and not expose users to windows.
EricWF added inline comments.
Comment at: include/__threading_support:30
+#define WIN32_LEAN_AND_MEAN
+#include
+#include
EricWF wrote:
> smeenai wrote:
> > EricWF wrote:
> > > > Can we do as Reid suggests and not expose users to windows.h?
> > >
> > > I was a
EricWF added inline comments.
Comment at: include/__threading_support:30
+#define WIN32_LEAN_AND_MEAN
+#include
+#include
smeenai wrote:
> EricWF wrote:
> > > Can we do as Reid suggests and not expose users to windows.h?
> >
> > I was about to ask the same que
smeenai added inline comments.
Comment at: include/__threading_support:30
+#define WIN32_LEAN_AND_MEAN
+#include
+#include
EricWF wrote:
> > Can we do as Reid suggests and not expose users to windows.h?
>
> I was about to ask the same question. These includes
compnerd removed rL LLVM as the repository for this revision.
compnerd updated this revision to Diff 83469.
compnerd marked 2 inline comments as done.
compnerd added a comment.
remove `WIN32_LEAN_AND_MEAN`, fix decoration, remove inline decorations
https://reviews.llvm.org/D28220
Files:
inclu
EricWF added inline comments.
Comment at: include/__threading_support:30
+#define WIN32_LEAN_AND_MEAN
+#include
+#include
> Can we do as Reid suggests and not expose users to windows.h?
I was about to ask the same question. These includes are dragging in the
EricWF added inline comments.
Comment at: include/__threading_support:580
+int __libcpp_tls_create(__libcpp_tls_key* __key,
+void(_LIBCPP_TLS_DESTRUCTOR_CC* __at_exit)(void*))
+{
The `_LIBCPP_TLS_DESTRUCTOR_CC` needs to appear an the initi
majnemer added inline comments.
Comment at: include/__threading_support:29-30
+#include
+#define WIN32_LEAN_AND_MEAN
+#include
+#include
EricWF wrote:
> I think we agreed that we cannot use this macro.
Can we do as Reid suggests and not expose users to windows
EricWF added inline comments.
Comment at: include/__threading_support:29
+#include
+#define WIN32_LEAN_AND_MEAN
+#include
I think we agreed that we cannot use this macro.
Comment at: include/__threading_support:426
+// Condition Variable
+_LI
EricWF added a comment.
Just tried the updated patch and I still get one build error:
C:\Users\Eric\workspace\libcxx\src\thread.cpp(135,16): error: use of
undeclared identifier 'nanosleep'
while (nanosleep(&ts, &ts) == -1 && errno == EINTR)
Repository:
rL LLVM
https://reviews.l
compnerd set the repository for this revision to rL LLVM.
compnerd updated this revision to Diff 83453.
Repository:
rL LLVM
https://reviews.llvm.org/D28220
Files:
include/__config
include/__threading_support
include/thread
Index: include/thread
==
EricWF added a comment.
I'm seeing the following build errors on Windows:
C:\Users\Eric\workspace\libcxx\include\__threading_support(521,3): warning:
cannot delete expression with pointer-to-'void' type 'void *'
[-Wdelete-incomplete]
delete __data;
^ ~~
C:\Users\Eric\works
halyavin added a comment.
LGTM.
Comment at: include/__threading_support:480
+ if (!SleepConditionVariableSRW(__cv, __m,
+ timeout_ms.count() > 0 ? timeout_ms.count()
+: 0,
compnerd removed rL LLVM as the repository for this revision.
compnerd updated this revision to Diff 83432.
compnerd added a comment.
rebase
https://reviews.llvm.org/D28220
Files:
include/__config
include/__threading_support
include/thread
Index: include/thread
==
compnerd updated this revision to Diff 83430.
compnerd marked an inline comment as done.
compnerd added a comment.
rebase, address negative timeouts, fix thunking for `__libcpp_tls_create`
Repository:
rL LLVM
https://reviews.llvm.org/D28220
Files:
include/__config
include/__threading_sup
EricWF requested changes to this revision.
EricWF added a comment.
This revision now requires changes to proceed.
This needs to be rebased following the `<__threading_support>` cleanup in
r291275.
Repository:
rL LLVM
https://reviews.llvm.org/D28220
halyavin added inline comments.
Comment at: include/__threading_support:474
+
system_clock::time_point(duration_cast(duration));
+ auto timeout_ms = duration_cast(abstime - system_clock::now());
+
compnerd wrote:
> halyavin wrote:
> > Since negative timeou
compnerd marked an inline comment as done.
compnerd added inline comments.
Comment at: include/__threading_support:474
+
system_clock::time_point(duration_cast(duration));
+ auto timeout_ms = duration_cast(abstime - system_clock::now());
+
halyavin wrote:
halyavin added inline comments.
Comment at: include/__threading_support:474
+
system_clock::time_point(duration_cast(duration));
+ auto timeout_ms = duration_cast(abstime - system_clock::now());
+
Since negative timeouts can't be avoided, we must make sure
EricWF accepted this revision.
EricWF added a comment.
This revision is now accepted and ready to land.
This LGTM. I'm sure we can flush out any bugs once we get the tests running.
Repository:
rL LLVM
https://reviews.llvm.org/D28220
___
cfe-comm
compnerd updated this revision to Diff 83182.
compnerd marked an inline comment as done.
compnerd added a comment.
Fix `__libcpp_condvar_timedwait` parameter usage (absolute vs relative time)
Repository:
rL LLVM
https://reviews.llvm.org/D28220
Files:
include/__config
include/__threading_
compnerd marked an inline comment as done.
compnerd added inline comments.
Comment at: include/__threading_support:458
+ __libcpp_mutex_reference&& __m,
+ timespec* __ts)
+{
halyavin wrote:
> In posix, p
compnerd updated this revision to Diff 83178.
compnerd added a comment.
Address a number of review comments.
Repository:
rL LLVM
https://reviews.llvm.org/D28220
Files:
include/__config
include/__threading_support
Index: include/__threading_support
===
compnerd marked 6 inline comments as done.
compnerd added inline comments.
Comment at: include/__threading_support:527
+static inline _LIBCPP_ALWAYS_INLINE unsigned int WINAPI
+__libcpp_thread_trampoline(void *__data)
+{
rnk wrote:
> halyavin wrote:
> > Trampolin
compnerd updated the summary for this revision.
compnerd updated this revision to Diff 83174.
Repository:
rL LLVM
https://reviews.llvm.org/D28220
Files:
include/__config
include/__threading_support
Index: include/__threading_support
rnk added inline comments.
Comment at: include/__threading_support:44
+#define WIN32_LEAN_AND_MEAN
+#define VC_EXTRA_LEAN
+#include
kimgr wrote:
> EricWF wrote:
> > compnerd wrote:
> > > EricWF wrote:
> > > > Do these definitions have any affect when `` has alre
halyavin added inline comments.
Comment at: include/__threading_support:458
+ __libcpp_mutex_reference&& __m,
+ timespec* __ts)
+{
In posix, pthread_cond_timedwait uses absolute time but we use relative
compnerd updated this revision to Diff 82954.
compnerd added a comment.
Remove incorrect use of `VC_EXTRALEAN`. Fix signature for
`__libcpp_set_thread_specific`. Provide CC adjustment thunks for the thread
creation. Add an assertion for sleep timeout truncation. Implement exec-once
interfac
halyavin added inline comments.
Comment at: include/__threading_support:421
+if (!SleepConditionVariableCS(__cv, __m,
+
duration_cast(timeout).count()))
+ return GetLastError();
We can have integer overflow on cast to DW
awson added a comment.
You don't need #define VC_EXTRA_LEAN since:
1. VC_EXTRA_LEAN is a mistype, only VC_EXTRALEAN has some meaning in Windows
ecosystem;
2. VC_EXTRALEAN is used *only* in MFC headers which obviously aren't used in
clang codebase.
Thus it would be better to remove this altoget
kimgr added a comment.
Re Eric's windows.h concern.
Comment at: include/__threading_support:44
+#define WIN32_LEAN_AND_MEAN
+#define VC_EXTRA_LEAN
+#include
EricWF wrote:
> compnerd wrote:
> > EricWF wrote:
> > > Do these definitions have any affect when `` ha
EricWF added inline comments.
Comment at: include/__threading_support:44
+#define WIN32_LEAN_AND_MEAN
+#define VC_EXTRA_LEAN
+#include
compnerd wrote:
> EricWF wrote:
> > Do these definitions have any affect when `` has already been
> > included?
> > Also are t
compnerd added inline comments.
Comment at: include/__threading_support:46
+inline _LIBCPP_INLINE_VISIBILITY
+int __libcpp_recursive_mutex_init(__libcpp_mutex_t *__m);
+
EricWF wrote:
> The forward declarations of the `__libcpp_` threading wrapper should be
> sh
compnerd updated this revision to Diff 82846.
compnerd added a comment.
add more context
Repository:
rL LLVM
https://reviews.llvm.org/D28220
Files:
include/__config
include/__threading_support
Index: include/__threading_support
===
compnerd updated this revision to Diff 82843.
compnerd added a comment.
update for separation of mutex and recursive_mutex.
Repository:
rL LLVM
https://reviews.llvm.org/D28220
Files:
include/__config
include/__threading_support
Index: include/__threading_support
EricWF added inline comments.
Comment at: include/__threading_support:44
+#define WIN32_LEAN_AND_MEAN
+#define VC_EXTRA_LEAN
+#include
Do these definitions have any affect when `` has already been
included?
Also are these definitions required before including t
smeenai added inline comments.
Comment at: include/__threading_support:300-305
+int __libcpp_recursive_mutex_init(__libcpp_mutex_t *__m)
+{
+ InitializeSRWLock(__m);
+ return 0;
+}
+
majnemer wrote:
> compnerd wrote:
> > majnemer wrote:
> > > I don't think you
smeenai added inline comments.
Comment at: include/__threading_support:83
+} __libcpp_mutex_t;
+#define _LIBCPP_MUTEX_INITIALIZER {{0}, SRWLOCK_INIT, 0}
+
Why not a tagged union?
Repository:
rL LLVM
https://reviews.llvm.org/D28220
majnemer added inline comments.
Comment at: include/__threading_support:300-305
+int __libcpp_recursive_mutex_init(__libcpp_mutex_t *__m)
+{
+ InitializeSRWLock(__m);
+ return 0;
+}
+
compnerd wrote:
> majnemer wrote:
> > I don't think you can use slim rw locks
EricWF added a comment.
Could you upload this patch with more context?
Repository:
rL LLVM
https://reviews.llvm.org/D28220
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
compnerd updated this revision to Diff 82834.
compnerd added a comment.
switch between a CRITICAL_SECTION and SRWLOCK
Repository:
rL LLVM
https://reviews.llvm.org/D28220
Files:
include/__config
include/__threading_support
Index: include/__threading_support
==
compnerd added inline comments.
Comment at: include/__threading_support:300-305
+int __libcpp_recursive_mutex_init(__libcpp_mutex_t *__m)
+{
+ InitializeSRWLock(__m);
+ return 0;
+}
+
majnemer wrote:
> I don't think you can use slim rw locks for recursive locks
majnemer added inline comments.
Comment at: include/__threading_support:300-305
+int __libcpp_recursive_mutex_init(__libcpp_mutex_t *__m)
+{
+ InitializeSRWLock(__m);
+ return 0;
+}
+
I don't think you can use slim rw locks for recursive locks. I think we will
compnerd updated this revision to Diff 82826.
compnerd added a comment.
Use SRW locks, rebase for avoid redeclaration of interfaces, remove static
initialization check removals.
Repository:
rL LLVM
https://reviews.llvm.org/D28220
Files:
include/__config
include/__threading_support
Inde
compnerd marked 3 inline comments as done.
compnerd added a comment.
The SRW initializers avoid the initializer changes, so its one less set of
changes that is necessary.
Comment at: include/__threading_support:33
#include <__external_threading>
+#elif defined(_WIN32) && defi
majnemer added a comment.
In https://reviews.llvm.org/D28220#633622, @compnerd wrote:
> @majnemer Im using the Fls* APIs since they provide the thread termination
> callback, which Tls* doesn't. Good point about the SRW. Those are newer,
> but, we can always provide a fallback later.
I don'
compnerd added a comment.
@majnemer Im using the Fls* APIs since they provide the thread termination
callback, which Tls* doesn't. Good point about the SRW. Those are newer, but,
we can always provide a fallback later.
Repository:
rL LLVM
https://reviews.llvm.org/D28220
___
EricWF added a comment.
Added inline comments.
The main issue is that this patch needs to use the same `<__threading_support>`
forward declarations as every other supported threading API; Only new
definitions should be provided.
Comment at: include/__threading_support:33
#i
majnemer added a comment.
slim reader-writer locks are faster than critical sections, I'd recommend your
implementation switch to those.
Also, why do you use Fls instead of Tls?
Repository:
rL LLVM
https://reviews.llvm.org/D28220
___
cfe-commit
53 matches
Mail list logo
