Windows x64 SEH __except inside __finally the finally runs twice

[Quuxplusone]

Bugzilla Link	PR30761
Status	NEW
Importance	P normal
Reported by	nb@ravenbrook.com
Reported on	2016-10-21 08:36:11 -0700
Last modified on	2016-10-28 02:56:55 -0700
Version	3.9
Hardware	PC All
CC	llvm-bugs@lists.llvm.org, rnk@google.com
Fixed by commit(s)
Attachments	file_30761.txt (6758 bytes, text/plain) t.s (5662 bytes, text/plain)
Blocks
Blocked by
See also

Created attachment 17474
C source, with disassembly and comparison to code generated by Visual Studio

Using Clang 3.9.0 for x64 on Windows:

int f(int x)
{
    __try {
        __try {
            x = g(x);
        } __except((x == 42) ? EXCEPTION_EXECUTE_HANDLER : EXCEPTION_CONTINUE_SEARCH) {
            ++x;
        }
    } __finally {
        x *= 2;
    }
    return x;
}

if called with x == 42, and g raises an exception, then f returns 170 and not
86 as it should.  The finally clause is run twice, once as a "cleanup" function
called from __C_specific_handler, after the filter fires but before the
handler, and once in the continuation from the try clause.  It is incorrect for
the cleanup function to be called in this case.

Looking through the generated code, and comparing with that produced by Visual
Studio, the problem seems to be that the data produced by Clang doesn't trigger
the EXCEPTION_TARGET_UNWIND case inside __C_specific_handler.  That case
specifically prevents cleanup functions from being called if the TargetIP is in
the ScopeRecord's range.  However, the TargetIP in this case is the address of
the handler continuation, and the handler continuation has not been placed in
the range of the scope record.

The attachment here has a detailed disassembly and commentary on the unwind
data etc, and a comparison with code generated by Visual Studio.

Clang generates a good filter function and continuation code for the __except
clause, and also generates a cleanup function for the __finally clause.

[Quuxplusone]

Attached file_30761.txt (6758 bytes, text/plain): C source, with disassembly and comparison to code generated by Visual Studio

[Quuxplusone]

I'm unable to reproduce this locally with clang 3.9 or trunk. My steps:

$ cat t.cpp
int g(int x) {
  if (x == 42)
    throw 1;
  return x;
}
int f(int x) {
  __try {
    __try {
      x = g(x);
    } __except ((x == 42) ? 1 : 0) {
      ++x;
    }
  } __finally {
    x *= 2;
  }
  return x;
}
int main() { return f(42); }

$ clang-cl -EHsc t.cpp && ./t.exe ; echo $?
86

$ clang-cl -O1 -EHsc t.cpp && ./t.exe ; echo $?
86

$ clang-cl -O2 -EHsc t.cpp && ./t.exe ; echo $?
86

This is using the MSVC 2015 x64 CRT. Can you elaborate on how your steps are
different? Thanks for the report.

[Quuxplusone]

I would guess that your case has a different result because it's C++, so will
be using the CxxFrameHandler3 personality, whereas mine is C so uses
__C_specific_handler.  That changes everything to do with the LSDA (language-
specific data area) which is where my previous attachment indicated the problem
probably lies.  Here's a smaller single-file example:

$ cat filters-small.c
#include <stdio.h>
#include <Windows.h>

extern void g(int x)
{
    RaiseException(0xE0123456, 0, 0, NULL);
}

int f(int x)
{
    __try {
        __try {
            g(x);
        } __except((x == 42) ? EXCEPTION_EXECUTE_HANDLER : EXCEPTION_CONTINUE_SEARCH) {
            ++x;
        }
    } __finally {
        x *= 2;
    }
    return x;
}

int main(void)
{
    printf("Raising     42 ->  86: %d\n", f(42));
    return 0;
}
$ clang -v
clang version 3.9.0 (branches/release_39)
Target: x86_64-pc-windows-msvc
Thread model: posix
InstalledDir: C:\Program Files\LLVM-3.9.0\bin
$ clang filters-small.c
$ ./a.exe
Raising     42 ->  86: 170
$

[Quuxplusone]

I'm also building Clang from trunk sources to see if this is fixed there.

[Quuxplusone]

(In reply to comment #2)
> I would guess that your case has a different result because it's C++, so
> will be using the CxxFrameHandler3 personality, whereas mine is C so uses
> __C_specific_handler.

Using __try and __except always causes the affected function to use
__C_specific_handler, even in C++. I just didn't want to include windows.h so I
used C++ throw. :)

I'll try your example, thanks.

[Quuxplusone]

FWIW I get 86 at all optimizations levels, so I think this has been fixed.

[Quuxplusone]

Is that with 3.9 or trunk? I've been having problems building from trunk (possibly something to do with my Visual Studio installation), so I haven't been able to test against it.

[Quuxplusone]

Trunk, but 3.9 is very recent. Maybe this is a CRT difference?

BTW, trunk requires VS 2015 these days. That might be your issue.

[Quuxplusone]

Yes, I'm building with VS 2015. I think my build problems are all to do with the brain-damaged VM I'm running it on. We'll find out tomorrow.

[Quuxplusone]

Can you run with -O -S and post the results? I'm particularly interested in the LSDA, so I'd also like to see dumpbin /disasm and dumpbin /all of the object file (my interest in this stuff is driven by a need to generate LLIR for a compiler for a client's own programming language; the exception-handling is the hardest part).

[Quuxplusone]

Attached t.s (5662 bytes, text/plain): O1 assembly

[Quuxplusone]

I still get 170 with a 4.0.0 trunk build (checked out yesterday).  But further
experimentation suggests that it is something to do with the CRT, as if I run
in a VS2015 CMD after a vcvars64.bat then I get the right result.  Linker,
perhaps?
I'll poke at this some more in the morning, to see if I can narrow it down.
What I'm interested in is ensuring I get the right object code (including LSDA)
from LLVM libraries linked into a client's compiler; if I have to tweak the
environment to do that then I need to know that.  In the short term we're going
to be faking __C_specific_handler (by post-processing the UNWIND_INFO) and
rolling our own, so I need to dig through the entrails of this a bit more.

[Quuxplusone]

I have a long document reverse engineering C_specific_handler, which explains why the finally funclet is called. Possibly the C_specific_handler is different in VS2015's runtime? I can clean that document up and post it tomorrow.

[Quuxplusone]

The range check of the except label that you describe is hilariously naive. It assumes that the generated code remains in source order. LLVM's machine block placement puts the except code after the normal return, so it's definitely not in the try range of the try/finally. Given that exceptions are very unlikely, LLVM's code layout is probably better for performance.

I'm definitely curious to find out why this works with 2015, though. :)

[Quuxplusone]

Performance has to come second to semantic correctness, though. You can get the right semantics with the VS2010 runtime by adding additional ScopeRecords covering the handler code, which is what the VS2010 compiler will do if necessary. The interesting question is the semantics of the __C_specific_handler in VS2015. I'll look into that tomorrow, if I have time.

[Quuxplusone]

(In reply to comment #15)
> Performance has to come second to semantic correctness, though.  You can get
> the right semantics with the VS2010 runtime by adding additional
> ScopeRecords covering the handler code, which is what the VS2010 compiler
> will do if necessary.

Sure, but the system is just doing what it was designed to do. We don't express
__C_specific_handler's invariant to block placement, so it lays code out this
way. Actually expressing that invariant would be hard, and I'm not sure it's
worth doing if what we already have works with more recent CRTs.

[Quuxplusone]

Maybe the clang executable could detect that it's going to link against an outdated version of __C_specific_handler? Basically if a developer has multiple versions of VS installed (not uncommon) and has run the wrong vcvars script most recently, then clang will silently produce executables which have this quite subtle wrong behaviour and are very difficult to diagnose.

Our project won't be affected: now that we understand the problem we can put checks in our build system.