[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