[ccan] [PATCH] altstack: stack alignment and accounting tweaks

Dan Good dan at dancancode.com
Wed Feb 17 04:41:43 AEDT 2016


I see.  Thank you.

On Tue, Feb 16, 2016 at 5:04 AM David Gibson <david at gibson.dropbear.id.au>
wrote:

> On Mon, Feb 15, 2016 at 06:29:10PM +0000, Dan Good wrote:
> > I thought I understood from our emails at the end of last year that a
> > single "polished" commit was desirable.  Is that not the same thing as
> > taking several very small commits and bundling them into one?
>
> Often not, in practice.  The ultimate goal is reviewability (both
> before merging and when looking back at the commit history).  But
> that's often accomplished different ways depending on the situation.
> In particular it tends to be different for initial commits of new code
> from changes to existing code.
>
> For initial commits, it's generally best to elide patches which
> correct false steps made during development, or really anything which
> doesn't increase the total code size or complexity significantly.
> That gives less commits to review, and cleaner code in them to
> understand.
>
> For changes to existing code, however, the important thing is
> understanding why the change is desirable and then that the changes
> actually accomplish that goal.  That tends towards small, single
> purpose commits, so it's easy to see what changes belong to what goal
> (explained in the commit message).
>
> Of course, that's a very broad rule of tumb, with plenty of exceptions
> and wiggle room.
>
> > On Mon, Feb 15, 2016 at 5:53 AM David Gibson <
> david at gibson.dropbear.id.au>
> > wrote:
> >
> > > 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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/ccan/attachments/20160216/5a54fa2f/attachment-0001.html>


More information about the ccan mailing list