Bug 26400 - Internal error when using .offset with jumps in RISC-V as
Summary: Internal error when using .offset with jumps in RISC-V as
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: gas (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Jim Wilson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-08-17 11:53 UTC by Rupert Swarbrick
Modified: 2020-09-24 22:25 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Rupert Swarbrick 2020-08-17 11:53:52 UTC
If you try to assemble the following code with riscv32-unknown-elf-as:

.offset 0
jal x0, 100

then you get an internal error:

> rv.asm: Assembler messages:
> rv.asm:2: Internal error in frag_new at /home/vsts/work/1/s/build/gcc/.build/riscv32-unknown-elf/src/binutils/gas/frags.c:169.
> Please report this bug.

I've reproduced this with the head of master from today (53d5a2a), but the relevant code is all pretty old, so I would expect the same behaviour from almost any version.

As far as I can understand it, the problem is that add_relaxed_insn() (from tc-riscv.c) calls frag_var(). It seems that this function finishes off the (currently empty) fragment that will contain the jump instruction and adds 8 bytes of padding for it. It then starts a new fragment, but the calculated address (calculated in frag_now_fix_octets) is still zero because we're in an absolute section and nothing got incremented.

I'm willing to believe that the assembly code is just Doing It Wrong, but the internal error isn't great.

I don't know whether the correct fix is to increment `abs_section_offset` after finishing off the fragment for the JAL instruction or whether it's just to generate a helpful error message ("Don't mix jumps and .offset", or similar).
Comment 1 Jim Wilson 2020-09-22 22:10:25 UTC
I had to read the gas docs to figure out what .offset does.

I see that frag_new calls frag_now_fix_octets, and that one has
  if (now_seg == absolute_section)
    return abs_section_offset;
which is not what the rest of the frag code is expecting.  Not obvious what that is for, or how that can work.

Though looking at tc-i386.c, in output_branch, I see it has
  if (now_seg == absolute_section)
    {
      as_bad (_("relaxable branches not supported in absolute section"));
      return;
    }
and that tracks back to a patch a couple of months ago from Jan Beulich
    https://sourceware.org/pipermail/binutils/2020-July/112420.html
This checks for the absolute section in various places, and increments the absolute section offset instead of writing bytes to frags which is compatible with what frag_now_fix_octets is doing.

It appears that significant changes are required to be able to support instructions in the absolute section.  And that the best we can do for relaxable instructions is emit a polite error.  Since we have a lot of relaxable instructions this doesn't look very useful in general for RISC-V.  Unless you turn off relaxation, but you will lose code size and performance if you do that.

The x86 port allows one to calculate the size of an insn via the absolute section, but otherwise putting things in the absolute section isn't useful.  They won't appear in the output .o file for instance.

Why are you trying to put instructions in the absolute section?  Maybe this did something useful with very old a.out or coff targets, but not for ELF?  You did say the code was pretty old.
Comment 2 Rupert Swarbrick 2020-09-23 13:04:04 UTC
> Why are you trying to put instructions in the absolute section?  Maybe this did
> something useful with very old a.out or coff targets, but not for ELF?  You did say
> the code was pretty old.

The "old code" is in the internals of binutils, not the RISC-V assembly. This assembly is actually being emitted by a random instruction generator for processor testing. When doing this, you want to be able to put hunks of code all over the place and you don't need any relocation support.

I had thought that I'd be able to use .offset lines to "jump" to each section. I'm totally willing to believe this is the wrong way to do it! Reading your reply, I think a polite "Nope, that ain't gonna work" is a perfectly reasonable behaviour for the assembler (but an internal error probably isn't).

I solved the problem by emitting lots of sections, together with a linker script that laid them out properly. If you know a clever way to avoid needing two files (foo.s and foo.ld), it might make things a little less messy and I'd be glad to hear it.
Comment 3 Jim Wilson 2020-09-24 22:05:16 UTC
A linker script is the normal GNU ld solution for putting code at specific addresses.  I see that there is an option
    --section-start=SECTIONNAME=ORG
but I have no experience using that.

Yes we need to fix this, I've got a patch to emit an error instead of an abort.  While writing it, I discovered that there is a testcase gas/elf/warn-2.s that is putting code in the absolute section, but it only needs nop to work.  It is the gas relaxable instructions that don't work, so I'm erroring only for those.
Comment 4 Sourceware Commits 2020-09-24 22:17:50 UTC
The master branch has been updated by Jim Wilson <wilson@sourceware.org>:

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

commit 743f5cfc006375655a20f255d25f6b8e1452b30b
Author: Jim Wilson <jimw@sifive.com>
Date:   Thu Sep 24 15:08:52 2020 -0700

    RISC-V: Error for relaxable branch in absolute section.
    
    Emit an error instead of crashing in frag_new, handling this same as the
    i386 port.
    
    gas/
            PR 26400
            * config/tc-riscv.c (append_insn): If in absolute section, emit
            error before add_relaxed_insn call.
            * testsuite/gas/riscv/absolute-sec.d: New.
            * testsuite/gas/riscv/absolute-sec.l: New.
            * testsuite/gas/riscv/absolute-sec.s: New.
Comment 5 Jim Wilson 2020-09-24 22:25:00 UTC
Fixed on mainline.