Fix unsafe `PUSHF_CLI`/`POPF` macros for gcc

[jwt27]

I just noticed these macros, I'm surprised this hasn't caused any trouble yet. Or maybe it has, it just would be very hard to debug. At least in gcc, modifying the stack pointer in an asm block is very risky. I don't know how well the other compilers cope with this.

A safer solution is to keep the stack pointer in the same place, and store the flags someplace where the compiler can see them, in a variable.

[gvanem]

I'm surprised this hasn't caused any trouble yet.

Why? The PUSHF_CLI() (and the POPF()) modifies SP. Function arguments in gcc (or most others) uses the BP register. So where is the problem?

[jwt27]

Function arguments in gcc (or most others) uses the BP register.

Not if you compile with -fomit-frame-pointer (enabled by -O1 and higher).

[gvanem]

Not if you compile with -fomit-frame-pointer

Oh, I see. Any way in gcc to avoid such naked functions? I do not like storing the FLAGS in a local eflags. What if there are 2 PUSHF_CLI() in a function?

[jwt27]

There isn't really an alternative. If you have "esp" in the clobber list (which this code should have done from the beginning), you'll see:

<source>: In function 'void f()':
<source>:4:5: warning: listing the stack pointer register 'esp' in a clobber list is deprecated [-Wdeprecated]
    4 |     asm ("" ::: "esp");
      |     ^~~
<source>:4:5: note: the value of the stack pointer after an 'asm' statement must be the same as it was before the statement

You could add a fake dependency on the frame pointer, then the compiler will save it and use it for addressing. But it's still a horrible hack:

asm volatile ("pushf; cli" :: "r" (__builtin_frame_pointer (0)));

There is also no hard guarantee that both macros will be invoked with the stack pointer at exactly the same position.

[jwt27]

If you need multiple pushf-cli-popf sequences in one function, you could put them in separate scope blocks:

void f ()
{
  {
    PUSHF_CLI ();
    /* ... */
    POPF ();
  }
  {
    PUSHF_CLI ();
    /* ... */
    POPF ();
  }
}

[gvanem]

These macro had been there since 1997 (I think). I saw no problems then when running djgpp on MSDOS or Win-XP. Then the others Watcom 16/32-bit etc. would have the same "bug".

But lets merge this anyway.

[jwt27]

Well as I said, I'm surprised it hasn't caused problems yet. Maybe older gccs also had more predictable code generation. I don't know much about Watcom, maybe it can handle this (don't see anything about it in the docs).

Right now these macros are only used in clockbits() (timer.c). It happens to work because the compiler has little reason to use the stack there. I think -O0 -fomit-frame-pointer would already break it though.

But if you used these anywhere else (eg. re-enable the RMCB code (undef USE_FAST_PKT)), I'm sure you'd notice some problems :)

Even for something as simple as:

    PUSHF_CLI ();
    do_stuff (123);
    POPF ();

gcc will happily compile it to:

    pushf
    cli
    push 123
    call do_stuff
    popf
    add esp, 4