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

[Andre Vieira (lists)]

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 <***@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.

[Thomas Preudhomme]

Hi Andre,

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

Post by Andre Vieira (lists)
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

Post by Andre Vieira (lists)
gas/ChangeLog
* 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)]

Hi Thomas,

Thanks for the review.

Post by Thomas Preudhomme
Hi Andre,
On Fri, 19 Oct 2018 at 15:59, Andre Vieira (lists)

Post by Andre Vieira (lists)
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.

Post by Thomas Preudhomme
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

[Andre Vieira (lists)]

Post by Andre Vieira (lists)
Hi Thomas,
Thanks for the review.

Post by Thomas Preudhomme
Hi Andre,
On Fri, 19 Oct 2018 at 15:59, Andre Vieira (lists)

Post by Andre Vieira (lists)
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.

Post by Thomas Preudhomme
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)]

Post by Andre Vieira (lists)
Hi Thomas,
Thanks for the review.

Post by Thomas Preudhomme
Hi Andre,
On Fri, 19 Oct 2018 at 15:59, Andre Vieira (lists)

Post by Andre Vieira (lists)
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.

Post by Thomas Preudhomme
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]

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)

Post by Andre Vieira (lists)
Hi Thomas,
Thanks for the review.

Post by Thomas Preudhomme
Hi Andre,
On Fri, 19 Oct 2018 at 15:59, Andre Vieira (lists)

Post by Andre Vieira (lists)
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.

Post by Thomas Preudhomme
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)]

Post by Thomas Preudhomme
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)

Post by Andre Vieira (lists)
Hi Thomas,
Thanks for the review.

Post by Thomas Preudhomme
Hi Andre,
On Fri, 19 Oct 2018 at 15:59, Andre Vieira (lists)

Post by Andre Vieira (lists)
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.

Post by Thomas Preudhomme
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]

Hi Andre,

Post by Andre Vieira (lists)
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