Bug 23305 - RISC-V illegal operands with lla and .set
Summary: RISC-V illegal operands with lla and .set
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: gas (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-06-18 12:30 UTC by Sebastian Huber
Modified: 2018-06-20 05:28 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2018-06-19 00:00:00


Attachments
Potential fix. (1.70 KB, patch)
2018-06-19 05:41 UTC, Sebastian Huber
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sebastian Huber 2018-06-18 12:30:49 UTC
The following test assembler file:

        .option nopic
        .text

        .globl sym
        .set sym, 0xabc

        .align  1
        .globl  f
        .type   f, @function
f:
        lla     a0,sym
        ret
        .size   f, .-f

Leads to:

riscv32-rtems5-as -march=rv64gc -mabi=lp64 lla.s
lla.s: Assembler messages:
lla.s:11: Error: illegal operands `lla a0,sym'

This code can be generated by GCC with:

extern char sym[];

__asm__(
  "\t.globl sym\n"
  "\t.set sym, 0xabc\n"
);

long f(void)
{
	return (long)sym;
}

riscv32-rtems5-gcc -S -o - test.c -march=rv64gc -mabi=lp64 -mcmodel=medany -O2
        .file   "test.c"
        .option nopic
        .text
 #APP
        .globl sym
        .set sym, 0xabc

 #NO_APP
        .align  1
        .globl  f
        .type   f, @function
f:
        lla     a0,sym
        ret
        .size   f, .-f
        .ident  "GCC: (GNU) 9.0.0 20180605

Using -mcmodel=medlow works since a different load sequence is generated:

        lui     a0,%hi(sym)
        addi    a0,a0,%lo(sym)
Comment 1 Sebastian Huber 2018-06-18 13:12:17 UTC
One problem is that:

opcodes/riscv-opc.c:{"lla",       "I",   "d,A",  0,    (int) M_LLA,  match_never, INSN_MACRO },

Running the test case in GDB yields:

1487          for (args = insn->args;; ++args)
(gdb) 
2120            }
(gdb) 
1489              s += strspn (s, " \t");
(gdb) 
1490              switch (*args)
(gdb) 
1929                  my_getExpression (imm_expr, s);
(gdb) 
1930                  normalize_constant_expr (imm_expr);
(gdb) 
1932                  if (imm_expr->X_op != O_symbol)
(gdb) 
2121          s = argsStart;
(gdb) 
2122          error = _("illegal operands");
(gdb) p imm_expr->X_op
$1 = O_constant
Comment 2 Jim Wilson 2018-06-18 18:42:38 UTC
lla is only valid for symbol addresses.  It isn't meant to be used for constants.  But that is an interesting testcase.  Did this come from real code?  If so then we need to fix this.

You can make medlow fail with -fpic, though I'm not sure if this makes much sense, since it isn't pic if you are giving a symbol an explicit address.

You can make both medlow and medany fail by adding __thread, but I'm not sure if this case make any sense either.  You can't give an address to a symbol and still have it be a valid TLS symbol.

This looks simple enough to fix, we can add lines like the M_LI line in opcodes/riscv-opc.c, except with opcode lla instead of li.  And likewise with la if we need to fix that also.
Comment 3 Sebastian Huber 2018-06-19 05:41:09 UTC
Created attachment 11083 [details]
Potential fix.
Comment 4 Sebastian Huber 2018-06-19 05:44:46 UTC
The C code test case works at least on arm, bfin, epiphany, lm32, m32c, m68k, mips, moxie, nios2, or1k, powerpc, sh, sparc64, sparc, v850, and x86_64.

The use case for this is that high level configuration code can pass some information to low level initialization code. During the execution of the low level initialization code the read-only or read-write global data may be not available, e.g. due to some magic address mapping mode of a processor. Addresses may be used as configuration constants in this scenario, e.g. to get the location and size of the initialization stack.
Comment 5 Jim Wilson 2018-06-19 18:14:38 UTC
The proposed patch looks OK.

Normally patches should be mailed to the main binutils mailing list, and you can mention that it was preapproved by me.
Comment 6 cvs-commit@gcc.gnu.org 2018-06-20 05:26:35 UTC
The master branch has been updated by Sebastian Huber <sh@sourceware.org>:

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

commit 160d1b3d74593bf42155da24569f54a6e7140f65
Author: Sebastian Huber <sebastian.huber@embedded-brains.de>
Date:   Tue Jun 19 07:12:48 2018 +0200

    RISC-V: Accept constant operands in la and lla
    
    opcodes/
    	PR gas/23305
    	* riscv-opc.c (riscv_opcodes): Use new format specifier 'B' for
    	la and lla.
    
    gas/
    	PR gas/23305
    	* config/tc-riscv.c (riscv_ip): Add format specifier 'B' for
    	constants and symbols.
    	* testsuite/gas/riscv/lla32.d: New file.
    	* testsuite/gas/riscv/lla32.s: Likewise.
    	* testsuite/gas/riscv/lla64-fail.d: Likewise.
    	* testsuite/gas/riscv/lla64-fail.l: Likewise.
    	* testsuite/gas/riscv/lla64-fail.s: Likewise.
    	* testsuite/gas/riscv/lla64.d: Likewise.
    	* testsuite/gas/riscv/lla64.s: Likewise.
Comment 7 Sebastian Huber 2018-06-20 05:28:33 UTC
Fixed from my point of view.