Miscompilation of _Unwind_RaiseException? (32-bit mode)

[Luís Marques]

Hi,

I'm trying to add exception handling support to the LLVM's RISC-V backend, to be able to properly support D's runtime and standard libraries, but I've run into a puzzling behavior of libgcc's _Unwind_RaiseException.
When a D program throws an exception, the D runtime calls _Unwind_RaiseException. Instead of calling the D exception handling personality, _Unwind_RaiseException returns a meaningless value. I'm saying meaningless because it's only supposed to return something like this:

  typedef enum
    {
      _URC_OK = 0,       /* operation completed successfully */
      _URC_FOREIGN_EXCEPTION_CAUGHT = 1,
      _URC_END_OF_STACK = 5,
      _URC_HANDLER_FOUND = 6,
      _URC_INSTALL_CONTEXT = 7,
      _URC_CONTINUE_UNWIND = 8,
      _URC_FAILURE = 9   /* unspecified failure of some kind */
    }
  _Unwind_Reason_Code;

...but instead it returns garbage (large values that change with the calling program).

I instrumented _Unwind_RaiseException with some printfs, like this:

      (...)
      code = uw_frame_state_for (&cur_context, &fs);
      printf("_Unwind_RaiseException uw_frame_state_for %d\n", code);

      if (code == _URC_END_OF_STACK)
      {
    /* Hit end of stack with no handler found.  */
        printf("_Unwind_RaiseException code == _URC_END_OF_STACK\n");
    return _URC_END_OF_STACK;
      }
      (...)

And it prints:

_Unwind_RaiseException uw_frame_state_for 5
_Unwind_RaiseException code == _URC_END_OF_STACK

...even though the observed return value is different from 5.
Looking at the disassembly of _Unwind_RaiseException we see this:

00001db0 <_Unwind_RaiseException>:
    1db0:    a7010113              addi    sp,sp,-1424
    1db4:    58912223              sw    s1,1412(sp)
    1db8:    58a12023              sw    a0,1408(sp)

(...)

    2034:    58012503              lw    a0,1408(sp)
    2038:    59010113              addi    sp,sp,1424
    203c:    00e10133              add    sp,sp,a4
    2040:    00008067              ret

So it seems that the original value of a0 is returned instead of the value returned by uw_frame_state_for?
I didn't want to assume this was a miscompilation. I checked the execution of _Unwind_RaiseException by stepping it instruction by instruction in spike. We see a0 be 5 initially, until the "lw a0,1408(sp)" overrides it with garbage. The only other theory I could come up with was that the attribute "LIBGCC2_UNWIND_ATTRIBUTE" in _Unwind_RaiseException's prototype was changing the ABI, but grepping GCC's source code didn't reveal much to support that. Also, no register had the value 5 when returning, nor did it seem like the code was storing the result in the stack (I can send spike's trace, if that's helpful).

So... is this a miscompilation of _Unwind_RaiseException?

This is from commit 994cafdbe00b25fa3442efa163503b616fcfdcac of riscv-gnu-toolchain, compiled on macOS with "CC=gcc-6 CXX=g++-6 ./build-rv32ima.sh".

[Jim Wilson]

On Thu, May 24, 2018 at 11:57 AM, Luís Marques <luism...@gmail.com> wrote:
> I'm trying to add exception handling support to the LLVM's RISC-V backend,
> to be able to properly support D's runtime and standard libraries, but I've
> run into a puzzling behavior of libgcc's _Unwind_RaiseException.

Yes, it is broken. It looks like stack unwinding has mostly been
working by accident for us. I traced the problem to the
implementation of the __builtin_eh_return function in the RISC-V
backend. The code was copied from the MIPS port. It is using the
first four arg registers for EH data, and then saving them in the
prologue and restoring them in the epilogue. The MIPS port however
has separate function return value regs from the argument registers,
so it works. In the RISC-V port, the function return value regs
overlap the argument regs, so this is clobbering the function return
value in the epilogue, which is the problem you noticed. We could
change the registers, but that might be an ABI change. I'm not sure
if anyone outside libgcc cares about the registers used here,
builtin_eh_return isn't supposed to be called by users. I also think
that saving/restoring these register is not required, because they are
call clobbered regs to begin with. That looks like a safer change,
but that will take some time to test. And I need to read through a
lot of code and make sure I correctly understand what is going on
here.

Untested patch attached that may or may not work.

Jim

[Luís Marques]

The patch seemed to solve the issue with the wrong return value. Thanks!

Now I only have to figure out why it thinks it has reached the end of the stack without having found an exception handler :(

[Jim Wilson]

On Fri, May 25, 2018 at 6:48 AM, Luís Marques <luism...@gmail.com> wrote:
> The patch seemed to solve the issue with the wrong return value. Thanks!
> Now I only have to figure out why it thinks it has reached the end of the
> stack without having found an exception handler :(

FYI There is a glibc bug not terminating the stop of stack, but if you
hit that the unwinder should be going into an infinite loop. It was
first reported as a gcc bug
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85492
This might actually be related to your bug. I had problems debugging
this at first, and might have tripped over the bug you found without
recognizing it before we decided it was a glibc bug. At least I got
it to work when I added the glibc patch, so I decided it was a glibc
bug.

Jim

[Luís Marques]

With your patch this simple program no longer works:

    try {
        throw 42;
    }
    catch(...) {
        printf("catch\n");
    }

[Jim Wilson]

On Sat, May 26, 2018 at 4:30 AM, Luís Marques <luism...@gmail.com> wrote:
> With your patch this simple program no longer works:
>
> try {
> throw 42;
> }
> catch(...) {
> printf("catch\n");
> }

Yes, I have now had a chance to run the gcc/g++ testsuites, and my
untested patch is causing testsuite failures. I will need time to
find the correct solution.

Jim

[Jim Wilson]

I added a patch upstream to fix this. The final fix ended up a lot
bigger than my first attempt to fix it. The code for
Unwind_RaiseException looks correct now, though the patch doesn't have
any effect on gcc/g++ testsuite results.
https://gcc.gnu.org/ml/gcc-patches/2018-06/msg00190.html

Jim

[Luís Marques]

Cool. I'll check it out and report my results.

[Luís Marques]

It seems to work for me (patch applied on top of the GCC 9 latest snapshot).

At least it works in the sense that I now see the _URC_END_OF_STACK return code.
Now I can start investigating why the system doesn't detect my D exception handler (the CFI directives looked correct in the assembly code).