#1328 __try1 (excpt.h) missing memory clobber

[thomas]

Problem: __try1 will fail "mysteriously" (consistently, reproducably, but not according to any immediately obvious pattern) if optimisations are turned on.
Looking at the disassembly shows that in these cases of failure, instructions were reordered, thus if an exception occurred too soon after __try1, it would sometimes only install the SEH handler after the application had already crashed.

Fix: add a "memory" clobber as compiler barrier to __try1 (and probably it's a good idea to add one to __except1 too).

#define __try1(pHandler) \ __asm__ ("pushl %0;pushl %%fs:0;movl %%esp,%%fs:0;" : : "g" (pHandler) : "memory");

[Earnie Boyd]

Assigning to Chris for follow up.

[Earnie Boyd]

I need a sample test program attached to this issue. Thomas can you supply it?

[Earnie Boyd]

No test program to display the issue.

[Keith Marshall]

I'm reopening this. I don't understand why it should have been closed as "invalid", because the OP has a very valid point; the inline assembly modifies memory behind the compiler's back, without offering even the slightest hint that it intends to do so. In fact, I would go further, and add not only the memory clobber constraint, but also explicitly add the "volatile" attribute; (it should be implied anyway, because there are no output parameters specified, but I prefer to make it explicitly obvious).

And yes, __except1 also writes to memory behind the compiler's back, so it wouldn't just be a good idea to add a memory clobber constraint to it too -- it really should be considered as a mandatory requirement.

[Keith Marshall]

It's actually much, much worse than just a missing memory clobber attribute; so bad, in fact, that I wonder did it ever work? It's certainly been FUBAR since GCC-3.4.5, if not before. The problem is not so much that it modifies memory behind the compiler's back, but that it modifies the stack pointer, in a manner which is entirely beyond GCC's comprehension. Here's an illustration, (compiled with GCC-3.4.5, after adding the memory clobber attribute -- adding __volatile__ makes no difference -- and replacing embedded semicolons by "\n\t", within the inline assembly code, to yield more legible intermediate assembly output):

$ cat foo.c
#include <stdio.h>
#include <excpt.h>

extern int add2( int *, int * );

extern EXCEPTION_DISPOSITION bar
(struct _EXCEPTION_RECORD *a, void *b, struct _CONTEXT *c, void *d );

void foo( const char *msg )
{
  int a = 2, b = 5;

  puts( msg );
  __try1(bar)
  {
    add2( &a, &b );
    puts( msg );
  }
  __except1;
}

$ use mingw32-gcc-3.4.5

$ mingw32-gcc --version
mingw32-gcc (GCC) 3.4.5
Copyright (C) 2004 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ mingw32-gcc -S -O2 foo.c -o /dev/stdout
    .file   "foo.c"
    .text
    .p2align 2,,3
.globl _foo
    .def    _foo;   .scl    2;  .type   32; .endef
_foo:
    pushl   %ebp
    movl    %esp, %ebp
    pushl   %ebx
    subl    $32, %esp
    movl    8(%ebp), %ebx
    pushl   %ebx
    movl    $2, -12(%ebp)
    movl    $5, -8(%ebp)
    call    _puts
/APP
    pushl $_bar
    pushl %fs:0
    movl %esp, %fs:0
/NO_APP
    popl    %eax
    popl    %edx
    leal    -8(%ebp), %eax
    pushl   %eax
    leal    -12(%ebp), %eax
    pushl   %eax
    call    _add2
    movl    %ebx, (%esp)
    call    _puts
/APP
    movl (%esp), %eax
    movl %eax, %fs:0
    addl $8, %esp
/NO_APP
    movl    -4(%ebp), %ebx
    leave
    ret
    .def    _add2;  .scl    3;  .type   32; .endef
    .def    _bar;   .scl    3;  .type   32; .endef
    .def    _puts;  .scl    3;  .type   32; .endef

Notice the two popl instructions, immediately following the first /APP.../NOAPP block, (which corresponds to the __try1(bar) statement). These are emitted by GCC, presumably with the (too late) intention to clear the argument pushed for the preceding call _puts; what they actually do is delete the exception handler reference frame we thought we'd just installed. This happens because __try1(bar) changed the stack pointer when GCC wasn't looking, so it doesn't know it's undoing the effect of the inline assembly code; unfortunately, adding a stack pointer clobber doesn't mitigate this.

So, it's broken for GCC-3.4.5 already; it gets (possibly) even worse for later versions of GCC. Here's what the same code looks like, with GCC-5.3.0:

$ use mingw32-gcc-5.3.0

$ mingw32-gcc --version
mingw32-gcc (GCC) 5.3.0
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ mingw32-gcc -S -O2 foo.c -o /dev/stdout
    .file   "foo.c"
    .section    .text.unlikely,"x"
LCOLDB0:
    .text
LHOTB0:
    .p2align 4,,15
    .globl  _foo
    .def    _foo;   .scl    2;  .type   32; .endef
_foo:
LFB16:
    .cfi_startproc
    pushl   %ebx
    .cfi_def_cfa_offset 8
    .cfi_offset 3, -8
    subl    $40, %esp
    .cfi_def_cfa_offset 48
    movl    48(%esp), %ebx
    movl    $2, 24(%esp)
    movl    $5, 28(%esp)
    movl    %ebx, (%esp)
    call    _puts
/APP
 # 14 "foo.c" 1
    pushl $_bar
    pushl %fs:0
    movl %esp, %fs:0
 # 0 "" 2
/NO_APP
    leal    28(%esp), %eax
    movl    %eax, 4(%esp)
    leal    24(%esp), %eax
    movl    %eax, (%esp)
    call    _add2
    movl    %ebx, (%esp)
    call    _puts
/APP
 # 19 "foo.c" 1
    movl (%esp), %eax
    movl %eax, %fs:0
    addl $8, %esp
 # 0 "" 2
/NO_APP
    addl    $40, %esp
    .cfi_def_cfa_offset 8
    popl    %ebx
    .cfi_restore 3
    .cfi_def_cfa_offset 4
    ret
    .cfi_endproc
LFE16:
    .section    .text.unlikely,"x"
LCOLDE0:
    .text
LHOTE0:
    .ident  "GCC: (GNU) 5.3.0"
    .def    _puts;  .scl    2;  .type   32; .endef
    .def    _bar;   .scl    2;  .type   32; .endef
    .def    _add2;  .scl    2;  .type   32; .endef

Now, the destructive stack pointer adjustment after the __try1(bar) code has disappeared, but the compiler is still overwriting the exception handler frame, (by the movl %eax, 4(%esp) and movl %eax, (%esp) instructions). Furthermore, the references the the auto variables, a and b, are now computed relative to %esp,
(the leal 24(%esp), %eax and leal 28(%esp), %eax instructions respectively), where GCC-3.4.5 had used references based on %ebp; these %esp references are off by eight bytes, because the compiler doesn't comprehend the change in the stack pointer, within the __try1(bar) code, and once again, specifying a stack pointer clobber has no mitigating effect.

So, the current implementation is utterly broken, in a manner which will vector exceptions to trashed code addresses, so anything (possibly very bad) could happen, when an exception is thrown. I'm wondering if it's worth trying to fix this, or should <excpt.h> just disappear without trace?

[Keith Marshall]

While simply pushing an exception handler frame on to the stack is doomed to failure, having GCC manage the reservation via alloca(), (actually __builtin_alloca()) should be safe, and appears to work. Hence, I committed [fc36f6].