Bad CC0 optimizer in VAX backend (was Re: Patches to fix GCC’s C++ exception handling on NetBSD/VAX)

I have some bad news and some good news. The bad news is that there has been a nasty optimizer bug lurking in the VAX backend for GCC for many years, which has to do with over-optimistic removal of necessary tst/cmp instructions under certain circumstances. This manifests at -O or higher and the symptoms are strange behavior like stack overflows because of branches going the wrong way.

The good news is that my suspicions about the CC0 handler appear to be correct, and better yet, I'm currently testing a patch that isn't too big and I believe will fix this bug. The bad behavior is in vax_notice_update_cc (), which is supposed to call CC_STATUS_INIT if the CC flags have been reset, or set cc_status.value1 and possibly cc_status.value2 with the destination of the current (set...) instruction, whose signed comparison to 0 will be stored in the N and Z flags as a side effect (most VAX instructions do this).

The first bug is that most, but not all of the define_insn patterns in vax.md actually do this. The paths that emit movz* instructions will not set N and Z, and there are some code paths that can't be trusted because they handle a 64-bit integer in two 32-bit pieces, for example, so N and Z won't reflect the desired result (comparison of operand 0 to 0).

The current version of vax_notice_update_cc has a specific check for this: it won't set the cc_status cache if GET_CODE (SET_DEST (exp)) != ZERO_EXTRACT. The problem is that this is checking the RTL expression for (zero_extract...) and not whether what was actually emitted is a movz* or not. That's why all of the define_insn()'s have to be annotated with their actual behavior vis-a-vis compare to 0, and then the change to vax.c to read the CC attribute makes it really much easier to get the correct behavior.

I need to do a lot of testing today to see if this really does help fix GCC's VAX backend, but I'm hopeful that this is at least a real bug that needs to be fixed, and that I have a fix for it that should work. The other good news is that you only need three cc enums: "none" (doesn't alter Z or N flags), "compare" (Z and N reflect comparison of operand 0 to literal 0), and "clobber" (call CC_STATUS_INIT to reset cc_status cache).

One thing to note is that the current version of vax.c only sets CC_NO_OVERFLOW on certain paths, while it should actually always set CC_NO_OVERFLOW so as to avoid emitting any unsigned branch instructions that would use an incorrect value of the C flag (which most VAX instructions do NOT set). The code that handles CC_NO_OVERFLOW is in final.c, and what it does is change signed comparisons with 0 into unsigned ones that do the same thing. (a < 0) is always false (for unsigned ints), (a >= 0) is always true, (a <= 0) turns into (a == 0), and (a > 0) turns into (a != 0). Here's the patch to vax.c. I'll send the rest after I've tested that it does what I think it should do. The diff to vax.md will be larger, but it's mostly adding the "cc" attributes and not code.


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

On 03/28/2016 04:29 PM, Jake Hamby wrote:
> I have some bad news and some good news. The bad news is that there
> has been a nasty optimizer bug lurking in the VAX backend for GCC for
> many years, which has to do with over-optimistic removal of necessary
> tst/cmp instructions under certain circumstances. This manifests at
> -O or higher and the symptoms are strange behavior like stack
> overflows because of branches going the wrong way.
So let's get that addressed.  Matt would be the best person to review 
this change as he's the Vax maintainer.  But if it's simple and 
straightforward others can handle.

It will make Matt's job easier if you can create a self-contained 
testcases which show these problems.  Ideally it'll exit (0) when the 
test passes and abort() when the test fails.  Matt (or someone else) can 
use that to help verify exactly what is happening *and* the test can be 
added to the regression testsuite.

There are hints for testcase reduction on the gcc website.

>
> The good news is that my suspicions about the CC0 handler appear to
> be correct, and better yet, I'm currently testing a patch that isn't
> too big and I believe will fix this bug. The bad behavior is in
> vax_notice_update_cc (), which is supposed to call CC_STATUS_INIT if
> the CC flags have been reset, or set cc_status.value1 and possibly
> cc_status.value2 with the destination of the current (set...)
> instruction, whose signed comparison to 0 will be stored in the N and
> Z flags as a side effect (most VAX instructions do this).
Right.  Pretty standard stuff for a cc0 target.


>
> The first bug is that most, but not all of the define_insn patterns
> in vax.md actually do this. The paths that emit movz* instructions
> will not set N and Z, and there are some code paths that can't be
> trusted because they handle a 64-bit integer in two 32-bit pieces,
> for example, so N and Z won't reflect the desired result (comparison
> of operand 0 to 0).
Understood.  So it sounds like there's two bugs, which implies we'd 
really like two independent tests.

One is the movz instructions don't set the cc0 codes, but 
notice_update_cc isn't aware of that or doesn't handle it properly.

Second is the handling of notice_update_cc for double-word instructions. 
  Many ports use CC_STATUS_INIT for those as tracking the status bits 
can be painful.

>
> The current version of vax_notice_update_cc has a specific check for
> this: it won't set the cc_status cache if GET_CODE (SET_DEST (exp))
> != ZERO_EXTRACT. The problem is that this is checking the RTL
> expression for (zero_extract...) and not whether what was actually
> emitted is a movz* or not. That's why all of the define_insn()'s have
> to be annotated with their actual behavior vis-a-vis compare to 0,
> and then the change to vax.c to read the CC attribute makes it really
> much easier to get the correct behavior.
Right.  That's similar to how other cc0 ports work -- they have 
attributes which clearly indicate the cc0 status for each alternative. 
Then you just need to add the attribute to each insn and check it in 
notice_update_cc.  See the v850 port as a fairly simple example of how 
this can be handled.


>
> I need to do a lot of testing today to see if this really does help
> fix GCC's VAX backend, but I'm hopeful that this is at least a real
> bug that needs to be fixed, and that I have a fix for it that should
> work. The other good news is that you only need three cc enums:
> "none" (doesn't alter Z or N flags), "compare" (Z and N reflect
> comparison of operand 0 to literal 0), and "clobber" (call
> CC_STATUS_INIT to reset cc_status cache).
It's not uncommon to need an attribute that says the cc0 bits aren't 
modified, but operand0 is modified.  In fact, that may be what you want 
for the movz instructions on the vax from what I've read above.

As far as the patch itself, I'd drop all the grubbing around in RTL, 
except for the case where the cc0 bits are set from a comparison. 
Essentially you let the insn's attributes entirely describe the cc0 
status.   Again, see v850.c::notice_update_cc and the attributes in v850.md.

Jeff