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

[Jake Hamby <jehamby420 at me dot com>]

The results you want to see for the test program are the following:

throwtest(0) returned 0
throwtest(1) returned 1
Caught int exception: 123
Caught double exception: 123.450000
Caught float exception: 678.900024
enter recursive_throw(6)
calling recursive_throw(5)
enter recursive_throw(5)
calling recursive_throw(4)
enter recursive_throw(4)
calling recursive_throw(3)
enter recursive_throw(3)
calling recursive_throw(2)
enter recursive_throw(2)
calling recursive_throw(1)
enter recursive_throw(1)
calling recursive_throw(0)
enter recursive_throw(0)
throwing exception
Caught int exception: 456

Before I made the changes I've submitted, this worked on m68k and presumably everywhere else but VAX. On VAX, it crashed due to the pointer size discrepancies that I already talked about. I believe that it should be possible to improve GCC's backend by allowing %ap to be used as an additional general register (removing it from FIXED_REGISTERS, but leaving it in CALL_USED_REGISTERS, since it's modified on CALLS), without breaking the DWARF stack unwinding stuff, since the .cfi information it's emitting notes the correct %fp offset to find the frame, which it actually uses instead of the %ap in stack unwinding.

Gaining an additional general register to use within a function would be a nice win if it worked. But there are at two problems that must be solved before doing this (that I know of). The first is that certain combinations of VAX instructions and addressing modes seem to have problems when %ap, %fp, and/or %sp are used. I discovered this in the OpenVMS VAX Macro reference (which is essentially an updated version of the 1977 VAX architecture handbook), in Table 8-5, General Register Addressing.

An additional source of info on which modes fail with address faults when AP or above is used, SimH's vax_cpu.c correctly handles this, and you can trace these macros to find the conditions:

#define CHECK_FOR_PC    if (rn == nPC) \
                            RSVD_ADDR_FAULT
#define CHECK_FOR_SP    if (rn >= nSP) \
                            RSVD_ADDR_FAULT
#define CHECK_FOR_AP    if (rn >= nAP) \
                            RSVD_ADDR_FAULT

So as long as the correct code is added to vax.md and vax.c to never emit move instructions under the wrong circumstances when %ap is involved, it could be used as a general register. I wonder if the use of %ap to find address arguments is a special case that happens to never emit anything that would fail (with a SIGILL, I believe).

But the other problem with making %ap available as a general (with a few special cases) register is that it would break one part of the patch I submitted at the beginning of the thread to fix C++ exceptions. One necessary part of that fix was to change "#define FRAME_POINTER_CFA_OFFSET(FNDECL) 0" to "#define ARG_POINTER_CFA_OFFSET(FNDECL) 0", which correctly generates the code to return the value for __builtin_dwarf_cfa () (as an offset of 0 from %ap).

When I was working on that fix, it seemed like it should be possible, since the DWARF / CFA code that's in there now is using an offset from %fp that it knows, that an improved fix would define FRAME_POINTER_CFA_OFFSET(FNDECL) as something that knows how to return the current CFA (canonical frame address) as an offset from %fp, since that's what it's using for all the .cfi_* directives. But I don't know what a correct definition of FRAME_POINTER_CFA_OFFSET should be in order for it to return that value, instead of 0, because I know that a 0 offset from %fp is definitely wrong, and it's not a fixed offset either (it depends on the number of registers saved in the procedure entry mask). Fortunately, %ap points directly to CFA, so that always works.

Just some thoughts on future areas for improval... I'm very happy to be able to run the NetBSD testsuite on VAX now. It gives me a lot of confidence as to what works and what doesn't. Most of the stuff I expected to fail (like libm tests, since it's not IEEE FP) failed, and most of the rest succeeded.

-Jake


> On Mar 27, 2016, at 15:34, Jake Hamby <jehamby420@me.com> wrote:
> 
> 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
>> 
>