[GAS,ARM] Fix testism for bl local v4t test

Message ID 5BC9F15F.7020403@arm.com
State New
Headers show
Series
  • [GAS,ARM] Fix testism for bl local v4t test
Related show

Commit Message

Andre Vieira (lists) Oct. 19, 2018, 2:59 p.m.
Hi,

This patch fixes a testism.
I believe this test was never really intended to test for this error
message and
'# stderr' never actually checked it.  This warning is tested elsewhere
and to my
understanding it would have not made any sense here either.

I did some archaeological digging and found that the original patch that
was reviewed on the ml contained a blx-thumb-local.{d,s,l}.
See https://sourceware.org/ml/binutils/2009-03/msg00511.html
The commit itself was missing the .s and .d files, I suspect these fell
through the 'svn add' crack and they help explain why there is a
blx-thumb-local.l to begin with.
I re-added them in this patch and updated due to bitrot.

Is this OK for trunk?

gas/ChangeLog

2018-10-19  Andre Vieira  <andre.simoesdiasvieira@arm.com>

        * testsuite/gas/arm/bl-local-v4t.d: Remove
        warning check.
        * testsuite/gas/arm/blx-local-thumb.s: New.
        * testsuite/gas/arm/blx-local-thumb.d: New.

Comments

Thomas Preudhomme Oct. 26, 2018, 10:38 a.m. | #1
Hi Andre,

On Fri, 19 Oct 2018 at 15:59, Andre Vieira (lists)
<Andre.SimoesDiasVieira@arm.com> wrote:
>

> Hi,

>

> This patch fixes a testism.

> I believe this test was never really intended to test for this error

> message and

> '# stderr' never actually checked it.  This warning is tested elsewhere

> and to my

> understanding it would have not made any sense here either.

>

> I did some archaeological digging and found that the original patch that

> was reviewed on the ml contained a blx-thumb-local.{d,s,l}.

> See https://sourceware.org/ml/binutils/2009-03/msg00511.html

> The commit itself was missing the .s and .d files, I suspect these fell

> through the 'svn add' crack and they help explain why there is a

> blx-thumb-local.l to begin with.

> I re-added them in this patch and updated due to bitrot.

>

> Is this OK for trunk?


The commit 2ac93be706418f3b2aebeb22159a328023faed52 removed support
for arm aout and coff targets, you can see that it simplifies the very
same skip line to skipping only wince and pe arm targets and would
therefore suggest to adapt the test you add to do the same.

Beyond that, there is mention of relocation for foo so I'd suggest
adding a readelf -r action and check for those relocation.

Best regards,

Thomas

>

> gas/ChangeLog

>

> 2018-10-19  Andre Vieira  <andre.simoesdiasvieira@arm.com>

>

>         * testsuite/gas/arm/bl-local-v4t.d: Remove

>         warning check.

>         * testsuite/gas/arm/blx-local-thumb.s: New.

>         * testsuite/gas/arm/blx-local-thumb.d: New.
Andre Vieira (lists) Oct. 26, 2018, 2:18 p.m. | #2
Hi Thomas,

Thanks for the review.
On 26/10/18 11:38, Thomas Preudhomme wrote:
> Hi Andre,

> 

> On Fri, 19 Oct 2018 at 15:59, Andre Vieira (lists)

> <Andre.SimoesDiasVieira@arm.com> wrote:

>>

>> Hi,

>>

>> This patch fixes a testism.

>> I believe this test was never really intended to test for this error

>> message and

>> '# stderr' never actually checked it.  This warning is tested elsewhere

>> and to my

>> understanding it would have not made any sense here either.

>>

>> I did some archaeological digging and found that the original patch that

>> was reviewed on the ml contained a blx-thumb-local.{d,s,l}.

>> See https://sourceware.org/ml/binutils/2009-03/msg00511.html

>> The commit itself was missing the .s and .d files, I suspect these fell

>> through the 'svn add' crack and they help explain why there is a

>> blx-thumb-local.l to begin with.

>> I re-added them in this patch and updated due to bitrot.

>>

>> Is this OK for trunk?

> 

> The commit 2ac93be706418f3b2aebeb22159a328023faed52 removed support

> for arm aout and coff targets, you can see that it simplifies the very

> same skip line to skipping only wince and pe arm targets and would

> therefore suggest to adapt the test you add to do the same.

> 

Done.

> Beyond that, there is mention of relocation for foo so I'd suggest

> adding a readelf -r action and check for those relocation.

If you mean in bl-local-v4t's R_ARM_THM_CALL relocation on foo2, then I
feel that is sufficiently checked with objdump as it prints relocations
as comments.

blx-local-thumb has no relocations and I didn't expect it to have any.

Cheers,
Andre
diff --git a/gas/testsuite/gas/arm/bl-local-v4t.d b/gas/testsuite/gas/arm/bl-local-v4t.d
index 2985ceee906a06111fcbca6a8d081a79405c4e6c..cf68093988d91d99903017ac772d593779ff46d6 100644
--- a/gas/testsuite/gas/arm/bl-local-v4t.d
+++ b/gas/testsuite/gas/arm/bl-local-v4t.d
@@ -2,7 +2,6 @@
 #objdump: -drw --prefix-addresses --show-raw-insn
 #target: *-*-*eabi* *-*-nacl*
 #as:
-#warning_output: blx-local-thumb.l
 
 .*: +file format .*arm.*
 Disassembly of section .text:
diff --git a/gas/testsuite/gas/arm/blx-local-thumb.d b/gas/testsuite/gas/arm/blx-local-thumb.d
new file mode 100644
index 0000000000000000000000000000000000000000..a0a2c048f0b5ddf614bf677953875dfe38dbe5e4
--- /dev/null
+++ b/gas/testsuite/gas/arm/blx-local-thumb.d
@@ -0,0 +1,24 @@
+#name: Local BLX instructions in Thumb mode.
+#objdump: -drw --prefix-addresses --show-raw-insn
+#skip: *-*-pe *-*-wince
+#as:
+#warning_output: blx-local-thumb.l
+
+.*: +file format .*arm.*
+Disassembly of section .text:
+[^<]*<one> f000 f80e 	bl	00000020 <foo>
+[^<]*<one\+0x4> f000 e812 	blx	0000002c <foo2>
+[^<]*<one\+0x8> f000 f80a 	bl	00000020 <foo>
+[^<]*<one\+0xc> f000 e80e 	blx	0000002c <foo2>
+[^<]*<one\+0x10> f000 e80e 	blx	00000030 <fooundefarm>
+[^<]*<one\+0x14> f000 f80c 	bl	00000030 <fooundefarm>
+[^<]*<one\+0x18> f000 e806 	blx	00000028 <fooundefthumb>
+[^<]*<one\+0x1c> f000 f804 	bl	00000028 <fooundefthumb>
+[^<]*<foo> e7ee      	b.n	00000000 <one>
+[^<]*<foo\+0x2> e003      	b.n	0000002c <foo2>
+[^<]*<foo\+0x4> 46c0      	nop			; \(mov r8, r8\)
+[^<]*<foo\+0x6> 46c0      	nop			; \(mov r8, r8\)
+[^<]*<fooundefthumb> 46c0      	nop			; \(mov r8, r8\)
+	...
+[^<]*<foo2> e1a00000 	nop			; \(mov r0, r0\)
+[^<]*<fooundefarm> e1a00000 	nop			; \(mov r0, r0\)
diff --git a/gas/testsuite/gas/arm/blx-local-thumb.s b/gas/testsuite/gas/arm/blx-local-thumb.s
new file mode 100644
index 0000000000000000000000000000000000000000..504aa96e33e92b6da6cbdde24b610512184db585
--- /dev/null
+++ b/gas/testsuite/gas/arm/blx-local-thumb.s
@@ -0,0 +1,30 @@
+        .text
+	.arch armv5t
+	.syntax unified
+	.thumb
+one:
+        blx	foo   @ bl foo
+	blx     foo2  @ blx foo2
+	bl	foo   @ bl foo
+	bl	foo2  @ blx foo2
+	blx	fooundefarm
+	bl      fooundefarm
+	blx     fooundefthumb
+	bl      fooundefthumb
+	.thumb
+        .type foo, %function
+        .thumb_func
+foo:
+	b  one	@no relocs
+	b  foo2	@ THUMB_PCREL_JUMP
+        nop
+	nop
+fooundefthumb:
+	nop
+        .type foo2, %function
+	.arm
+	.align  2
+foo2:
+        nop
+fooundefarm:
+	nop
Andre Vieira (lists) Oct. 30, 2018, 11:18 a.m. | #3
On 26/10/18 15:18, Andre Vieira (lists) wrote:
> Hi Thomas,

> 

> Thanks for the review.

> On 26/10/18 11:38, Thomas Preudhomme wrote:

>> Hi Andre,

>>

>> On Fri, 19 Oct 2018 at 15:59, Andre Vieira (lists)

>> <Andre.SimoesDiasVieira@arm.com> wrote:

>>>

>>> Hi,

>>>

>>> This patch fixes a testism.

>>> I believe this test was never really intended to test for this error

>>> message and

>>> '# stderr' never actually checked it.  This warning is tested elsewhere

>>> and to my

>>> understanding it would have not made any sense here either.

>>>

>>> I did some archaeological digging and found that the original patch that

>>> was reviewed on the ml contained a blx-thumb-local.{d,s,l}.

>>> See https://sourceware.org/ml/binutils/2009-03/msg00511.html

>>> The commit itself was missing the .s and .d files, I suspect these fell

>>> through the 'svn add' crack and they help explain why there is a

>>> blx-thumb-local.l to begin with.

>>> I re-added them in this patch and updated due to bitrot.

>>>

>>> Is this OK for trunk?

>>

>> The commit 2ac93be706418f3b2aebeb22159a328023faed52 removed support

>> for arm aout and coff targets, you can see that it simplifies the very

>> same skip line to skipping only wince and pe arm targets and would

>> therefore suggest to adapt the test you add to do the same.

>>

> Done.

> 

>> Beyond that, there is mention of relocation for foo so I'd suggest

>> adding a readelf -r action and check for those relocation.

> If you mean in bl-local-v4t's R_ARM_THM_CALL relocation on foo2, then I

> feel that is sufficiently checked with objdump as it prints relocations

> as comments.

> 

> blx-local-thumb has no relocations and I didn't expect it to have any.

> 

> Cheers,

> Andre

> 

Ping
Andre Vieira (lists) Nov. 9, 2018, 4:33 p.m. | #4
On 30/10/18 11:18, Andre Vieira (lists) wrote:
> On 26/10/18 15:18, Andre Vieira (lists) wrote:

>> Hi Thomas,

>>

>> Thanks for the review.

>> On 26/10/18 11:38, Thomas Preudhomme wrote:

>>> Hi Andre,

>>>

>>> On Fri, 19 Oct 2018 at 15:59, Andre Vieira (lists)

>>> <Andre.SimoesDiasVieira@arm.com> wrote:

>>>>

>>>> Hi,

>>>>

>>>> This patch fixes a testism.

>>>> I believe this test was never really intended to test for this error

>>>> message and

>>>> '# stderr' never actually checked it.  This warning is tested elsewhere

>>>> and to my

>>>> understanding it would have not made any sense here either.

>>>>

>>>> I did some archaeological digging and found that the original patch that

>>>> was reviewed on the ml contained a blx-thumb-local.{d,s,l}.

>>>> See https://sourceware.org/ml/binutils/2009-03/msg00511.html

>>>> The commit itself was missing the .s and .d files, I suspect these fell

>>>> through the 'svn add' crack and they help explain why there is a

>>>> blx-thumb-local.l to begin with.

>>>> I re-added them in this patch and updated due to bitrot.

>>>>

>>>> Is this OK for trunk?

>>>

>>> The commit 2ac93be706418f3b2aebeb22159a328023faed52 removed support

>>> for arm aout and coff targets, you can see that it simplifies the very

>>> same skip line to skipping only wince and pe arm targets and would

>>> therefore suggest to adapt the test you add to do the same.

>>>

>> Done.

>>

>>> Beyond that, there is mention of relocation for foo so I'd suggest

>>> adding a readelf -r action and check for those relocation.

>> If you mean in bl-local-v4t's R_ARM_THM_CALL relocation on foo2, then I

>> feel that is sufficiently checked with objdump as it prints relocations

>> as comments.

>>

>> blx-local-thumb has no relocations and I didn't expect it to have any.

>>

>> Cheers,

>> Andre

>>

> Ping

> 

Ping
Thomas Preudhomme Nov. 19, 2018, 1:40 p.m. | #5
Hi Andre,

Sorry for the delay. I was confused by the THUMB_PCREL_JUMP comment, I
suppose that's an internal relocation. LGTM now but as I said I'm not
a maintainer so cannot approve your patch.

Best regards,

Thomas
On Fri, 9 Nov 2018 at 16:33, Andre Vieira (lists)
<Andre.SimoesDiasVieira@arm.com> wrote:
>

> On 30/10/18 11:18, Andre Vieira (lists) wrote:

> > On 26/10/18 15:18, Andre Vieira (lists) wrote:

> >> Hi Thomas,

> >>

> >> Thanks for the review.

> >> On 26/10/18 11:38, Thomas Preudhomme wrote:

> >>> Hi Andre,

> >>>

> >>> On Fri, 19 Oct 2018 at 15:59, Andre Vieira (lists)

> >>> <Andre.SimoesDiasVieira@arm.com> wrote:

> >>>>

> >>>> Hi,

> >>>>

> >>>> This patch fixes a testism.

> >>>> I believe this test was never really intended to test for this error

> >>>> message and

> >>>> '# stderr' never actually checked it.  This warning is tested elsewhere

> >>>> and to my

> >>>> understanding it would have not made any sense here either.

> >>>>

> >>>> I did some archaeological digging and found that the original patch that

> >>>> was reviewed on the ml contained a blx-thumb-local.{d,s,l}.

> >>>> See https://sourceware.org/ml/binutils/2009-03/msg00511.html

> >>>> The commit itself was missing the .s and .d files, I suspect these fell

> >>>> through the 'svn add' crack and they help explain why there is a

> >>>> blx-thumb-local.l to begin with.

> >>>> I re-added them in this patch and updated due to bitrot.

> >>>>

> >>>> Is this OK for trunk?

> >>>

> >>> The commit 2ac93be706418f3b2aebeb22159a328023faed52 removed support

> >>> for arm aout and coff targets, you can see that it simplifies the very

> >>> same skip line to skipping only wince and pe arm targets and would

> >>> therefore suggest to adapt the test you add to do the same.

> >>>

> >> Done.

> >>

> >>> Beyond that, there is mention of relocation for foo so I'd suggest

> >>> adding a readelf -r action and check for those relocation.

> >> If you mean in bl-local-v4t's R_ARM_THM_CALL relocation on foo2, then I

> >> feel that is sufficiently checked with objdump as it prints relocations

> >> as comments.

> >>

> >> blx-local-thumb has no relocations and I didn't expect it to have any.

> >>

> >> Cheers,

> >> Andre

> >>

> > Ping

> >

> Ping
Andre Vieira (lists) Nov. 22, 2018, 5:16 p.m. | #6
On 19/11/18 13:40, Thomas Preudhomme wrote:
> Hi Andre,

> 

> Sorry for the delay. I was confused by the THUMB_PCREL_JUMP comment, I

> suppose that's an internal relocation. LGTM now but as I said I'm not

> a maintainer so cannot approve your patch.

> 

> Best regards,

> 

> Thomas

> On Fri, 9 Nov 2018 at 16:33, Andre Vieira (lists)

> <Andre.SimoesDiasVieira@arm.com> wrote:

>>

>> On 30/10/18 11:18, Andre Vieira (lists) wrote:

>>> On 26/10/18 15:18, Andre Vieira (lists) wrote:

>>>> Hi Thomas,

>>>>

>>>> Thanks for the review.

>>>> On 26/10/18 11:38, Thomas Preudhomme wrote:

>>>>> Hi Andre,

>>>>>

>>>>> On Fri, 19 Oct 2018 at 15:59, Andre Vieira (lists)

>>>>> <Andre.SimoesDiasVieira@arm.com> wrote:

>>>>>>

>>>>>> Hi,

>>>>>>

>>>>>> This patch fixes a testism.

>>>>>> I believe this test was never really intended to test for this error

>>>>>> message and

>>>>>> '# stderr' never actually checked it.  This warning is tested elsewhere

>>>>>> and to my

>>>>>> understanding it would have not made any sense here either.

>>>>>>

>>>>>> I did some archaeological digging and found that the original patch that

>>>>>> was reviewed on the ml contained a blx-thumb-local.{d,s,l}.

>>>>>> See https://sourceware.org/ml/binutils/2009-03/msg00511.html

>>>>>> The commit itself was missing the .s and .d files, I suspect these fell

>>>>>> through the 'svn add' crack and they help explain why there is a

>>>>>> blx-thumb-local.l to begin with.

>>>>>> I re-added them in this patch and updated due to bitrot.

>>>>>>

>>>>>> Is this OK for trunk?

>>>>>

>>>>> The commit 2ac93be706418f3b2aebeb22159a328023faed52 removed support

>>>>> for arm aout and coff targets, you can see that it simplifies the very

>>>>> same skip line to skipping only wince and pe arm targets and would

>>>>> therefore suggest to adapt the test you add to do the same.

>>>>>

>>>> Done.

>>>>

>>>>> Beyond that, there is mention of relocation for foo so I'd suggest

>>>>> adding a readelf -r action and check for those relocation.

>>>> If you mean in bl-local-v4t's R_ARM_THM_CALL relocation on foo2, then I

>>>> feel that is sufficiently checked with objdump as it prints relocations

>>>> as comments.

>>>>

>>>> blx-local-thumb has no relocations and I didn't expect it to have any.

>>>>

>>>> Cheers,

>>>> Andre

>>>>

>>> Ping

>>>

>> Ping

Ping
Nick Clifton Nov. 23, 2018, 9:57 a.m. | #7
Hi Andre,

>>>>>>> Is this OK for trunk?


Approved - please apply.

(Sorry for the delay, I was hoping that one the ARM maintainers would review this patch).

Cheers
  Nick

Patch

diff --git a/gas/testsuite/gas/arm/bl-local-v4t.d b/gas/testsuite/gas/arm/bl-local-v4t.d
index 2985ceee906a06111fcbca6a8d081a79405c4e6c..cf68093988d91d99903017ac772d593779ff46d6 100644
--- a/gas/testsuite/gas/arm/bl-local-v4t.d
+++ b/gas/testsuite/gas/arm/bl-local-v4t.d
@@ -2,7 +2,6 @@ 
 #objdump: -drw --prefix-addresses --show-raw-insn
 #target: *-*-*eabi* *-*-nacl*
 #as:
-#warning_output: blx-local-thumb.l
 
 .*: +file format .*arm.*
 Disassembly of section .text:
diff --git a/gas/testsuite/gas/arm/blx-local-thumb.d b/gas/testsuite/gas/arm/blx-local-thumb.d
new file mode 100644
index 0000000000000000000000000000000000000000..775c6783e7ec09b90d6164fd8a861ba4a34ef50b
--- /dev/null
+++ b/gas/testsuite/gas/arm/blx-local-thumb.d
@@ -0,0 +1,24 @@ 
+#name: Local BLX instructions in Thumb mode.
+#objdump: -drw --prefix-addresses --show-raw-insn
+#skip: *-*-*coff *-*-pe *-*-wince *-*-*aout* *-*-netbsd *-*-riscix*
+#as:
+#warning_output: blx-local-thumb.l
+
+.*: +file format .*arm.*
+Disassembly of section .text:
+[^<]*<one> f000 f80e 	bl	00000020 <foo>
+[^<]*<one\+0x4> f000 e812 	blx	0000002c <foo2>
+[^<]*<one\+0x8> f000 f80a 	bl	00000020 <foo>
+[^<]*<one\+0xc> f000 e80e 	blx	0000002c <foo2>
+[^<]*<one\+0x10> f000 e80e 	blx	00000030 <fooundefarm>
+[^<]*<one\+0x14> f000 f80c 	bl	00000030 <fooundefarm>
+[^<]*<one\+0x18> f000 e806 	blx	00000028 <fooundefthumb>
+[^<]*<one\+0x1c> f000 f804 	bl	00000028 <fooundefthumb>
+[^<]*<foo> e7ee      	b.n	00000000 <one>
+[^<]*<foo\+0x2> e003      	b.n	0000002c <foo2>
+[^<]*<foo\+0x4> 46c0      	nop			; \(mov r8, r8\)
+[^<]*<foo\+0x6> 46c0      	nop			; \(mov r8, r8\)
+[^<]*<fooundefthumb> 46c0      	nop			; \(mov r8, r8\)
+	...
+[^<]*<foo2> e1a00000 	nop			; \(mov r0, r0\)
+[^<]*<fooundefarm> e1a00000 	nop			; \(mov r0, r0\)
diff --git a/gas/testsuite/gas/arm/blx-local-thumb.s b/gas/testsuite/gas/arm/blx-local-thumb.s
new file mode 100644
index 0000000000000000000000000000000000000000..504aa96e33e92b6da6cbdde24b610512184db585
--- /dev/null
+++ b/gas/testsuite/gas/arm/blx-local-thumb.s
@@ -0,0 +1,30 @@ 
+        .text
+	.arch armv5t
+	.syntax unified
+	.thumb
+one:
+        blx	foo   @ bl foo
+	blx     foo2  @ blx foo2
+	bl	foo   @ bl foo
+	bl	foo2  @ blx foo2
+	blx	fooundefarm
+	bl      fooundefarm
+	blx     fooundefthumb
+	bl      fooundefthumb
+	.thumb
+        .type foo, %function
+        .thumb_func
+foo:
+	b  one	@no relocs
+	b  foo2	@ THUMB_PCREL_JUMP
+        nop
+	nop
+fooundefthumb:
+	nop
+        .type foo2, %function
+	.arm
+	.align  2
+foo2:
+        nop
+fooundefarm:
+	nop