Stuck in do{} while(false) loop

Question

First off, this is related to an assignment.

It's an OS course, and we're supposed to use fibers so that our system is responsive while doing long computations. For this, we have been provided with functions to save and restore stacks etc. The idea is that instead of a for loop running 100 times, we run it once, save the stack and go back, do some other stuff, and then restore the function's stack.

The trouble is that the provided macro to save and restore stacks is getting stuck in an infinite loop. The surprising part is that it is written as a do{}while(false) loop, so that shouldn't even happen.

What might cause it to happen? Here's the macro:

#define stack_saverestore(from_stack,to_stack) do {                  \
 asm volatile(                                                       \
   "  pushl %%eax      \n\t"                                         \
   "  pushl %%ecx      \n\t"                                         \
   "  pushl %%ebp      \n\t"                                         \
   "  pushl $1f        \n\t"                                         \
   "                   \n\t"                                         \
   "  movl  %%esp,(%0) \n\t"                                         \
   "  movl  (%1),%%esp \n\t"                                         \
   "                   \n\t"                                         \
   "  ret              \n\t"                                         \
   "1:                 \n\t"                                         \
   "  popl %%ebp       \n\t"                                         \
   "  popl %%ecx       \n\t"                                         \
   "  popl %%eax       \n\t"                                         \
  :                                                                  \
  :"a" (&from_stack), "c"  (&to_stack)                               \
  :_ALL_REGISTERS, "memory"                                          \
 );                                                                  \
} while(false)

I'm pretty sure the problem isn't in the macro itself, since I'm using it in another place, and not having any trouble. I'm having trouble debugging it, since the macro is mostly assembly code.

Edit : the logic behind stack_saverestore,

//
// Switch stacks.
//
// Algo: 
//   1. Save _c's context to stack, 
//   2. push ip of _c's restore handler
//   3. switch stacks
//   4. execute ip of _n's restore handler to restore _n's context from stack.
//
//
// stack layout: 
//  teip[-1:-32]: continuation to restore, 
//  Stack layout expected by teip:
//     ebp[ -33: -64], 
//     ebx[ -65: -96], 
//     eax[ -97:-128], 
//     Stack layout expected by eip+4:
//        Preserved.

Usage details : The macro is being used to implement fibers for a very rudimentary shell. The thing I'm doing is unimportant, but basically I'm adding the divisors of a large number. I know this is not the optimal way, but that is not the issue here.

void fiberFactor(addr_t* pmain_stack, addr_t* pf_stack,
        shellstate_t& shellstate) {

    addr_t & main_stack = main_stack; 
    addr_t & f_stack = f_stack;
    bool& done = shellstate.fiber_done;

    int n = shellstate.factorArg;
    int i = 1;
    int sum = 0;


    for (i = 1; i < n; i++) {
        for (int j = 0; j < 1000; j++) {
            if (n % i == 0) {
                sum = sum + i;
            }
            i++;
        }
        i--;
        shellstate.fiber_done = false;
        hoh_debug("about to switch stacks, i "<<i<<sum);
        stack_saverestore(f_stack, main_stack); //Never returns from here.
    }
//The hope is that with each iteration of the outer for loop, 
//we do some computation, and then yield execution. 
//Eventually, the computation is finished, and we set the flags here,   
//and switch out for the last time.
    for (;;) {
        shellstate.fiber_done = true;
        shellstate.fiber_do = false;
        shellstate.factorVal = sum;
        stack_saverestore(f_stack, main_stack);
    }

}


//This function is called by the shell as part of the main loop. 
//If we have to do something, as indicated by the booleans, do it.
void shell_step_fiber(shellstate_t& shellstate, addr_t& main_stack,
        addr_t& f_stack, addr_t f_array, uint32_t f_arraysize) {

    if (shellstate.resetFiber) {
        shellstate.resetFiber = false;
        stack_init3(f_stack, f_array, f_arraysize, &fiberFactor, &main_stack, &f_stack, &shellstate); //Reset the stacks.
    }

    if (shellstate.fiber_do){
        stack_saverestore(main_stack, f_stack); //Switch to fiberFactor. This works without a hitch.
    }
}

The trouble is in the fiberFactor function, where I'm calling stack_saverestore inside the for loop.

Answer 1

Turns out the error was not in the loop, but in my code - in particular, this line in fiberFactor:

addr_t & main_stack = main_stack; 

This makes no sense, as the parameter passed to the function is pmain_stack. Changing to

addr_t & main_stack = *pmain_stack; 

and same for f_stack, solved the issue.

Credits to @DavidWolfherd for spotting it. Though why this caused not an error but an infinite loop in the code for stack_saverestore, I have no clue.

Answer 2

A couple of things are jumping out at me.

First off, your stacks need space to store their stuff. If I am reading things correctly, your stacks are essentially addresses, likely 4 bytes apart--and you are trying to store 16 bytes on the stack. The net result is that when you write to one stack, you corrupt the other.

To correct this, I would recommend creating yourself a stack structure that explicitly has space for everything you want to save/restore from it.

typedef struct {
    uint32_t  eip;
    uint32_t  ebp;
    uint32_t  ecx;
    uint32_t  eax;
} my_stacktype_t; 

The items are in reverse order from what you push as the stack on the x86 grows down.

The next thing that jumps out at me is that you are only saving a subset of the necessary registers. Perhaps the body of your loop is only using those regs, but if it changes, you will need to store more/different registers. I recommend saving/restoring all the general purpose registers: eax, ebx, ecx, edx, esi, edi, ebp, esp and eip (hopefully I have not forgotten one--I am going by memory here).

I'd have to think about your use case scenario to be absolutely certain, but storing the stacks on the stack is at minimum a code smell. In my experience, stacks are typically stored as global variables or dynamically allocated from the heap.

Hope this helps.