lgtm after the two comments are addressed.
================
Comment at: cmake/config-ix.cmake:340
@@ +339,3 @@
+if (SAFESTACK_SUPPORTED_ARCH AND
+ OS_NAME MATCHES "Darwin|Linux|FreeBSD")
+ set(COMPILER_RT_HAS_SAFESTACK TRUE)
----------------
pcc wrote:
> jfb wrote:
> > What's missing for Windows support? Thread and TLS support? It's the most
> > significant platform for Chrome (well, and Android), and we're looking
> > forward to LLVM support on Windows. Not having SafeStack would be sad. This
> > can be a different patch, but I think it's important.
> > What's missing for Windows support? Thread and TLS support?
>
> LLVM already supports TLS on Windows (I believe), but we will have to find
> some way to intercept thread creation to allocate both stacks.
>
> > This can be a different patch, but I think it's important.
>
> Agree.
Tagging @rnk FYI.
================
Comment at: lib/safestack/safestack.cc:61
@@ +60,3 @@
+static inline void unsafe_stack_setup(void *start, size_t size, size_t guard) {
+ void* stack_ptr = (char*) start + size;
+ CHECK_EQ((((size_t)stack_ptr) & (kStackAlign-1)), 0);
----------------
Overflow check here too, and with guard.
================
Comment at: lib/safestack/safestack.cc:174
@@ +173,3 @@
+ if (initialized)
+ return;
+
----------------
ksvladimir wrote:
> pcc wrote:
> > jfb wrote:
> > > Is this initialization check useful if `__attribute__((constructor(0)))`
> > > was used? Can this be done concurrently, or is it just called multiple
> > > times from the same thread of execution? If concurrent then
> > > initialization is racy.
> > > Is this initialization check useful if __attribute__((constructor(0)))
> > > was used?
> >
> > I don't think it is. Removed.
> >
> > (Regarding Vova's comment, we don't currently support linking SafeStack
> > into DSOs.)
> The primary reason for this check is to handle cases when libsafestack is
> linked into multiple DSOs and __safestack_init ends up being on multiple
> constructor lists. It can only be called concurrently if multiple DSOs are
> being dlopen'ed in parallel. I think that dlopen itself isn't thread-safe and
> shouldn't be used that way but, perhaps, for extra safety it's useful to make
> this code non-racy using atomics.
Gotcha. Would it be useful to have nothing here for now (@pcc just deleted the
code), but add a comment explaining @ksvladimir's point?
http://reviews.llvm.org/D6096
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
