Re: [RFC PATCH] aarch64: ld: fix relaxations for ilp32 mode

[Jiong Wang <jiong dot wang at foss dot arm dot com>]

On 17/01/17 17:04, Yury Norov wrote:
> On Tue, Jan 17, 2017 at 12:20:39PM +0000, Jiong Wang wrote:
> > On 17/01/17 08:48, Yury Norov wrote:
> > > On Mon, Jan 16, 2017 at 10:13:43AM +0000, Jiong Wang wrote:
> > > > On 14/01/17 10:48, Yury Norov wrote:
> > > > > Hi all,
> > > > >
> > > > > This patch continues the work of replacing 64-bit registers, offsets etc with
> > > > > 32-bit ones in elfNN_aarch64_tls_relax(). It doesn't fix any test I have tried,
> > > > > but it's generally correct, and I think that it should be applied.
> > > > >
> > > > > I also added new tests for ilp32 mode, and the problem is that some fail. So I'd
> > > > > like to ask for help here.
> > > > > Namely, 3 new tests are failed. tls-relax-large-desc-ie-ilp32 and
> > > > > tls-relax-large-desc-le-ilp32 fail due to error "cannot represent
> > > > > BFD_RELOC_AARCH64_TLSDESC_OFF_G1 relocation in this object file format", and
> > > > > test tls-desc-ie-ilp32 fails due to weird "undefined reference to `v2".
> > > > >
> > > > > If my understanding is correct, this error is because GAS cannot translate
> > > > > assembler commands to machine codes properly. If so, it may happen with some real
> > > > > program one day.
> > > > >
> > > > > It may also be my misunderstanding on how gas/ld work.
> > > > The "undefined reference to v2" is caused by missing of "-shared" in your linker command.
> > > Thank you, it works. My fail.
> > >
> > > > BFD_RELOC_AARCH64_TLSDESC_OFF_G1 and the several other relocations in the
> > > > error message are designed to be used by large code model only, so they are
> > > > not expected to be generated under ILP32.
> > > Hmm... Frankly, I don't understand how to proceed with that. The
> > > simplest way is to drop that couple of tests and resend the patch.
> > >
> > > But for me there's a problem here. If some asm file will contain the
> > > instruction sequence like in those tests, compilation will fail. It
> > > may come, say, from C inline asm, or whatsoever...
> > >
> > > Maybe we should wrap that relocations with "#if ARCH_SIZE == 64 ... #endif"
> > > like it has been done for BFD_RELOC_AARCH64_TLSG_MOW_G1 and
> > > BFD_RELOC_AARCH64_TLSGD_MOVW_G0_NC?
> > That looks correct to me, simply move those existed relocations into the existed ARCH_SIZE == 64 check block.
> Simple move into "ARCH_SIZE == 64" block doesn't work. At least, tests still
> produce that errors.

Sure, I mean we still need to move those large model TLS decriptor relocs, for example BFD_RELOC_AARCH64_TLSDESC_OFF_G1, to ARCH_SIZE == 64 block to parallel their TLS trad counterparts, although this is another issue and can be a seperate patch.

> Now I'm thinking to drop tls-relax-large-desc-ie-ilp32
> and tls-relax-large-desc-le-ilp32, and resend the patch, and then get deeper
> into the problem. Is the rest of the patch OK with this change?

I feel there are too many ifdef macro checks and a few duplicated code after this patch.

LP64 and ILP32 generally share the same relaxation strategy, so I would prefer to center the checks into one place for example some thing like:

#if #if ARCH_SIZE == 64
#define movz_r0 0xd2a00000
#define add_r0_r0 0x91000000
...
#else
#define movz_r0 0x52a00000
#define add_r0_r0 0x11000000
...
#endif

then in the code,

bfd_putl32 (movz_r0/add_r0_r0, ...