Re: Patches to fix GCCâs C++ exception handling on NetBSD/VAX

[Jake Hamby <jehamby420 at me dot com>]

I'm very pleased to report that I was able to successfully build a NetBSD/vax system using the checked-in GCC 5.3, with the patches I've submitted, setting FIRST_PSEUDO_REGISTER to 17 and DWARF_FRAME_REGISTERS to 16. The kernel produced with GCC 5.3 crashes (on a real VS4000/90 and also SimH) in UVM, which may be a bug in the kernel that different optimization exposed, or a bug in GCC's generated code.

If you don't set DWARF_FRAME_REGISTERS to 16, then C++ exceptions break again, and GDB suddenly can't deal with the larger debug frames because of the data structure size mismatch between GCC and GDB. But with that additional define, you can raise FIRST_PSEUDO_REGISTER to include PSW (which is correct, since DWARF already uses that meaning), remove the "#ifdef notworking" around the asserts that Christos added in the NetBSD tree, and everything works as well as it did with #define FIRST_PSEUDO_REGISTER 16.

Here's the C++ test case I've been using to verify that the stack unwinding works and that different simple types can be thrown and caught. My ultimate goal is to be able to run GCC's testsuite because I'm far from certain that the OS, or even the majority of packages, really exercise all of the different paths in this very CISC architecture.

#include <string>
#include <stdio.h>

int recursive_throw(int i) {
  printf("enter recursive_throw(%d)\n", i);
  if (i > 0) {
    printf("calling recursive_throw(%d)\n", i - 1);
    recursive_throw(i - 1);
  } else {
    printf("throwing exception\n");
    throw 456;
  }
  printf("exit recursive_throw(%d)\n", i);
}

/* Test several kinds of throws. */
int throwtest(int test) {
  switch (test) {
    case 0:
    case 1:
      return test;

    case 2:
      throw 123;

    case 3:
      throw 123.45;

    case 4:
      throw 678.9f;

    case 5:
      recursive_throw(6);
      return 666;  // fail

    default:
      return 999;  // not used in test
  }
}

int main() {
  for (int i = 0; i < 6; i++) {
    try {
      int ret = throwtest(i);
      printf("throwtest(%d) returned %d\n", i, ret);
    } catch (int e) {
      printf("Caught int exception: %d\n", e);
    } catch (double d) {
      printf("Caught double exception: %f\n", d);
    } catch (float f) {
      printf("Caught float exception: %f\n", (double)f);
    }
  }
}

I'm pleased that I got it working, but the change I made to except.c to add:

RTX_FRAME_RELATED_P (insn) = 1;

below:

#ifdef EH_RETURN_HANDLER_RTX
      rtx insn = emit_move_insn (EH_RETURN_HANDLER_RTX, crtl->eh.ehr_handler);

isn't really correct, I don't think. It adds an additional .cfi directive that wasn't there before, and a GCC ./buildsh release fails building unwind-dw2.c (that's the place where the build either succeeds or fails or generates bad code) if you try to compile with assertions (and it doesn't without my change to except.c).

Unfortunately, I don't have a patch for the root cause for me having to add that line to except.c, which is that the required mov instruction to copy the __builtin_eh_return() return address into the old stack frame is being deleted for some reason, otherwise. Since I know the #ifdef EH_RETURN_HANDLER_RTX line *is* staying in the final code on other archs, I presume the problem is something VAX-related in the .md file.

If anyone knows about GCC's liveness detection, and specifically any potential problems that might cause this to be happening (removing a required "emit_move_insn (EH_RETURN_HANDLER_RTX, ...)" before a return call that was added in expand_eh_return () but then deleted if -O or higher is used), any advice would be appreciated as to where to look.

What I'm working on now is cleaning up and refactoring the .md insn definitions, but I'm not ready to share that code until it works and does something useful. I'm hoping to be able to optimize the removal of unneeded tst / cmp instructions when the condition codes have been set to something useful by a previous insn. I don't think the code in vax_notice_update_cc () is necessarily correct, and it's very difficult to understand exactly how it's working, because it's trying to determine this based entirely on looking at the RTL of the insn (set, call, zero_extract, etc), which I think may have become out of sync with the types of instructions that are actually emitted in vax.md for those operations.

So I've just finished tagging the define_insn calls in vax.md with a "cc" attribute (like the avr backend) which my (hopefully more correct and more optimized) version of vax_notice_update_cc will use to know exactly what the flag status is after the insn, for Z, N, and C. Here's my definition of "cc". I'll share the rest after I'm sure that it works.

;; Condition code settings.  On VAX, the default value is "clobber".
;; The V flag is often set to zero, or else it has a special meaning,
;; usually related to testing for a signed integer range overflow.
;; "cmp_czn", "cmp_zn", and "cmp_z" are all assumed to modify V, and
;; none is expected to modify C.  "plus" is used after an add/sub,
;; when the flags, including C, return info about the result,
;; including a carry bit to use with, e.g. "adwc".

;; The important thing to note is that the C flag, in the case of "plus",
;; doesn't reflect the results of a signed integer comparison,
;; as "cmp_czn" does.  Finally, "cmp_zn_use_c" and "cmp_z_use_cn" indicate
;; that all four flags are updated: Z and N, or Z alone, will be a comparison,
;; but C is set to 0, or some other value, instead of retaining its old value.
;; Only instructions of type "compare" set the C, Z, and N flags correctly
;; for both signed and unsigned ordered comparisons.

;; For branching, only Z is needed to test for equality, between two
;; operands or between the first operand and 0.  The N flag is necessary
;; to test for less than zero, and for FP or signed integer comparisons.
;; Both N and Z are required to branch on signed (a <= b) or (a > b).
;; For comparing unsigned integers, the C flag is used instead of N.
;; Both C and Z are required to branch on unsigned (a <= b) or (a > b).

;; The VAX instruction set is biased towards leaving C alone, relative to
;; the other flags, which tend to be modified on almost every instruction.
;; It's possible to cache the results of two signed int comparison,
;; as long as they're of the form (a < b) or (a >= b), where b may be 0,
;; in the C flag, while other instructions modify the other flags. Then,
;; a test for a branch can be saved later by referring to the previous value.
;; The cc attributes are intended so that this optimization may be performed.

(define_attr "cc" "none,cmp_czn,cmp_zn,cmp_zn_use_c,
			cmp_z,cmp_z_use_czn,plus,clobber"
  (const_string "clobber"))


I'll send another email once I have something substantial to contribute. I give my permission to NetBSD and the FSF to integrate any or all of my changes under the copyright terms of the original files. Please let me know if I have to do any additional legal stuff for code contributions. I'm doing this on my own time and not on behalf of any company or organization.

Best regards,
Jake


> On Mar 26, 2016, at 07:38, Jake Hamby <jehamby420@me.com> wrote:
> 
> Unfortunately, my previous patch that included a change to gcc/config/vax/vax.h that increased FIRST_PSEUDO_REGISTER from 16 to 17 breaks the C++ exception handling that Iâd worked so hard to get right with the rest of the patch. I believe I need to define DWARF_FRAME_REGISTERS to 16 in the same file to fix the size of the array that libgcc/unwind-dw2.c creates. The i386 backend and several others also define it their .h file for the same reason (compatibility with hardcoded frame offsets).
> 
> Hereâs the first part of the patch to vax.h that increases FIRST_PSEUDO_REGISTER and also adds a definition of DWARF_FRAME_REGISTERS as 16, with suitable comment. Iâm testing it now. I know that C++ exceptions were working before I increased FIRST_PSEUDO_REGISTER to 17.
> 
> Regards,
> Jake
> 
> Index: external/gpl3/gcc.old/dist/gcc/config/vax/vax.h
> ===================================================================
> RCS file: /cvsroot/src/external/gpl3/gcc.old/dist/gcc/config/vax/vax.h,v
> retrieving revision 1.3
> diff -u -r1.3 vax.h
> --- external/gpl3/gcc.old/dist/gcc/config/vax/vax.h	23 Sep 2015 03:39:18 -0000	1.3
> +++ external/gpl3/gcc.old/dist/gcc/config/vax/vax.h	26 Mar 2016 14:34:29 -0000
> @@ -119,13 +119,17 @@
>    The hardware registers are assigned numbers for the compiler
>    from 0 to just below FIRST_PSEUDO_REGISTER.
>    All registers that the compiler knows about must be given numbers,
> -   even those that are not normally considered general registers.  */
> -#define FIRST_PSEUDO_REGISTER 16
> +   even those that are not normally considered general registers.
> +   This includes PSW, which the VAX backend did not originally include.  */
> +#define FIRST_PSEUDO_REGISTER 17
> +
> +/* For compatibility, DWARF_FRAME_REGISTERS must still be 16.  */
> +#define DWARF_FRAME_REGISTERS 16
> 
> /* 1 for registers that have pervasive standard uses
>    and are not available for the register allocator.
> -   On the VAX, these are the AP, FP, SP and PC.  */
> -#define FIXED_REGISTERS {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1}
> +   On the VAX, these are the AP, FP, SP, PC, and PSW.  */
> +#define FIXED_REGISTERS {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1}
> 
> /* 1 for registers not available across function calls.
>    These must include the FIXED_REGISTERS and also any
>