code review 88190043: liblink, cmd/ld: reenable nosplit checking and test

src/cmd/ld/lib.c

 // Derived from Inferno utils/6l/obj.c and utils/6l/span.c
 // http://code.google.com/p/inferno-os/source/browse/utils/6l/obj.c
 // http://code.google.com/p/inferno-os/source/browse/utils/6l/span.c
 //
 //      Copyright © 1994-1999 Lucent Technologies Inc.  All rights reserved.
 //      Portions Copyright © 1995-1997 C H Forsyth (forsyth@terzarima.net)
 //      Portions Copyright © 1997-1999 Vita Nuova Limited
 //      Portions Copyright © 2000-2007 Vita Nuova Holdings Limited (www.vitanuova.com)
 //      Portions Copyright © 2004,2006 Bruce Ellis
 //      Portions Copyright © 2005-2007 C H Forsyth (forsyth@terzarima.net)
@@ skipping 999 matching lines @@
         if(thechar == '5')
                 return 0;
         return RegSize;
 }
 
 void
 dostkcheck(void)
 {
         Chain ch;
         LSym *s;
-        
+
         morestack = linklookup(ctxt, "runtime.morestack", 0);
         newstack = linklookup(ctxt, "runtime.newstack", 0);
 
-        // TODO
         // First the nosplits on their own.
         for(s = ctxt->textp; s != nil; s = s->next) {
-                if(s->text == nil || s->text->link == nil || (ctxt->arch->textflag(s->text) & NOSPLIT) == 0)
+                if(s->nosplit == 0)
                         continue;
                 ctxt->cursym = s;
                 ch.up = nil;
                 ch.sym = s;
                 ch.limit = StackLimit - callsize();
                 stkcheck(&ch, 0);
                 s->stkcheck = 1;

[iant, 2014/04/16 21:29:09]

I don't think you need to set the stkcheck field here, it will be set in the
stkcheck function.

[rsc, 2014/04/16 22:59:16]

Done.

         }
 ········
         // Check calling contexts.
         // Some nosplits get called a little further down,
         // like newproc and deferproc.  We could hard-code
         // that knowledge but it's more robust to look at
         // the actual call sites.
         for(s = ctxt->textp; s != nil; s = s->next) {
-                if(s->text == nil || s->text->link == nil || (ctxt->arch->textflag(s->text) & NOSPLIT) != 0)
+                if(s->nosplit != 0)
                         continue;
                 ctxt->cursym = s;
                 ch.up = nil;
                 ch.sym = s;
                 ch.limit = StackLimit - callsize();
                 stkcheck(&ch, 0);
         }
 }
 
 static int
 stkcheck(Chain *up, int depth)
 {
         Chain ch, ch1;
-        Prog *p;
         LSym *s;
-        int limit, prolog;
+        int limit, prolog, i, end;
+        Reloc *r;
+        Pciter pcsp;
 ········
         limit = up->limit;
         s = up->sym;
-        p = s->text;
 ········
-        // Small optimization: don't repeat work at top.
-        if(s->stkcheck && limit == StackLimit-callsize())
-                return 0;
+        // Don't duplicate work: only need to consider each
+        // function at top of safe zone once.
+        if(limit == StackLimit-callsize()) {
+                if(s->stkcheck)
+                        return 0;
+                s->stkcheck = 1;
+        }
 ········
         if(depth > 100) {
                 diag("nosplit stack check too deep");
                 stkbroke(up, 0);
                 return -1;
         }
-
-        if(p == nil || p->link == nil) {
+        

[iant, 2014/04/16 21:29:09]

Trailing whitespace?

[rsc, 2014/04/16 22:59:16]

Done, but everywhere in the file (sorry).

+        if(s->external || s->pcln == nil) {
                 // external function.
                 // should never be called directly.
                 // only diagnose the direct caller.
                 if(depth == 1 && s->type != SXREF)
                         diag("call to external function %s", s->name);
                 return -1;
         }
 
         if(limit < 0) {
                 stkbroke(up, limit);
                 return -1;
         }
 
         // morestack looks like it calls functions,
         // but it switches the stack pointer first.
         if(s == morestack)
                 return 0;
 
         ch.up = up;
-        prolog = (ctxt->arch->textflag(s->text) & NOSPLIT) == 0;
-        for(p = s->text; p != P; p = p->link) {
-                limit -= p->spadj;
-                if(prolog && p->spadj != 0) {
-                        // The first stack adjustment in a function with a
-                        // split-checking prologue marks the end of the
-                        // prologue.  Assuming the split check is correct,
-                        // after the adjustment there should still be at least
-                        // StackLimit bytes available below the stack pointer.
-                        // If this is not the top call in the chain, no need
-                        // to duplicate effort, so just stop.
-                        if(depth > 0)
-                                return 0;
-                        prolog = 0;
-                        limit = StackLimit;
+        prolog = !s->nosplit;

[iant, 2014/04/16 21:29:09]

Argh, negating a flag expressed as a negative.

[rsc, 2014/04/16 22:59:16]

I know, but NOSPLIT is the name of the assembler directive.

+        pciterinit(&pcsp, &s->pcln->pcsp);
+
+        // walk relocs, advancing spadj table at the same time
+        r = nil;
+        for(i = 0; i <= s->nr; i++) {
+                if(i == s->nr)
+                        end = s->size;
+                else {
+                        r = &s->r[i];
+                        if(r->type != R_CALL && r->type != R_CALLIND)
+                                continue;
+                        end = r->off;
                 }
-                if(limit < 0) {
-                        stkbroke(up, limit);
-                        return -1;
+                while(!pcsp.done && pcsp.pc < end) {
+                        if(prolog && pcsp.value > 0) {
+                                // The first stack adjustment in a function with a
+                                // split-checking prologue marks the end of the
+                                // prologue.  Assuming the split check is correct,
+                                // after the adjustment there should still be at least
+                                // StackLimit bytes available below the stack pointer.
+                                // If this is not the top call in the chain, no need
+                                // to duplicate effort, so just stop.
+                                if(depth > 0)
+                                        return 0;
+                                prolog = 0;
+                                limit = StackLimit + pcsp.value;
+                        }
+                        if(limit - pcsp.value < 0) {
+                                stkbroke(up, limit - pcsp.value);
+                                return -1;
+                        }
+                        if(end < pcsp.nextpc)

[iant, 2014/04/16 21:29:09]

Why check this both here and at the top of the loop?

[rsc, 2014/04/16 22:59:16]

It made sense at the time (yesterday morning). Note that the difference is that
we avoid the call to pciternext if we break early. It's unclear to me now. I
will write it to be clearer and come back.

[rsc, 2014/04/17 00:28:01]

This code is tricky because it needs to iterate over the relocations and the
pcsp table simultaneously. This version loops over the relocations and, inside,
over the pcsp table. My latest upload swap them, looping over the pcsp table
and, inside, the relocations. This is much clearer.

+                                break;
+                        pciternext(&pcsp);
                 }
-                if(ctxt->arch->iscall(p)) {
-                        limit -= callsize();
-                        ch.limit = limit;
-                        if(p->to.type == D_BRANCH) {
-                                // Direct call.
-                                ch.sym = p->to.sym;
-                                if(stkcheck(&ch, depth+1) < 0)
-                                        return -1;
-                        } else {
-                                // Indirect call.  Assume it is a splitting function,
-                                // so we have to make sure it can call morestack.
-                                limit -= callsize();
-                                ch.sym = nil;
-                                ch1.limit = limit;
-                                ch1.up = &ch;
-                                ch1.sym = morestack;
-                                if(stkcheck(&ch1, depth+2) < 0)
-                                        return -1;
-                                limit += callsize();
+                if(i == s->nr)
+                        break;
+
+                ch.limit = limit - pcsp.value - callsize();
+                if(r->type == R_CALL) {
+                        // Direct call.
+                        ch.sym = r->sym;
+                        if(stkcheck(&ch, depth+1) < 0)
+                                return -1;
+                        if(prolog && s->locals == 0 && strstr(r->sym->name, "morestack")) {
+                                prolog = 0;
+                                limit = StackLimit;
                         }
-                        limit += callsize();
+                } else {
+                        // Indirect call.  Assume it is a splitting function,
+                        // so we have to make sure it can call morestack.
+                        ch.sym = nil;
+                        ch1.limit = ch.limit - callsize(); // for morestack in called prologue
+                        ch1.up = &ch;
+                        ch1.sym = morestack;
+                        if(stkcheck(&ch1, depth+2) < 0)

[iant, 2014/04/16 21:29:09]

Because the code checks explicitly for morestack above, this seems to boil down
to if(ch1.limit < 0).  Is it worth doing the recursive call?

[rsc, 2014/04/16 22:59:16]

Yes, to get a nice diagnostic that tells you that a function pointer is
involved. Also, this is old Go 1.2 code - I didn't write this today - so I'm
just trying to go with what I know works (or worked).

+                                return -1;
                 }
-················
         }
+
         return 0;
 }
 
 static void
 stkbroke(Chain *ch, int limit)
 {
         diag("nosplit stack overflow");
         stkprint(ch, limit);
 }
 
 static void
 stkprint(Chain *ch, int limit)
 {
         char *name;
 
         if(ch->sym)
                 name = ch->sym->name;
         else
                 name = "function pointer";
 
         if(ch->up == nil) {
                 // top of chain.  ch->sym != nil.
-                if(ctxt->arch->textflag(ch->sym->text) & NOSPLIT)
+                if(ch->sym->nosplit)
                         print("\t%d\tassumed on entry to %s\n", ch->limit, name);
                 else
                         print("\t%d\tguaranteed after split check in %s\n", ch->limit, name);
         } else {
                 stkprint(ch->up, ch->limit + (!HasLinkRegister)*PtrSize);
                 if(!HasLinkRegister)
                         print("\t%d\ton entry to %s\n", ch->limit, name);
         }
         if(ch->limit != limit)
                 print("\t%d\tafter %s uses %d\n", limit, name, ch->limit - limit);
@@ skipping 312 matching lines @@
         vseprint(buf, buf+sizeof(buf), fmt, arg);
         va_end(arg);
         print("%s%s%s\n", tn, sep, buf);
 
         nerrors++;
         if(nerrors > 20) {
                 print("too many errors\n");
                 errorexit();
         }
 }