This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Wed, 07 Nov 2018 17:22:18 PST (-0800), jrtc27@jrtc27.com wrote:
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's a bit off topic, but IIRC we didn't have an assembly manual when the psABI doc was started so it ended up in there. It doesn't make a whole lot of sense, I'd be OK dropping the assembler syntax from the psABI doc.

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.

Specifically: we want to make sure the LLVM syntax stays in sync with the GCC syntax. Alex (To'd) is the person to talk to on the LLVM side, but our general practice is to propose assembler syntax additions in the spec and then point it out on sw-dev for good measure.

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.

Andrew (also To'd) would know for sure, as this predates my involvement, but I'm pretty sure these all came from MIPS and then just sort of evolved until they did something useful. At some point the relocations were all redesigned for RISC-V -- that one I was around for, and I'm pretty sure we decided not to mess with the assembler syntax which is probably why the mappings appear nonsensical.

Given that MIPS has "%got_hi", I'd also go with "%got_hi" -- at least that way we're consistently inconsistent :). I'm fine either way, though.

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

Thanks!


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]