Bug 56875 - vax target miscompiles short negative constants for 64bit values
Summary: vax target miscompiles short negative constants for 64bit values
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: unknown
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-08 12:04 UTC by Martin Husemann
Modified: 2013-09-21 08:18 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments
Use the D format specifie for ashqs second arg (505 bytes, patch)
2013-04-08 12:04 UTC, Martin Husemann
Details | Diff
Updated patch including testcase. (1.44 KB, patch)
2013-09-20 16:51 UTC, Jan-Benedict Glaw
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Husemann 2013-04-08 12:04:43 UTC
Created attachment 29823 [details]
Use the D format specifie for ashqs second arg

The documentation for VAX operand formatting codes says:

   D    64-bit immediate operand

and:

/* The purpose of D is to get around a quirk or bug in vax assembler
   whereby -1 in a 64-bit immediate operand means 0x00000000ffffffff,
   which is not a 64-bit minus one.  */

However, the ashq instruction patters do not use this properly.

This small test program triggers it:

#include <stdio.h>
#include <inttypes.h>

int main(int argc, char **argv)
{
        size_t i;
        uint64_t v, nv;

        for (i = 0; i < 16; i++) {
                v = ~(uint64_t)0 << i;
                nv = ~v;
                printf("%zu: mask %08llx not %08llx\n", i, v, nv);
        }

        return 0;
}

The relevant line from assembly:

    ashq %r6,$-1,%r2

which is misinterpreted by the assembler, so dissasembly is:

   10637:       79 56 8f ff     ashq r6,$0x00000000ffffffff,r2
   1063b:       ff ff ff 00 
   1063f:       00 00 00 52 


The attached patch fixes it.
Comment 1 Jan-Benedict Glaw 2013-09-16 21:06:46 UTC
The `gas' bug seems to only show up on 32bit host platform. Creating a cross-gas on a amd64 systems seems to always result in "correct" VAX binary output, even for old 2.21 releases. (Will further check this.)

I tend to not fix GCC in this regard. Its output is correct, though doing hex output instead of negative decimals would work-around gas's bug. However, I think we'd better fix gas and possibly add a version check to gcc?
Comment 2 Jan-Benedict Glaw 2013-09-17 01:06:23 UTC
This is gas's tc-vax.c:

3158                           if ((is_absolute) && (expP->X_op != O_big))
3159                             {
3160                               /* If nbytes > 4, then we are scrod. We
3161                                  don't know if the high order bytes
3162                                  are to be 0xFF or 0x00.  BSD4.2 & RMS
3163                                  say use 0x00. OK --- but this
3164                                  assembler needs ANOTHER rewrite to
3165                                  cope properly with this bug.  */
3166                               md_number_to_chars (p + 1, this_add_number,
3167                                                   min (sizeof (valueT),
3168                                                        (size_t) nbytes));
3169                               if ((size_t) nbytes > sizeof (valueT))
3170                                 memset (p + 1 + sizeof (valueT),
3171                                         '\0', nbytes - sizeof (valueT));
3172                             }
[...]

This is for "small" values (ie. "-1" fitting in a 32bit valueT) and it doesn't properly sign-extend in this case. Taking into account the next couple lines (not shown here), this part of the code needs to rethought.

A workaround would be to actually place an all-hex value (through GCC) here, but that wouldn't fix gas for any hand-written assembler.
Comment 3 Martin Husemann 2013-09-17 07:11:54 UTC
Of course I do not mind fixing gas, but the whole point of the "D" formatting code is to work around this assembler bug, and while this might be mostly irrelevant nowadays, isn't gcc supposed to work with other assemblers (like the VMS one) as well? Gas seems to be "bug-compatible" here.

Overall, especially since this change is trivial, wouldn't it be best/easiest to apply it anyway?
Comment 4 Jan-Benedict Glaw 2013-09-17 07:20:57 UTC
You're quite right, Martin!  It's a simple fix on the GCC side working around a buggy gas, and it would also work with a fixed gas. However, gas isn't too simple to fix, at least not with a well-architected fix.
Comment 5 Jan-Benedict Glaw 2013-09-20 16:51:23 UTC
Created attachment 30872 [details]
Updated patch including testcase.
Comment 6 jbglaw 2013-09-20 19:00:05 UTC
Author: jbglaw
Date: Fri Sep 20 19:00:02 2013
New Revision: 202796

URL: http://gcc.gnu.org/viewcvs?rev=202796&root=gcc&view=rev
Log:
Work around buggy gas not properly sign-extending a 64bit value on a 32bit host

PR target/56875
2013-09-20  Martin Husemann  <martin@NetBSD.org>
	    Jan-Benedict Glaw  <jbglaw@lug-owl.de>

gcc/
	* config/vax/vax.c (vax_output_int_move): Use D format specifier.
	* config/vax/vax.md (ashldi3, <unnamed>): Ditto.

gcc/testsuite/
	* gcc.target/vax/vax.exp: New.
	* gcc.target/vax/pr56875.c: Ditto.

Added:
    trunk/gcc/testsuite/gcc.target/vax/
    trunk/gcc/testsuite/gcc.target/vax/pr56875.c
    trunk/gcc/testsuite/gcc.target/vax/vax.exp
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/vax/vax.c
    trunk/gcc/config/vax/vax.md
    trunk/gcc/testsuite/ChangeLog
Comment 7 Martin Husemann 2013-09-21 08:18:24 UTC
fixed on trunk