[PATCH] D38250: [libunwind] Implement the Get/SetTopOfFunctionStack functions via a __thread TLS variable

2017-10-01 Thread Martin Storsjö via Phabricator via cfe-commits 
mstorsjo abandoned this revision.
mstorsjo added a comment.

Works fine with the version from r314632 (with a minor fixup in r314635).


https://reviews.llvm.org/D38250



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

[PATCH] D38250: [libunwind] Implement the Get/SetTopOfFunctionStack functions via a __thread TLS variable

2017-10-01 Thread Saleem Abdulrasool via Phabricator via cfe-commits 
compnerd added a comment.

I believe that SVN r314632 should address the issue more thorougly.


https://reviews.llvm.org/D38250



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

[PATCH] D38250: [libunwind] Implement the Get/SetTopOfFunctionStack functions via a __thread TLS variable

2017-10-01 Thread Martin Storsjö via Phabricator via cfe-commits 
mstorsjo added inline comments.



Comment at: src/Unwind-sjlj.c:481
+#endif // !defined(__APPLE__)
+
 #endif // defined(_LIBUNWIND_BUILD_SJLJ_APIS)

compnerd wrote:
> mstorsjo wrote:
> > compnerd wrote:
> > > Can't both of these also be static?
> > No, since they're declared earlier as non-static.
> Yeah, that is a bug :-).  `__Unwind_SjLj_GetTopOfFunctionStack` and 
> `__Unwind_SjLj_SetToOfFunctionStack` are implementation details of LLVM's 
> libunwind.  They are not part of the public interfaces, and are not used 
> outside of this TU, and should be marked as static as such.  I think that 
> changing the prototype declaration to indicate this is reasonable.  I suppose 
> that I can make that change separately.
Yes, except that in the apple case, those functions are provided by another TU 
(Unwind_AppleExtras.cpp). So in that case, those functions only should be 
static in the non-apple case.


https://reviews.llvm.org/D38250



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

[PATCH] D38250: [libunwind] Implement the Get/SetTopOfFunctionStack functions via a __thread TLS variable

2017-10-01 Thread Saleem Abdulrasool via Phabricator via cfe-commits 
compnerd requested changes to this revision.
compnerd added a comment.
This revision now requires changes to proceed.

Actually, Ill go ahead and make the more invasive change and that will provide 
this properly.


https://reviews.llvm.org/D38250



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

[PATCH] D38250: [libunwind] Implement the Get/SetTopOfFunctionStack functions via a __thread TLS variable

2017-10-01 Thread Saleem Abdulrasool via Phabricator via cfe-commits 
compnerd accepted this revision.
compnerd added inline comments.
This revision is now accepted and ready to land.



Comment at: src/Unwind-sjlj.c:481
+#endif // !defined(__APPLE__)
+
 #endif // defined(_LIBUNWIND_BUILD_SJLJ_APIS)

mstorsjo wrote:
> compnerd wrote:
> > Can't both of these also be static?
> No, since they're declared earlier as non-static.
Yeah, that is a bug :-).  `__Unwind_SjLj_GetTopOfFunctionStack` and 
`__Unwind_SjLj_SetToOfFunctionStack` are implementation details of LLVM's 
libunwind.  They are not part of the public interfaces, and are not used 
outside of this TU, and should be marked as static as such.  I think that 
changing the prototype declaration to indicate this is reasonable.  I suppose 
that I can make that change separately.


https://reviews.llvm.org/D38250



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

[PATCH] D38250: [libunwind] Implement the Get/SetTopOfFunctionStack functions via a __thread TLS variable

2017-09-28 Thread Martin Storsjö via Phabricator via cfe-commits 
mstorsjo updated this revision to Diff 117089.
mstorsjo edited the summary of this revision.
mstorsjo added a comment.

Made the tls variable static, changed ifndef into !defined().


https://reviews.llvm.org/D38250

Files:
  src/Unwind-sjlj.c


Index: src/Unwind-sjlj.c
===
--- src/Unwind-sjlj.c
+++ src/Unwind-sjlj.c
@@ -465,4 +465,18 @@
   return 0;
 }
 
+#if !defined(__APPLE__)
+static __thread struct _Unwind_FunctionContext *stack = NULL;
+
+_LIBUNWIND_HIDDEN
+struct _Unwind_FunctionContext *__Unwind_SjLj_GetTopOfFunctionStack() {
+  return stack;
+}
+
+_LIBUNWIND_HIDDEN
+void __Unwind_SjLj_SetTopOfFunctionStack(struct _Unwind_FunctionContext *fc) {
+  stack = fc;
+}
+#endif // !defined(__APPLE__)
+
 #endif // defined(_LIBUNWIND_BUILD_SJLJ_APIS)


Index: src/Unwind-sjlj.c
===
--- src/Unwind-sjlj.c
+++ src/Unwind-sjlj.c
@@ -465,4 +465,18 @@
   return 0;
 }
 
+#if !defined(__APPLE__)
+static __thread struct _Unwind_FunctionContext *stack = NULL;
+
+_LIBUNWIND_HIDDEN
+struct _Unwind_FunctionContext *__Unwind_SjLj_GetTopOfFunctionStack() {
+  return stack;
+}
+
+_LIBUNWIND_HIDDEN
+void __Unwind_SjLj_SetTopOfFunctionStack(struct _Unwind_FunctionContext *fc) {
+  stack = fc;
+}
+#endif // !defined(__APPLE__)
+
 #endif // defined(_LIBUNWIND_BUILD_SJLJ_APIS)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38250: [libunwind] Implement the Get/SetTopOfFunctionStack functions via a __thread TLS variable

2017-09-28 Thread Martin Storsjö via Phabricator via cfe-commits 
mstorsjo added inline comments.



Comment at: src/Unwind-sjlj.c:468
 
+#ifndef __APPLE__
+__thread struct _Unwind_FunctionContext *stack = NULL;

compnerd wrote:
> I would prefer:
> 
> #if !defined(__APPLE__)
Sure, I can change that.



Comment at: src/Unwind-sjlj.c:469
+#ifndef __APPLE__
+__thread struct _Unwind_FunctionContext *stack = NULL;
+

compnerd wrote:
> Please make this `static`.
Oh, indeed, yes, I'll change that.



Comment at: src/Unwind-sjlj.c:481
+#endif // !defined(__APPLE__)
+
 #endif // defined(_LIBUNWIND_BUILD_SJLJ_APIS)

compnerd wrote:
> Can't both of these also be static?
No, since they're declared earlier as non-static.


https://reviews.llvm.org/D38250



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

[PATCH] D38250: [libunwind] Implement the Get/SetTopOfFunctionStack functions via a __thread TLS variable

2017-09-28 Thread Saleem Abdulrasool via Phabricator via cfe-commits 
compnerd added a comment.

Ugh, I was mixing up `_Unwind_SjLj_Register` with this function.




Comment at: src/Unwind-sjlj.c:468
 
+#ifndef __APPLE__
+__thread struct _Unwind_FunctionContext *stack = NULL;

I would prefer:

#if !defined(__APPLE__)



Comment at: src/Unwind-sjlj.c:469
+#ifndef __APPLE__
+__thread struct _Unwind_FunctionContext *stack = NULL;
+

Please make this `static`.



Comment at: src/Unwind-sjlj.c:481
+#endif // !defined(__APPLE__)
+
 #endif // defined(_LIBUNWIND_BUILD_SJLJ_APIS)

Can't both of these also be static?


https://reviews.llvm.org/D38250



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

[PATCH] D38250: [libunwind] Implement the Get/SetTopOfFunctionStack functions via a __thread TLS variable

2017-09-26 Thread Martin Storsjö via Phabricator via cfe-commits 
mstorsjo added a comment.

In https://reviews.llvm.org/D38250#880871, @compnerd wrote:

> I have a complete implementation for this which is known to work on Windows 
> at least.


Patches for libunwind, or standalone? In any case, please do share if you think 
it's relevant.

> The only thing that `__thread` requires is a working linker (which does not 
> include binutils -- I do have a local patch to make that work though).

Ok, thanks for clarifying. (If I read wikipedia right, it requires Vista if 
built into a DLL that isn't linked to by an executable but loaded via 
LoadLibrary though.)

> Are these two the only ones that are missing?

As far as I've seen in my tests, these are the only ones missing yes. Most of 
the rest of `Unwind_AppleExtras.cpp` contains things not relating to SJLJ.

> At the very least, the implementation of `SetTopOfFunctionStack` is incorrect.

In which way? My implementation does pretty much exactly the same as the apple 
version of `SetTopOfFunctionStack`, but using a `__thread` variable instead of 
`_pthread_setspecific_direct(__PTK_LIBC_DYLD_Unwind_SjLj_Key, fc);`.


https://reviews.llvm.org/D38250



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

[PATCH] D38250: [libunwind] Implement the Get/SetTopOfFunctionStack functions via a __thread TLS variable

2017-09-26 Thread Saleem Abdulrasool via Phabricator via cfe-commits 
compnerd requested changes to this revision.
compnerd added a comment.
This revision now requires changes to proceed.

I have a complete implementation for this which is known to work on Windows at 
least.  The only thing that `__thread` requires is a working linker (which does 
not include binutils -- I do have a local patch to make that work though).  Are 
these two the only ones that are missing?  At the very least, the 
implementation of `SetTopOfFunctionStack` is incorrect.


https://reviews.llvm.org/D38250



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

[PATCH] D38250: [libunwind] Implement the Get/SetTopOfFunctionStack functions via a __thread TLS variable

2017-09-25 Thread Martin Storsjö via Phabricator via cfe-commits 
mstorsjo created this revision.

When targeting apple platforms, these functions are implemented in
Unwind_AppleExtras.cpp, but there's previously no implementation for
other platforms.

Does `__thread` have any specific runtime requirements on e.g. windows?


https://reviews.llvm.org/D38250

Files:
  src/Unwind-sjlj.c


Index: src/Unwind-sjlj.c
===
--- src/Unwind-sjlj.c
+++ src/Unwind-sjlj.c
@@ -465,4 +465,18 @@
   return 0;
 }
 
+#ifndef __APPLE__
+__thread struct _Unwind_FunctionContext *stack = NULL;
+
+_LIBUNWIND_HIDDEN
+struct _Unwind_FunctionContext *__Unwind_SjLj_GetTopOfFunctionStack() {
+  return stack;
+}
+
+_LIBUNWIND_HIDDEN
+void __Unwind_SjLj_SetTopOfFunctionStack(struct _Unwind_FunctionContext *fc) {
+  stack = fc;
+}
+#endif // !defined(__APPLE__)
+
 #endif // defined(_LIBUNWIND_BUILD_SJLJ_APIS)


Index: src/Unwind-sjlj.c
===
--- src/Unwind-sjlj.c
+++ src/Unwind-sjlj.c
@@ -465,4 +465,18 @@
   return 0;
 }
 
+#ifndef __APPLE__
+__thread struct _Unwind_FunctionContext *stack = NULL;
+
+_LIBUNWIND_HIDDEN
+struct _Unwind_FunctionContext *__Unwind_SjLj_GetTopOfFunctionStack() {
+  return stack;
+}
+
+_LIBUNWIND_HIDDEN
+void __Unwind_SjLj_SetTopOfFunctionStack(struct _Unwind_FunctionContext *fc) {
+  stack = fc;
+}
+#endif // !defined(__APPLE__)
+
 #endif // defined(_LIBUNWIND_BUILD_SJLJ_APIS)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits