This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [GAS][ARM] Fix testism for bl local v4t test
- From: Thomas Preudhomme <thomas dot preudhomme at linaro dot org>
- To: Andre dot SimoesDiasVieira at arm dot com
- Cc: binutils at sourceware dot org, Nick Clifton <nickc at redhat dot com>, Richard Earnshaw <Richard dot Earnshaw at arm dot com>, Ramana Radhakrishnan <Ramana dot Radhakrishnan at arm dot com>
- Date: Mon, 19 Nov 2018 13:40:43 +0000
- Subject: Re: [GAS][ARM] Fix testism for bl local v4t test
- References: <5BC9F15F.7020403@arm.com> <CAKnkMGtqyPdatnYy-D_k27Vrd9HWbs8827FDA0aa9k1awfsUaw@mail.gmail.com> <5BD3224D.5040300@arm.com> <5BD83E13.8080009@arm.com> <5BE5B6E4.7070806@arm.com>
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