[PATCH][AArch64] Fix ILP32 memory access

[PATCH][AArch64] Fix ILP32 memory access

[Wilco Dijkstra]

This patch fixes a failure in gcc.target/aarch64/reload-valid-spoff.c 
triggered by https://gcc.gnu.org/ml/gcc-patches/2017-06/msg01367.html -
it supersedes https://gcc.gnu.org/ml/gcc-patches/2017-06/msg01907.html
as this fixes the root cause of the failure.

In ILP32 all memory accesses must have Pmode as the base address, but
aarch64_expand_mov_immediate wasn't emitting a conversion in one case.
Besides fixing this add an assert that flags any MEM operands that are
not Pmode.

Passes regress (with/without ilp32). OK for commit?

ChangeLog:
2017-06-27  Wilco Dijkstra  <wdijkstr@arm.com>

	* config/aarch64/aarch64 (aarch64_expand_mov_immediate):
	Convert memory address to Pmode.
--
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 329d244e9cf16dbdf849e5dd02b3999caf0cd5a7..9038748ba049ba589f067f3f04c31704fe673d2c 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -1958,6 +1958,8 @@ aarch64_expand_mov_immediate (rtx dest, rtx imm)
 	      gcc_assert (can_create_pseudo_p ());
 	      base = gen_reg_rtx (ptr_mode);
 	      aarch64_expand_mov_immediate (base, XEXP (mem, 0));
+	      if (ptr_mode != Pmode)
+		base = convert_memory_address (Pmode, base);
 	      mem = gen_rtx_MEM (ptr_mode, base);
 	    }
 
@@ -5207,6 +5209,7 @@ aarch64_print_operand (FILE *f, rtx x, int code)
 
 	case MEM:
 	  output_address (GET_MODE (x), XEXP (x, 0));
+	  gcc_assert (GET_MODE (XEXP (x, 0)) == Pmode);
 	  break;
 
 	case CONST:

Re: [PATCH][AArch64] Fix ILP32 memory access

[Richard Earnshaw (lists)]

On 27/06/17 14:39, Wilco Dijkstra wrote:
> This patch fixes a failure in gcc.target/aarch64/reload-valid-spoff.c 
> triggered by https://gcc.gnu.org/ml/gcc-patches/2017-06/msg01367.html -
> it supersedes https://gcc.gnu.org/ml/gcc-patches/2017-06/msg01907.html
> as this fixes the root cause of the failure.
> 
> In ILP32 all memory accesses must have Pmode as the base address, but
> aarch64_expand_mov_immediate wasn't emitting a conversion in one case.
> Besides fixing this add an assert that flags any MEM operands that are
> not Pmode.
> 
> Passes regress (with/without ilp32). OK for commit?
> 
> ChangeLog:
> 2017-06-27  Wilco Dijkstra  <wdijkstr@arm.com>
> 
> 	* config/aarch64/aarch64 (aarch64_expand_mov_immediate):
> 	Convert memory address to Pmode.

Missing ChangeLog entry for the new assert.

> --
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 329d244e9cf16dbdf849e5dd02b3999caf0cd5a7..9038748ba049ba589f067f3f04c31704fe673d2c 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -1958,6 +1958,8 @@ aarch64_expand_mov_immediate (rtx dest, rtx imm)
>  	      gcc_assert (can_create_pseudo_p ());
>  	      base = gen_reg_rtx (ptr_mode);
>  	      aarch64_expand_mov_immediate (base, XEXP (mem, 0));
> +	      if (ptr_mode != Pmode)
> +		base = convert_memory_address (Pmode, base);
>  	      mem = gen_rtx_MEM (ptr_mode, base);
>  	    }
>  
> @@ -5207,6 +5209,7 @@ aarch64_print_operand (FILE *f, rtx x, int code)
>  
>  	case MEM:
>  	  output_address (GET_MODE (x), XEXP (x, 0));
> +	  gcc_assert (GET_MODE (XEXP (x, 0)) == Pmode);
>  	  break;

This is worthy of a comment.  Something like "All memory references must
be in Pmode, which is the natural mode of the machine.  This remains the
case even if ptr_mode is different, as for ILP32."

Ok with those changes.

R.
>  
>  	case CONST:
> 

Re: [PATCH][AArch64] Fix ILP32 memory access

[Andreas Schwab]

On Jun 27 2017, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:

> @@ -5207,6 +5209,7 @@ aarch64_print_operand (FILE *f, rtx x, int code)
>  
>  	case MEM:
>  	  output_address (GET_MODE (x), XEXP (x, 0));
> +	  gcc_assert (GET_MODE (XEXP (x, 0)) == Pmode);
>  	  break;
>  
>  	case CONST:

That breaks a lot of gnat tests in ilp32 mode:

spawn -ignore SIGHUP /opt/gcc/gcc-20170630/Build/gcc/gnatmake --GCC=/opt/gcc/gcc-20170630/Build/gcc/xgcc --GNATBIND=/opt/gcc/gcc-20170630/Build/gcc/gnatbind --GNATLINK=/opt/gcc/gcc-20170630/Build/gcc/gnatlink -cargs -B/opt/gcc/gcc-20170630/Build/gcc -largs --GCC=/opt/gcc/gcc-20170630/Build/gcc/xgcc -B/opt/gcc/gcc-20170630/Build/gcc  -mabi=ilp32 -margs --RTS=/opt/gcc/gcc-20170630/Build/aarch64-suse-linux/ilp32/libada -q -f /opt/gcc/gcc-20170630/gcc/testsuite/gnat.dg/abstract_with_anonymous_result.adb -mabi=ilp32 -fno-diagnostics-show-caret -fdiagnostics-color=never -lm -o ./abstract_with_anonymous_result.exe
+===========================GNAT BUG DETECTED==============================+
| 8.0.0 20170630 (experimental) [trunk revision 249826] (aarch64-suse-linux) GCC error:|
| in aarch64_print_operand, at config/aarch64/aarch64.c:5271               |
| Error detected around /opt/gcc/gcc-20170630/gcc/testsuite/gnat.dg/abstract_with_anonymous_result.adb:37:5|
| Please submit a bug report; see https://gcc.gnu.org/bugs/ .              |
| Use a subject line meaningful to you and us to track the bug.            |
| Include the entire contents of this bug box in the report.               |
| Include the exact command that you entered.                              |
| Also include sources listed below.                                       |
+==========================================================================+


Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH][AArch64] Fix ILP32 memory access

[Wilco Dijkstra]

Andreas Schwab wrote:
> @@ -5207,6 +5209,7 @@ aarch64_print_operand (FILE *f, rtx x, int code)
>  
>        case MEM:
>          output_address (GET_MODE (x), XEXP (x, 0));
> +       gcc_assert (GET_MODE (XEXP (x, 0)) == Pmode);
>          break;
>  
>        case CONST:

> That breaks a lot of gnat tests in ilp32 mode:

That's interesting since it works fine in C and C++, and unless there are some
ADA specific MD patterns, it seems unlikely it could only affect ADA.

So how do you build the ADA backend? GCC can't even get past the configure as
it doesn't appear to understand GNAT is installed after finding it...
Is there some more magic setup required for ADA?

Wilco

checking for gnatbind... gnatbind
checking for gnatmake... gnatmake
checking whether compiler driver understands Ada... no
checking how to compare bootstrapped objects... cmp --ignore-initial=16 $$f1 $$f2
checking for objdir... .libs
configure: WARNING: using in-tree isl, disabling version check
configure: error: GNAT is required to build ada

>gnat -v
GNAT 7.1.0
Copyright 1996-2017, Free Software Foundation, Inc.

List of available commands
...

Re: [PATCH][AArch64] Fix ILP32 memory access

[Andreas Schwab]

On Jul 04 2017, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:

> checking whether compiler driver understands Ada... no

You need to fix that first.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH][AArch64] Fix ILP32 memory access

[Arnaud Charlet]

On Tue, Jul 04, 2017 at 12:19:35PM +0000, Wilco Dijkstra wrote:
> Andreas Schwab wrote:
> > @@ -5207,6 +5209,7 @@ aarch64_print_operand (FILE *f, rtx x, int code)
> >  
> >        case MEM:
> >          output_address (GET_MODE (x), XEXP (x, 0));
> > +       gcc_assert (GET_MODE (XEXP (x, 0)) == Pmode);
> >          break;
> >  
> >        case CONST:
> 
> > That breaks a lot of gnat tests in ilp32 mode:
> 
> That's interesting since it works fine in C and C++, and unless there are some
> ADA specific MD patterns, it seems unlikely it could only affect ADA.
> 
> So how do you build the ADA backend? GCC can't even get past the configure as
> it doesn't appear to understand GNAT is installed after finding it...
> Is there some more magic setup required for ADA?
> 
> Wilco
> 
> checking for gnatbind... gnatbind
> checking for gnatmake... gnatmake
> checking whether compiler driver understands Ada... no

The line above means that using $CC -c dummy_ada_file.adb
generates an error.

$CC is "gcc" by default, some installs use gnatgcc instead of gcc to
provide an Ada compiler.

In other words, try to do:

gcc -c hello.adb

manually and you'll see the error for yourself, or check your config.log
file.

hello.adb can be as simple as the following single line:

procedure hello is begin null; end;

Re: [PATCH][AArch64] Fix ILP32 memory access

[Ramana Radhakrishnan]

On Tue, Jul 4, 2017 at 1:56 PM, Arnaud Charlet <charlet@adacore.com> wrote:
> On Tue, Jul 04, 2017 at 12:19:35PM +0000, Wilco Dijkstra wrote:
>> Andreas Schwab wrote:
>> > @@ -5207,6 +5209,7 @@ aarch64_print_operand (FILE *f, rtx x, int code)
>> >
>> >        case MEM:
>> >          output_address (GET_MODE (x), XEXP (x, 0));
>> > +       gcc_assert (GET_MODE (XEXP (x, 0)) == Pmode);
>> >          break;
>> >
>> >        case CONST:
>>
>> > That breaks a lot of gnat tests in ilp32 mode:
>>
>> That's interesting since it works fine in C and C++, and unless there are some
>> ADA specific MD patterns, it seems unlikely it could only affect ADA.
>>
>> So how do you build the ADA backend? GCC can't even get past the configure as
>> it doesn't appear to understand GNAT is installed after finding it...
>> Is there some more magic setup required for ADA?
>>
>> Wilco
>>
>> checking for gnatbind... gnatbind
>> checking for gnatmake... gnatmake
>> checking whether compiler driver understands Ada... no
>
> The line above means that using $CC -c dummy_ada_file.adb
> generates an error.
>
> $CC is "gcc" by default, some installs use gnatgcc instead of gcc to
> provide an Ada compiler.
>
> In other words, try to do:
>
> gcc -c hello.adb
>
> manually and you'll see the error for yourself, or check your config.log
> file.
>
> hello.adb can be as simple as the following single line:
>
> procedure hello is begin null; end;

Yeah it turns out that on the machine Wilco was using, we are running
14.04 which has a gcc 4.8 base compiler that didn't have Ada on for
AArch64.

I think we can work around by installing a gcc-5 package and then
setting CC to gcc-5.

Ramana

Re: [PATCH][AArch64] Fix ILP32 memory access

[Michael Matz]

Hi,

On Tue, 4 Jul 2017, Ramana Radhakrishnan wrote:

> Yeah it turns out that on the machine Wilco was using, we are running 
> 14.04 which has a gcc 4.8 base compiler that didn't have Ada on for 
> AArch64.
> 
> I think we can work around by installing a gcc-5 package and then 
> setting CC to gcc-5.

You'll probably also have to set GNATBIND and GNATMAKE to the 
appropriately suffixed variants.  Just saying, because that's what I'm 
usually forgetting and end up with strange errors :)


Ciao,
Michael.

Re: [PATCH][AArch64] Fix ILP32 memory access

[Wilco Dijkstra]

Michael Matz wrote:
>
> You'll probably also have to set GNATBIND and GNATMAKE to the 
> appropriately suffixed variants.  Just saying, because that's what I'm 
> usually forgetting and end up with strange errors :)

Configure seems to be able to find gnatbind/gnatmake as they are in /usr/bin.

Compiling a simple .adb file as suggested gives:

> /usr/bin/gcc-5 tmp.adb -S
gcc-5: error trying to exec 'gnat1': execvp: No such file or directory

Is this part of the GCC driver, GNAT or something else that needs to be installed first?

Wilco

Re: [PATCH][AArch64] Fix ILP32 memory access

[Michael Matz]

[-- Attachment #1: Type: text/plain, Size: 923 bytes --]

Hi,

On Tue, 4 Jul 2017, Wilco Dijkstra wrote:

> > You'll probably also have to set GNATBIND and GNATMAKE to the 
> > appropriately suffixed variants.  Just saying, because that's what I'm 
> > usually forgetting and end up with strange errors :)
> 
> Configure seems to be able to find gnatbind/gnatmake as they are in /usr/bin.

Sure, but if you use gcc-5 as compiler you'll want to use gnatbind-5 as 
well, not gnatbind (which presumably is from the default 4.8 compiler you 
have).

> Compiling a simple .adb file as suggested gives:
> 
> > /usr/bin/gcc-5 tmp.adb -S
> gcc-5: error trying to exec 'gnat1': execvp: No such file or directory
> 
> Is this part of the GCC driver, GNAT or something else that needs to be 
> installed first?

Don't know the package naming convention on Ubuntu (?).  For us it's e.g. 
gcc5-ada that you'd need to install in addition.  google tells me gnat-5 
should be it.


Ciao,
Michael.

Re: [PATCH][AArch64] Fix ILP32 memory access

[Andreas Schwab]

On Jul 04 2017, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:

> Configure seems to be able to find gnatbind/gnatmake as they are in /usr/bin.

Strange there are ada tools, but no ada compiler.  Are you sure you
installed all relevant ada packages?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH][AArch64] Fix ILP32 memory access

[Ramana Radhakrishnan]

On Tue, Jul 4, 2017 at 2:53 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Tue, 4 Jul 2017, Wilco Dijkstra wrote:
>
>> > You'll probably also have to set GNATBIND and GNATMAKE to the
>> > appropriately suffixed variants.  Just saying, because that's what I'm
>> > usually forgetting and end up with strange errors :)
>>
>> Configure seems to be able to find gnatbind/gnatmake as they are in /usr/bin.
>
> Sure, but if you use gcc-5 as compiler you'll want to use gnatbind-5 as
> well, not gnatbind (which presumably is from the default 4.8 compiler you
> have).
>
>> Compiling a simple .adb file as suggested gives:
>>
>> > /usr/bin/gcc-5 tmp.adb -S
>> gcc-5: error trying to exec 'gnat1': execvp: No such file or directory
>>
>> Is this part of the GCC driver, GNAT or something else that needs to be
>> installed first?
>
> Don't know the package naming convention on Ubuntu (?).  For us it's e.g.
> gcc5-ada that you'd need to install in addition.  google tells me gnat-5
> should be it.

gnat-7 was on the machine instead of gnat-5 and that was probably the
cause of the fun and games.

Ramana

>
>
> Ciao,
> Michael.

Re: [PATCH][AArch64] Fix ILP32 memory access

[Andrew Pinski]

On Tue, Jun 27, 2017 at 6:39 AM, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> This patch fixes a failure in gcc.target/aarch64/reload-valid-spoff.c
> triggered by https://gcc.gnu.org/ml/gcc-patches/2017-06/msg01367.html -
> it supersedes https://gcc.gnu.org/ml/gcc-patches/2017-06/msg01907.html
> as this fixes the root cause of the failure.
>
> In ILP32 all memory accesses must have Pmode as the base address, but
> aarch64_expand_mov_immediate wasn't emitting a conversion in one case.
> Besides fixing this add an assert that flags any MEM operands that are
> not Pmode.
>
> Passes regress (with/without ilp32). OK for commit?

This looks related to PR 80266 in that one was crashing due to the
store pair instruction like what was reported.

Thanks,
Andrew


>
> ChangeLog:
> 2017-06-27  Wilco Dijkstra  <wdijkstr@arm.com>
>
>         * config/aarch64/aarch64 (aarch64_expand_mov_immediate):
>         Convert memory address to Pmode.
> --
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 329d244e9cf16dbdf849e5dd02b3999caf0cd5a7..9038748ba049ba589f067f3f04c31704fe673d2c 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -1958,6 +1958,8 @@ aarch64_expand_mov_immediate (rtx dest, rtx imm)
>               gcc_assert (can_create_pseudo_p ());
>               base = gen_reg_rtx (ptr_mode);
>               aarch64_expand_mov_immediate (base, XEXP (mem, 0));
> +             if (ptr_mode != Pmode)
> +               base = convert_memory_address (Pmode, base);
>               mem = gen_rtx_MEM (ptr_mode, base);
>             }
>
> @@ -5207,6 +5209,7 @@ aarch64_print_operand (FILE *f, rtx x, int code)
>
>         case MEM:
>           output_address (GET_MODE (x), XEXP (x, 0));
> +         gcc_assert (GET_MODE (XEXP (x, 0)) == Pmode);
>           break;
>
>         case CONST:

Re: [PATCH][AArch64] Fix ILP32 memory access

[Wilco Dijkstra]

Andrew Pinski wrote:
>
> This looks related to PR 80266 in that one was crashing due to the
> store pair instruction like what was reported.

Yes it's the same bug. I've now finally reproduced it, it seems many stack 
addresses in Ada are SImode which is incorrect (and ultimately can trigger
the LDP assertion like it did with -mcmodel=large). I don't see any checks in
the backend that enforce Pmode for addresses, for example
aarch64_classify_address doesn't reject SImode addresses...

Wilco