[PATCH 0/2] MIPS/GCC/testsuite: Fixes for data-sym-pool.c

[PATCH 0/2] MIPS/GCC/testsuite: Fixes for data-sym-pool.c

[Maciej W. Rozycki]

Hi,

 Paul has recently reported a regression test failure with data-sym-pool.c 
at -O0 in his configuration.  After some tweaking I was able to reproduce 
it with mine.  I also realised we need an n64 variant, hence this has 
become a mini patch series.

 As these changes are test suite updates only, not affecting code 
generation in any way and so limited in scope, I propose to have them 
included in GCC 8 despite it being so close to release.

 See individual changes for details.

  Maciej

[PATCH 1/2] MIPS/GCC/testsuite: Fix data-sym-pool.c for SVR4 model at -O0

[Maciej W. Rozycki]

With GCC configurations using the SVR4 rather than the PLT dynamic 
executable model and the o32 ABI with the data-sym-pool.c test case code 
like below is produced:

	.file	1 "data-sym-pool.c"
	.section .mdebug.abi32
	.previous
	.nan	legacy
	.module	fp=xx
	.module	nooddspreg
	.abicalls
	.text
	.align	2
	.globl	frob
	.set	mips16
	.set	nomicromips
	.ent	frob
	.type	frob, @function
frob:
	.frame	$17,8,$31		# vars= 0, regs= 1/0, args= 0, gp= 0
	.mask	0x00020000,-4
	.fmask	0x00000000,0
	save	8,$17
	move	$17,$sp
	lw	$2,$L4
	move	$sp,$17
	restore	8,$17
	jr	$31
	.type	__pool_frob_3, @object
__pool_frob_3:
	.align	2
$L3:
	.word	__gnu_local_gp
$L4:
	.word	305419896
	.type	__pend_frob_3, @function
__pend_frob_3:
	.insn
	.end	frob
	.size	frob, .-frob
	.ident	"GCC: (GNU) 8.0.1 20180410 (experimental)"

causing a failure due to the unexpected `__gnu_local_gp' entry in the 
constant pool, even though there is nothing wrong with it as far as the 
annotation being examined is concerned.

Given that the SVR4 vs PLT code model consideration is irrelevant for 
this test case rather than rewriting the regular expression to match 
this variant of code just enforce the PLT model by using the `-mplt' 
option.  It is safe to use this option unconditionally as it is silently 
ignored with configurations that do not support this model, e.g. bare 
metal ELF.

	gcc/testsuite/
	* gcc.target/mips/data-sym-pool.c (dg-options): Add `-mplt'.
---
Hi,

 I have regression-tested this with the `mips-mti-linux-gnu' target and 
the o32, n32 and n64 ABIs.  The two latters are demoted to o64 by the test 
framework due to the lack of MIPS16 support for the hard-float variants of 
these ABIs and I don't have soft-float multilibs configured, so instead I 
have verified n32/soft-float and n64/soft-float variants by hand.  The 
latter revealed the need for 2/2.

 Finally I do not have a bare metal ELF configuration available for 
regression-testing right now, so I only verified that `-mplt' is silently 
ignored.  Code generated is expected to be the same as in the PLT mode.

 OK to apply?

  Maciej
---
 gcc/testsuite/gcc.target/mips/data-sym-pool.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

gcc-mips16-test-data-sym-pool-extra-entries.diff
Index: gcc/gcc/testsuite/gcc.target/mips/data-sym-pool.c
===================================================================
--- gcc.orig/gcc/testsuite/gcc.target/mips/data-sym-pool.c	2016-11-17 21:24:46.000000000 +0000
+++ gcc/gcc/testsuite/gcc.target/mips/data-sym-pool.c	2018-04-10 23:27:49.226719338 +0100
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-mips16 -mcode-readable=yes" } */
+/* { dg-options "-mips16 -mcode-readable=yes -mplt" } */
 
 int
 frob (void)
@@ -20,6 +20,10 @@ $L3:						# The label must match.
 __pend_frob_3:					# The symbol must match.
 	.insn
 
-   that is `__pool_*'/`__pend_*' symbols inserted around a constant pool.  */
+   that is `__pool_*'/`__pend_*' symbols inserted around a constant pool.
+
+   This code is built with `-mplt' to prevent the special `__gnu_local_gp'
+   symbol from being placed in the constant pool at `-O0' for SVR4 code
+   and consequently interfering with test expectations.  */
 
 /* { dg-final { scan-assembler "\tlw\t\\\$\[0-9\]+,(.L(\[0-9\]+))\n.*\t\\.type\t(__pool_frob_\\2), @object\n\\3:\n\t\\.align\t2\n\\1:\n\t\\.word\t305419896\n\t\\.type\t(__pend_frob_\\2), @function\n\\4:\n\t\\.insn\n" } } */

RE: [PATCH 1/2] MIPS/GCC/testsuite: Fix data-sym-pool.c for SVR4 model at -O0

[Matthew Fortune]

Maciej Rozycki <Maciej.Rozycki@mips.com> writes:
> Given that the SVR4 vs PLT code model consideration is irrelevant for
> this test case rather than rewriting the regular expression to match
> this variant of code just enforce the PLT model by using the `-mplt'
> option.  It is safe to use this option unconditionally as it is silently
> ignored with configurations that do not support this model, e.g. bare
> metal ELF.
> 
> 	gcc/testsuite/
> 	* gcc.target/mips/data-sym-pool.c (dg-options): Add `-mplt'.
> ---
> Hi,
> 
>  I have regression-tested this with the `mips-mti-linux-gnu' target and
> the o32, n32 and n64 ABIs.  The two latters are demoted to o64 by the
> test framework due to the lack of MIPS16 support for the hard-float
> variants of these ABIs and I don't have soft-float multilibs configured,
> so instead I have verified n32/soft-float and n64/soft-float variants by
> hand.  The latter revealed the need for 2/2.
> 
>  Finally I do not have a bare metal ELF configuration available for
> regression-testing right now, so I only verified that `-mplt' is
> silently ignored.  Code generated is expected to be the same as in the
> PLT mode.
> 
>  OK to apply?

Hi Maciej,

Sorry for skimming past this and being slow to respond. I agree with your
choice of forcing an option to stabilise the generated code it does tend
to future proof better than leaving options floating.

OK to apply but given the release date I've added RMs to approve for
trunk right now.

Thanks,
Matthew

RE: [PATCH 1/2] MIPS/GCC/testsuite: Fix data-sym-pool.c for SVR4 model at -O0

[Maciej W. Rozycki]

On Wed, 18 Apr 2018, Matthew Fortune wrote:

> OK to apply but given the release date I've added RMs to approve for
> trunk right now.

 Applied to trunk, now that GCC 8 has branched.  Thanks for your review.

  Maciej

[PATCH 2/2] MIPS/GCC/testsuite: Fix data-sym-pool.c for n64 code

[Maciej W. Rozycki]

With the soft-float n64 ABI and the data-sym-pool.c test case code like 
below is produced:

	.file	1 "data-sym-pool.c"
	.section .mdebug.abi64
	.previous
	.nan	legacy
	.module	softfloat
	.module	oddspreg
	.abicalls
	.option	pic0
	.text
	.align	2
	.globl	frob
	.set	mips16
	.set	nomicromips
	.ent	frob
	.type	frob, @function
frob:
	.frame	$17,16,$31		# vars= 0, regs= 1/0, args= 0, gp= 0
	.mask	0x00020000,-8
	.fmask	0x00000000,0
	daddiu	$sp,-16
	sd	$17,8($sp)
	move	$17,$sp
	ld	$2,.L3
	move	$sp,$17
	ld	$17,8($sp)
	daddiu	$sp,16
	jr	$31
	.type	__pool_frob_3, @object
__pool_frob_3:
	.align	3
.L3:
	.dword	305419896
	.type	__pend_frob_3, @function
__pend_frob_3:
	.insn
	.end	frob
	.size	frob, .-frob
	.ident	"GCC: (GNU) 8.0.1 20180410 (experimental)"

(we have no support for hard-float n64 MIPS16 code generation), which 
means that the test case will fail, as the regular expression pattern 
expects `lw' and `.word' rather than `ld' and `.dword' respectively to 
appear in assembly code generation.  Correct the pattern in an obvious 
way then making it accept both intructions and pseudo-ops.

	gcc/testsuite/
	* gcc.target/mips/data-sym-pool.c (dg-options): Match `ld' and
	`.dword' in addition to `lw' and `.word'.
---
Hi,

 I have regression-tested this with the `mips-mti-linux-gnu' target and 
the o32 ABI.  I don't have an n64 soft-float multilib configured, so the 
manually generated assembly file included with the description serves as 
the proof for what needs to be matched.

 OK to apply?

  Maciej
---
 gcc/testsuite/gcc.target/mips/data-sym-pool.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

gcc-mips16-test-data-sym-pool-dword.diff
Index: gcc/gcc/testsuite/gcc.target/mips/data-sym-pool.c
===================================================================
--- gcc.orig/gcc/testsuite/gcc.target/mips/data-sym-pool.c	2018-04-10 23:27:49.000000000 +0100
+++ gcc/gcc/testsuite/gcc.target/mips/data-sym-pool.c	2018-04-10 23:37:05.357453843 +0100
@@ -26,4 +26,4 @@ __pend_frob_3:					# The symbol must mat
    symbol from being placed in the constant pool at `-O0' for SVR4 code
    and consequently interfering with test expectations.  */
 
-/* { dg-final { scan-assembler "\tlw\t\\\$\[0-9\]+,(.L(\[0-9\]+))\n.*\t\\.type\t(__pool_frob_\\2), @object\n\\3:\n\t\\.align\t2\n\\1:\n\t\\.word\t305419896\n\t\\.type\t(__pend_frob_\\2), @function\n\\4:\n\t\\.insn\n" } } */
+/* { dg-final { scan-assembler "\tl\[wd\]\t\\\$\[0-9\]+,(.L(\[0-9\]+))\n.*\t\\.type\t(__pool_frob_\\2), @object\n\\3:\n\t\\.align\t2\n\\1:\n\t\\.d?word\t305419896\n\t\\.type\t(__pend_frob_\\2), @function\n\\4:\n\t\\.insn\n" } } */

RE: [PATCH 2/2] MIPS/GCC/testsuite: Fix data-sym-pool.c for n64 code

[Matthew Fortune]

Maciej Rozycki <Maciej.Rozycki@mips.com> writes:
> (we have no support for hard-float n64 MIPS16 code generation), which
> means that the test case will fail, as the regular expression pattern
> expects `lw' and `.word' rather than `ld' and `.dword' respectively to
> appear in assembly code generation.  Correct the pattern in an obvious
> way then making it accept both intructions and pseudo-ops.
> 
> 	gcc/testsuite/
> 	* gcc.target/mips/data-sym-pool.c (dg-options): Match `ld' and
> 	`.dword' in addition to `lw' and `.word'.
> ---
> Hi,
> 
>  I have regression-tested this with the `mips-mti-linux-gnu' target and
> the o32 ABI.  I don't have an n64 soft-float multilib configured, so the
> manually generated assembly file included with the description serves as
> the proof for what needs to be matched.
> 
>  OK to apply?

Hi Maciej,

Apologies as before on slowness. This looks trivially OK to me but that's
thanks to the description, much appreciated.

OK for commit but adding RMs given the imminent release.

Thanks,
Matthew

RE: [PATCH 2/2] MIPS/GCC/testsuite: Fix data-sym-pool.c for n64 code

[Maciej W. Rozycki]

On Wed, 18 Apr 2018, Matthew Fortune wrote:

> Apologies as before on slowness. This looks trivially OK to me but that's
> thanks to the description, much appreciated.

 You are welcome, that just means I did my homework right.

> OK for commit but adding RMs given the imminent release.

 Applied to trunk, now that GCC 8 has branched.  Thanks for your review.

  Maciej