Bug 23956 - RISC-V 4-operand add doesn't check for %tprel_add
Summary: RISC-V 4-operand add doesn't check for %tprel_add
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: gas (show other bugs)
Version: 2.31
: P2 normal
Target Milestone: ---
Assignee: Jim Wilson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-06 10:53 UTC by Luís Marques
Modified: 2018-12-07 20:41 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2018-12-07 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Luís Marques 2018-12-06 10:53:06 UTC
In RISC-V assembly, there's a fictitious 4-operand ADD that's used to add a TP-relative relocation. It's usually used like "add a0,a0,tp,%tprel_add(sym)", but binutils allows a plain 0 as the 4th operand:

$ cat test.s
add a0, a0, tp, 0

$ riscv32-unknown-elf-as test.s 
$

Is this just loose error checking, or is it actually needed in some cases? There's a patch for TLS support in LLVM that requires the presence of %tprel_add in the 4-operand add. It's important, then, to clarify if the "add a0,a0,tp,0" should be accepted or not.
Comment 1 Jim Wilson 2018-12-07 00:41:04 UTC
I consider it a bug, but not serious enough that I had gotten around to trying to fix it yet.  The support for the 4-operand add is overloaded with the amo* instruction support that requires addresses with a constant 0 immediate offset.  It is an odd design.  I don't know why it was done this way.  The fact that we accept 0 for an amo* instruction is why zero gets accepted for the 4-operand add, which looked like a fairly harmless bug to me.  But looking at this closer, I see that the same overload means that we can use %tprel_add in an amo* instruction, which is a more serious problem that I think justifies a fix now.

rohan:2097$ cat tmp2.s
	amoadd.w x8,x9,%tprel_add(i)(x10)
	.globl	i
	.section	.tbss,"awT",@nobits
	.align	2
	.type	i, @object
	.size	i, 4
i:
	.zero	4
rohan:2098$ ./as-new tmp2.s
rohan:2099$ ../binutils/objdump -dr a.out

a.out:     file format elf32-littleriscv


Disassembly of section .text:

00000000 <.text>:
   0:	0095242f          	amoadd.w	s0,s1,(a0)
			0: R_RISCV_TPREL_ADD	i
			0: R_RISCV_RELAX	*ABS*
rohan:2100$
Comment 2 Sourceware Commits 2018-12-07 20:32:19 UTC
The master branch has been updated by Jim Wilson <wilson@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=f50fabe4f66534c9addacddeaa439e8d164eadda

commit f50fabe4f66534c9addacddeaa439e8d164eadda
Author: Jim Wilson <jimw@sifive.com>
Date:   Fri Dec 7 12:31:05 2018 -0800

    RISC-V: Fix 4-arg add parsing.
    
    	PR gas/23956
    	gas/
    	* config/tc-riscv.c (validate_riscv_insn) <'1'>: New case.
    	(percent_op_null): New.
    	(riscv_ip) <'j'>: Set imm_reloc before p.
    	<'1'>: New case.
    	<'0'>: Use percent_op_null and don't set imm_reloc.
    	<alu_op>: Handle *args == '1'.
    	* testsuite/gas/riscv/tprel-add.d: New.
    	* testsuite/gas/riscv/tprel-add.l: New.
    	* testsuite/gas/riscv/tprel-add.s: New.
    	opcodes/
    	* riscv-opc.c (riscv_opcodes) <"add">: Use 1 not 0 for fourth arg.
Comment 3 Jim Wilson 2018-12-07 20:41:57 UTC
Fixed.