Re: [PATCH] RISC-V: Add %got_pcrel_hi operator

[James Clarke <jrtc27 at jrtc27 dot com>]

On 8 Nov 2018, at 00:40, Jim Wilson <jimw@sifive.com> wrote:
> On 11/7/18 10:27 AM, James Clarke wrote:
>> This operator allows compilers to expand the "la" pseduo-instruction and
>> potentially split up the two instructions.
>> gas/
>> 	* config/tc-riscv.c (percent_op_utype): Add entry for a new
>> 	%got_pcrel_hi operator.
>> 	* testsuite/gas/riscv/no-relax-reloc.d: Update for new
>> 	%got_pcrel_hi operator test.
>> 	* testsuite/gas/riscv/no-relax-reloc.s: Add a %got_pcrel_hi
>> 	operator test.
>> 	* testsuite/gas/riscv/relax-reloc.d: Update for new
>> 	%got_pcrel_hi operator test.
>> 	* testsuite/gas/riscv/relax-reloc.s: Add a %got_pcrel_hi
>> 	operator test.
> 
> We have a list of assembler operators in the psABI document
>    https://github.com/riscv/riscv-elf-psabi-doc
> We also have a list of assembler operators in the assembler manual
>    https://github.com/riscv/riscv-asm-manual
> This looks a little redundant.  It isn't clear why assembler syntax is
> mentioned in the psABI.  It doesn't seem relevant to that.  Anyways, these
> should be updated.
> 
> It is probably best if there is a public proposal somewhere on the RISC-V side
> instead of unilaterally changing assembler syntax, particularly given that it
> is part of the psABI (even though it probably shouldn't be).  Someone on the
> LLVM side for instance might want to offer an opinion.  An issue filed with the
> psABI or assembler manual, or an email sent to sw-dev@riscv.org is probably
> sufficient.

Ok, makes sense to get it standardised. I'll file an issue/PR against
riscv-asm-manual as that seems the more logical place of the two repositories
to have the discussion, and reach out to the mailing list if nobody bites.

> Maybe the operator could just be %got_hi?  Not clear why we need the pcrel in
> there.  The reloc itself is just R_RISCV_GOT_HI20.

I wasn't sure which to go for, as there's some inconsistency. We have
%tls_{ie,gd}_pcrel_hi that map to R_RISCV_TLS_GOT_HI20, which would lead to
%got_pcrel_hi being the logical choice, but also %tprel_hi mapping to
R_RISCV_TPREL_HI20. However, %tprel_hi uses %tprel_lo for the second half,
whereas %tls_{ie,gd}_pcrel_hi use %pcrel_lo for the second half, so given we
also use %pcrel_lo here I decided that it made sense to match that.

> Do you have a copyright assignment for binutils?  I found ones for mach, hurd,
> and glibc.  I don't see an obvious binutils one.

No, it's something I should have done years ago. I've had small patches
accepted, but I don't know whether this is regarded as small enough. Either
way, I've just sent in a request for the binutils (and gcc while I'm at it)
forms.

James

> Otherwise this looks OK to me.
> 
> Jim