[ccan] [PATCH] altstack: stack alignment and accounting tweaks
David Gibson
david at gibson.dropbear.id.au
Mon Feb 15 21:54:44 AEDT 2016
On Wed, Feb 03, 2016 at 11:42:11PM +0000, Dan Good wrote:
> "messy bitser" - what does that mean? That's bad, isn't it?
It's... mildly bad.
By "bitser" I mean that it contains a number of changes that aren't
closely related to each other. In general that makes a patch more
difficult to review. How important that is depends on the complexity
of the changes and how mature the code being altered is, so it's not
really a big deal.
The fact that altstack has been breaking Travis builds for a while
probably predisposed me to be uncharitable.
> Can I help fix the builds?
So, I've figured out what the problem is, patch coming shortly.
>
> On Wed, Feb 3, 2016 at 4:28 PM David Gibson <david at gibson.dropbear.id.au>
> wrote:
>
> > On Mon, Feb 01, 2016 at 02:43:16AM +0000, Dan Good wrote:
> > >
> > > * add altstack_remn, returns amount of stack remaining
> > > * increase mapping by 1 page to handle abutment case
> > > * capture rsp earlier
> > > * align stack to 16 bytes
> > >
> > > Signed-off-by: Dan Good <dan at dancancode.com>
> >
> > Bit of a messy bitser, but doesn't look it will make anything
> > worse. so go ahead and push.
> >
> > Unfortunately, altstack is breaking travis builds at the moment (see,
> > e.g. https://travis-ci.org/dgibson/ccan/jobs/106661592) - but not
> > local builds for me which is making it difficult to debug.
> >
> > Some investigation showed that the test prog was getting a SIGBUS, but
> > I didn't get further than that.
> >
> > altstack also breaks "make all" on any non-x86_64 platform (including
> > 32-bit x86) which is pretty horrid. For that I think we need to
> > adjust the makefiles to look at rusty's new "ported" stuff from _info.
> >
> >
> > > ---
> > > ccan/altstack/altstack.c | 17 ++++++++++++++---
> > > ccan/altstack/altstack.h | 14 ++++++++++++++
> > > ccan/altstack/test/run.c | 20 +++++++++++++-------
> > > 3 files changed, 41 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/ccan/altstack/altstack.c b/ccan/altstack/altstack.c
> > > index 6faf38f..b71c64f 100644
> > > --- a/ccan/altstack/altstack.c
> > > +++ b/ccan/altstack/altstack.c
> > > @@ -8,6 +8,7 @@
> > > #include <signal.h>
> > > #include <stdio.h>
> > > #include <string.h>
> > > +#include <unistd.h>
> > > #include <sys/mman.h>
> > >
> > > static __thread char ebuf[ALTSTACK_ERR_MAXLEN];
> > > @@ -37,6 +38,11 @@ static void segvjmp(int signum)
> > > }
> > >
> > > static __thread void *rsp_save_[2];
> > > +static __thread rlim_t max_;
> > > +
> > > +rlim_t altstack_max(void) {
> > > + return max_;
> > > +}
> > >
> > > static ptrdiff_t rsp_save(unsigned i) {
> > > assert(i < 2);
> > > @@ -57,6 +63,7 @@ static __thread void *arg_, *out_;
> > >
> > > int altstack(rlim_t max, void *(*fn)(void *), void *arg, void **out)
> > > {
> > > + long pgsz = sysconf(_SC_PAGESIZE);
> > > int ret = -1, undo = 0;
> > > char *m;
> > > struct rlimit rl_save;
> > > @@ -69,11 +76,16 @@ int altstack(rlim_t max, void *(*fn)(void *), void
> > *arg, void **out)
> > > fn_ = fn;
> > > arg_ = arg;
> > > out_ = 0;
> > > + max_ = max;
> > > ebuf[elen = 0] = '\0';
> > > if (out) *out = 0;
> > >
> > > + // if the first page below the mapping is in use, we get max-pgsz
> > usable bytes
> > > + // add pgsz to max to guarantee at least max usable bytes
> > > + max += pgsz;
> > > +
> > > ok(getrlimit(RLIMIT_STACK, &rl_save), 1);
> > > - ok(setrlimit(RLIMIT_STACK, &(struct rlimit) { max,
> > rl_save.rlim_max }), 1);
> > > + ok(setrlimit(RLIMIT_STACK, &(struct rlimit) { max_,
> > rl_save.rlim_max }), 1);
> > > undo++;
> > >
> > > ok(m = mmap(0, max, PROT_READ|PROT_WRITE,
> > MAP_PRIVATE|MAP_ANONYMOUS|MAP_GROWSDOWN|MAP_NORESERVE, -1, 0), 1);
> > > @@ -91,8 +103,7 @@ int altstack(rlim_t max, void *(*fn)(void *), void
> > *arg, void **out)
> > > ok(sigaction(SIGSEGV, &sa, &sa_save), 1);
> > > undo++;
> > >
> > > - asm volatile ("movq %%rsp, %%r10\nmov %0, %%rsp\npush
> > %%r10" : : "g" (m + max) : "r10");
> > > - rsp_save(0);
> > > + asm volatile ("movq %%rsp, %%r10\nmov %1, %%rsp\nmov
> > %%rsp, %0\nsub $8, %%rsp\npush %%r10" : "=g" (rsp_save_[0]) : "g" (m + max)
> > : "r10");
> > > out_ = fn_(arg_);
> > > asm volatile ("pop %rsp");
> > > ret = 0;
> > > diff --git a/ccan/altstack/altstack.h b/ccan/altstack/altstack.h
> > > index 5570e7b..4445a2a 100644
> > > --- a/ccan/altstack/altstack.h
> > > +++ b/ccan/altstack/altstack.h
> > > @@ -104,6 +104,20 @@ char *altstack_geterr(void);
> > > ptrdiff_t altstack_used(void);
> > >
> > > /**
> > > + * altstack_max - return usable stack size
> > > + *
> > > + * Returns: max value from altstack() call
> > > + */
> > > +rlim_t altstack_max(void);
> > > +
> > > +/**
> > > + * altstack_remn - return amount of stack remaining
> > > + *
> > > + * Returns: altstack_max() minus altstack_used()
> > > + */
> > > +#define altstack_remn() (altstack_max() - altstack_used())
> > > +
> > > +/**
> > > * altstack_rsp_save - set initial rsp value
> > > *
> > > * Capture the current value of rsp for future altstack_used()
> > > diff --git a/ccan/altstack/test/run.c b/ccan/altstack/test/run.c
> > > index adc1020..d0b8d28 100644
> > > --- a/ccan/altstack/test/run.c
> > > +++ b/ccan/altstack/test/run.c
> > > @@ -4,6 +4,7 @@
> > > #include <setjmp.h>
> > > #include <signal.h>
> > > #include <string.h>
> > > +#include <unistd.h>
> > > #include <sys/mman.h>
> > > #include <ccan/tap/tap.h>
> > > #include <ccan/altstack/altstack.h>
> > > @@ -20,13 +21,13 @@ enum {
> > > };
> > > int fail, call1, call2;
> > > char *m_;
> > > -rlim_t max_;
> > > +rlim_t msz_;
> > > #define e(x) (900+(x))
> > > #define seterr(x) (errno = e(x))
> > > #define setcall(x) ((call1 |= !errno ? (x) : 0), (call2 |= errno ||
> > out_ ? (x) : 0))
> > > #define getrlimit(...) (fail&getrlimit_ ?
> > (seterr(getrlimit_), -1) : (setcall(getrlimit_),
> > getrlimit(__VA_ARGS__)))
> > > #define mmap(...) (fail&mmap_ ? (seterr(mmap_),
> > (void *)-1) : (setcall(mmap_), mmap(__VA_ARGS__)))
> > > -#define munmap(a, b) (fail&munmap_ ?
> > (seterr(munmap_), -1) : (setcall(munmap_),
> > munmap(m_=(a), max_=(b))))
> > > +#define munmap(a, b) (fail&munmap_ ?
> > (seterr(munmap_), -1) : (setcall(munmap_),
> > munmap(m_=(a), msz_=(b))))
> > > #define setrlimit(...) (fail&setrlimit_ ?
> > (seterr(setrlimit_), -1) : (setcall(setrlimit_),
> > setrlimit(__VA_ARGS__)))
> > > #define sigaltstack(...) (fail&sigaltstack_ ?
> > (seterr(sigaltstack_), -1) : (setcall(sigaltstack_),
> > sigaltstack(__VA_ARGS__)))
> > > #define sigaction(...) (fail&sigaction_ ?
> > (seterr(sigaction_), -1) : (setcall(sigaction_),
> > sigaction(__VA_ARGS__)))
> > > @@ -58,7 +59,9 @@ static void *wrap(void *i)
> > >
> > > int main(void)
> > > {
> > > - plan_tests(16);
> > > + long pgsz = sysconf(_SC_PAGESIZE);
> > > +
> > > + plan_tests(17);
> > >
> > > #define chkfail(x, y, z, c1, c2) (call1 = 0, call2 = 0, errno = 0,
> > ok1((fail = x) && (y) && errno == (z) && call1 == (c1) && call2 == (c2)));
> > > #define chkok( y, z, c1, c2) (call1 = 0, call2 = 0, errno = 0, fail
> > = 0, ok1((y) && errno == (z) && call1 == (c1) && call2 == (c2)));
> > > @@ -86,7 +89,7 @@ int main(void)
> > > chkfail(munmap_, altstack(8*MiB, wrap, 0, 0) == 1,
> > e(munmap_),
> > > getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > > setrlimit_|sigaltstack_|sigaction_);
> > > - if (fail = 0, munmap(m_, max_) == -1)
> > > + if (fail = 0, munmap(m_, msz_) == -1)
> > > err(1, "munmap");
> > >
> > > chkok( altstack(1*MiB, wrap, (void *) 1000000, 0)
> > == -1, EOVERFLOW,
> > > @@ -102,10 +105,12 @@ int main(void)
> > > chkfail(munmap_, altstack(1*MiB, wrap, (void *) 1000000, 0)
> > == -1, EOVERFLOW,
> > > getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > > setrlimit_|sigaltstack_|sigaction_);
> > > - if (fail = 0, munmap(m_, max_) == -1)
> > > + if (fail = 0, munmap(m_, msz_) == -1)
> > > err(1, "munmap");
> > >
> > > - ok1(used > 1*MiB-1*KiB && used < 1*MiB);
> > > + ok1(altstack_max() == 1*MiB);
> > > + diag("used: %lu", used);
> > > + ok1(used >= 1*MiB && used <= 1*MiB + pgsz);
> > >
> > > char *p;
> > > for(p = altstack_geterr(); *p; p++)
> > > @@ -128,7 +133,8 @@ int main(void)
> > > getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > > setrlimit_|munmap_|sigaltstack_|sigaction_);
> > >
> > > - ok1(used > 8*MiB-8*KiB && used < 8*MiB);
> > > + diag("used: %lu", used);
> > > + ok1(used >= 8*MiB && used <= 8*MiB + pgsz);
> > >
> > > used = 0;
> > > chkok( altstack(8*MiB, wrap, (void *) 100000, 0)
> > == 0, 0,
> >
> >
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/ccan/attachments/20160215/a703388f/attachment.sig>
More information about the ccan
mailing list