lcc: argument references in variadic functions

[kervinck]

Working on printf and implementing stdarg, when I noticed this:

#include <stdio.h>

void f(int a, int b, ...)
{
  putchar(a);
  putchar(b);
  putchar('\n');
}

int main(void)
{
  f('1', '2', '3', '4', '5');
  return 0;
}

This prints '45' instead of '12'.

The argument order on the stack is backwards in memory, because the data stack grows downwards and the first argument gets pushed first. Arguments are referenced relative to the stack pointer (by ldloc/stloc) and not the frame pointer (as there is no such a thing?).

From src/gt1.md:

static void function(Symbol f, Symbol caller[], Symbol callee[], int n) {
        // Frame:
        //
        // +-------------------------------+
        // | Incoming arguments            |
        // +-------------------------------+ <- original sp
        // | Saved registers               | <- saversize bytes
        // +-------------------------------+
        // | Locals                        | <- framesize bytes
        // +-------------------------------+ <- sp
        //
        // - Args end at sp + framesize + saversize
        // - Locals end at sp

It's not immediately clear to me how to fix this. We wouldn't want to introduce a frame pointer just for this. Reversing the order of arguments on the stack comes to mind. This requires a bit of surgery, and I believe C allows the implementation to perform backwards evaluation of arguments. I'm happy to take a shot at it. Any thoughts if I might overlook something, @pgavlin?

[kervinck]

This seems to work:

--- a/Utils/lcc/src/gt1.md
+++ b/Utils/lcc/src/gt1.md
@@ -843,8 +843,8 @@ static void inst_blkcopy(Node p) {

 static int localoffset(Node addr) {
        if (generic(addr->op) == ADDRF) {
-               // Args end at sp + 2 + framesize + saversize + argsize.
-               return 2 + argdepth + framesize + saversize + argsize - addr->syms[0]->x.offset;
+               // Args start at sp + 2 + framesize + saversize
+               return 2 + argdepth + framesize + saversize + addr->syms[0]->x.offset;
        }

        // Locals end at sp + 2 + framesize.
@@ -1198,11 +1198,11 @@ static void function(Symbol f, Symbol caller[], Symbol callee[], int n) {
        for (int i = 0; callee[i]; i++) {
                Symbol p = callee[i], q = caller[i];
                assert(q);
-               offset += roundup(q->type->size, 2);
                debug(fprint(stderr, "parameter %s of size %d @ offset %d\n", p->name, p->type->size, offset));
                p->x.offset = q->x.offset = offset;
                p->x.name = q->x.name = stringf("%d", p->x.offset);
                p->sclass = q->sclass = AUTO;
+               offset += roundup(q->type->size, 2);
        }
        argsize = offset;
        offset = maxoffset = 0;
@@ -1335,7 +1335,7 @@ Interface gt1IR = {
        0,        /* mulops_calls */
        0,        /* wants_callb */
        0,        /* wants_argb */
-       1,        /* left_to_right */
+       0,        /* left_to_right */
        0,        /* wants_dag */
        1,        /* unsigned_char */
        NULL /*address*/, // 2019-04-30 (marcelk) TODO: Enable when asm.py can evaluate symbol offsets

[kervinck]

There may be a secondary issue: the data stack doesn't seem to be fully unwound upon return from a variadic function. Only the formal arguments are popped, by the callee. The responsibility should in this case probably lie with the caller instead.

[kervinck]

Concept that seems to work for the secondary issue.

--- a/Utils/lcc/src/gt1.md
+++ b/Utils/lcc/src/gt1.md
@@ -1097,12 +1097,42 @@ static void inst_bcom(Node p) {
        }
 }

+static int fargsize(Type ty) { // size of all formal arguments
+        assert(isfunc(ty) && hasproto(ty));
+        int size = isstruct(ty->type) ? 2 : 0; // one hidden argument for struct return
+        for (int i = 0; ty->u.f.proto[i]; i++) {
+                if (ty->u.f.proto[i] == voidtype) {
+                        break; // variadic function
+                }
+                size += 2;
+        }
+        return size;
+}
+
 static void inst_call(Node p) {
        if (getregnum(p->kids[0]) == 0) {
                print("asm.call('vAC')\n");
        } else {
                print("asm.call('r%d')\n", getregnum(p->kids[0]));
        }
+
+       int vargsize = 0;
+       Type fntype = p->kids[0]->syms[0]->type;
+       if (hasproto(fntype)) {
+                vargsize = argdepth - fargsize(fntype);
+        }
+        assert(vargsize >= 0);
+
+       if (vargsize > 0) {
+                assert(variadic(fntype));
+               if (vargsize < 256) {
+                       print("asm.ldi(%d)#vargsize\n", vargsize);
+               } else {
+                       print("asm.ldwi(%d)#vargsize\n", vargsize);
+                }
+               print("asm.addw('sp')\n");
+               print("asm.stw('sp')\n");
+       }
        argdepth = 0;
 }

[kervinck]

Some thoughts before I forget:

1. We could also choose to let the caller of a variadic function pop all arguments from the data stack after returning from the call (that is: not just the excess ones, if any). There are advantages and disadvantages to either way. For now, I think I keep it a dual responsibility.

2. My va_arg(ap, type) macro doesn't handle struct arguments yet. It must determine if type is put on the stack by value or by reference. I think it only has the sizeof to go by (I don't know if the macro can test for structness?). This means that we should still put structs of 1 or 2 bytes by value on the stack. Is this indeed what is happening already?

3. The condition vargsize >= 256 is not too common :-). But it doesn't hurt to have it there. And if we ever decide to put struct arguments by value on the stack, it might be triggered. (I see no immediate reason what we couldn't do that? At some point we need to support 4-byte longs and 8-byte long longs. Perhaps floats, doubles and long doubles as well?

[kervinck]

4. The dag apparently can lose the function type information for CALL nodes. This means that the edge case of "calling a variadic function with extra arguments through a function pointer" still fails. inst_call() doesn't see any function prototype, and therefore it can't determine how many additional arguments it must pop after the CALL instruction.

See the comments in Utils/lcc/src/gt1.md

        // XXX This doesn't yet work for the case where the variadic
        //     function is called through a function pointer, because
        //     then the function type seems to be lost in the dag?!?!

[kervinck]

Remaining issue has low priority

[lb3361]

Suggesting to close this issue since glcc does this correctly already.