Discussion:
RISC-V: Relax RISCV_PCREL_* to RISCV_GPREL_*
(too old to reply)
Palmer Dabbelt
2017-10-10 21:56:11 UTC
Permalink
I've had these two patches sitting in my tree for a few months. I've been
slowly going though and testing all our out-of-tree patches and getting them
upstream, with these two as the last major ones we have. I was hoping to go
convert all our relaxations over to using R_RISCV_DELETE, with the idea being
that we could avoid the O(n^2) copying we do when relaxing, but it looks like I
won't get time to do that for a while.

Despite the patches not being quite done, they do seem to work correctly
(they're been on the branch we at SiFive make releases from for a while),
enable a new optimization, and generally don't seem to hurt -- essentially they
just add some code that should be used to clean up a few more places.

I'll commit these to master unless anyone has any objections or notices
anything I screwed up :).

[PATCH 1/2] RISC-V: Add R_RISCV_DELETE, which marks bytes for
[PATCH 2/2] RISC-V: Relax RISCV_PCREL_* to RISCV_GPREL_*
Palmer Dabbelt
2017-10-10 21:56:12 UTC
Permalink
We currently delete bytes by shifting an entire BFD backwards to
overwrite the bytes we no longer need. The result is that relaxing a
BFD is quadratic time.

This patch adds an additional relocation that specifies a byte range
that will be deleted from the final object file, and adds a relaxation
pass (between the existing passes that delete bytes and the alignment
pass) that actually deletes the bytes. Note that deletion is still
quadratic time, and nothing uses R_RISCV_DELETE yet.

I've been meaning to go convert all the other relaxations to use
R_RISCV_DELETE and then make it faster, but this patch has been sitting
around for months so it looks like that won't happen for a bit. The
PCREL->GPREL relaxation that comes next uses this, and since we've been
using these two patches out of tree since I wrote them months ago I
figure it's better to just get them in now. I (or someone else :)) can
convert all the relocations later...

R_RISCV_DELETE will never be emitted into ELF objects, so therefor isn't
exposed to the rest of binutils. As such, we're not considering this as
part of the ABI.

bfd/ChangeLog

2017-10-10 Palmer Dabbelt <***@dabbelt.com>

* elfnn-riscv (R_RISCV_DELETE): New define.
(_bfd_riscv_relax_delete): New function.
(perform_relocation): Handle R_RISCV_DELETE.
(_bfd_riscv_relax_section): Likewise.

ld/ChangeLog

2017-10-10 Palmer Dabbelt <***@dabbelt.com>

* emultempl/riscvelf.em (riscv_elf_before_allocation): Add a
third relaxation pass.
---
bfd/elfnn-riscv.c | 36 +++++++++++++++++++++++++++++++++---
ld/emultempl/riscvelf.em | 2 +-
2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
index 52c461d20a89..026b29f17e1d 100644
--- a/bfd/elfnn-riscv.c
+++ b/bfd/elfnn-riscv.c
@@ -32,6 +32,9 @@
#include "elf/riscv.h"
#include "opcode/riscv.h"

+/* Internal relocations used exclusively by the relaxation pass. */
+#define R_RISCV_DELETE (R_RISCV_max + 1)
+
#define ARCH_SIZE NN

#define MINUS_ONE ((bfd_vma)0 - 1)
@@ -1577,6 +1580,9 @@ perform_relocation (const reloc_howto_type *howto,
case R_RISCV_TLS_DTPREL64:
break;

+ case R_RISCV_DELETE:
+ return bfd_reloc_ok;
+
default:
return bfd_reloc_notsupported;
}
@@ -1899,6 +1905,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
case R_RISCV_SET16:
case R_RISCV_SET32:
case R_RISCV_32_PCREL:
+ case R_RISCV_DELETE:
/* These require no special handling beyond perform_relocation. */
break;

@@ -3041,8 +3048,28 @@ _bfd_riscv_relax_align (bfd *abfd, asection *sec,
rel->r_addend - nop_bytes);
}

-/* Relax a section. Pass 0 shortens code sequences unless disabled.
- Pass 1, which cannot be disabled, handles code alignment directives. */
+/* Relax PC-relative references to GP-relative references. */
+
+static bfd_boolean
+_bfd_riscv_relax_delete (bfd *abfd,
+ asection *sec,
+ asection *sym_sec ATTRIBUTE_UNUSED,
+ struct bfd_link_info *link_info ATTRIBUTE_UNUSED,
+ Elf_Internal_Rela *rel,
+ bfd_vma symval ATTRIBUTE_UNUSED,
+ bfd_vma max_alignment ATTRIBUTE_UNUSED,
+ bfd_vma reserve_size ATTRIBUTE_UNUSED,
+ bfd_boolean *again ATTRIBUTE_UNUSED)
+{
+ if (!riscv_relax_delete_bytes(abfd, sec, rel->r_offset, rel->r_addend))
+ return FALSE;
+ rel->r_info = ELFNN_R_INFO(0, R_RISCV_NONE);
+ return TRUE;
+}
+
+/* Relax a section. Pass 0 shortens code sequences unless disabled. Pass 1
+ deletes the bytes that pass 0 made obselete. Pass 2, which cannot be
+ disabled, handles code alignment directives. */

static bfd_boolean
_bfd_riscv_relax_section (bfd *abfd, asection *sec,
@@ -3095,6 +3122,7 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
int type = ELFNN_R_TYPE (rel->r_info);
bfd_vma symval;

+ relax_func = NULL;
if (info->relax_pass == 0)
{
if (type == R_RISCV_CALL || type == R_RISCV_CALL_PLT)
@@ -3120,7 +3148,9 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
/* Skip over the R_RISCV_RELAX. */
i++;
}
- else if (type == R_RISCV_ALIGN)
+ else if (info->relax_pass == 1 && type == R_RISCV_DELETE)
+ relax_func = _bfd_riscv_relax_delete;
+ else if (info->relax_pass == 2 && type == R_RISCV_ALIGN)
relax_func = _bfd_riscv_relax_align;
else
continue;
diff --git a/ld/emultempl/riscvelf.em b/ld/emultempl/riscvelf.em
index 99af4d864261..7b30f528628a 100644
--- a/ld/emultempl/riscvelf.em
+++ b/ld/emultempl/riscvelf.em
@@ -39,7 +39,7 @@ riscv_elf_before_allocation (void)
else
ENABLE_RELAXATION;

- link_info.relax_pass = 2;
+ link_info.relax_pass = 3;
}

static void
--
2.13.6
Palmer Dabbelt
2017-10-10 21:56:13 UTC
Permalink
In the medany code model the compiler generates PCREL_HI20+PCREL_LO12
relocation pairs against local symbols because HI20+LO12 relocations
can't reach high addresses. We relax HI20+LO12 pairs to GPREL
relocations when possible, which is an important optimization for
Dhrystone. Without this commit we are unable to relax
PCREL_HI20+PCREL_LO12 pairs to GPREL when possible, causing a 10%
permormance hit on Dhrystone on Rocket.

Note that we'll now relax

la gp, __global_pointer$

to

mv gp, gp

which probably isn't what you want in your entry code. Users who want
gp-relative symbols to continue to resolve should add ".option norelax"
accordingly. Due to this, the assembler now pairs PCREL relocations
with RELAX relocations when they're expected to be relaxed just like
every other relaxable relocation.

bfd/ChangeLog

2017-10-10 Palmer Dabbelt <***@dabbelt.com>

* elfnn-riscv.c (riscv_pcgp_hi_reloc): New structure.
(riscv_pcgp_lo_reloc): Likewise.
(riscv_pcgp_relocs): Likewise.
(riscv_init_pcgp_relocs): New function.
(riscv_free_pcgp_relocs): Likewise.
(riscv_record_pcgp_hi_reloc): Likewise.
(riscv_record_pcgp_lo_reloc): Likewise.
(riscv_delete_pcgp_hi_reloc): Likewise.
(riscv_use_pcgp_hi_reloc): Likewise.
(riscv_record_pcgp_lo_reloc): Likewise.
(riscv_find_pcgp_lo_reloc): Likewise.
(riscv_delete_pcgp_lo_reloc): Likewise.
(_bfd_riscv_relax_pc): Likewise.
(_bfd_riscv_relax_section): Handle R_RISCV_PCREL_* relocations
via the new functions above.

gas/ChangeLog

2017-10-10 Palmer Dabbelt <***@dabbelt.com>

* config/tc-riscv.c (md_apply_fix): Mark
BFD_RELOC_RISCV_PCREL_HI20 as relaxable when relaxations are
enabled.
---
bfd/elfnn-riscv.c | 286 ++++++++++++++++++++++++++++++++++++++++++++++++--
gas/config/tc-riscv.c | 5 +-
2 files changed, 283 insertions(+), 8 deletions(-)

diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
index 026b29f17e1d..b038ec205c31 100644
--- a/bfd/elfnn-riscv.c
+++ b/bfd/elfnn-riscv.c
@@ -2778,10 +2778,155 @@ riscv_relax_delete_bytes (bfd *abfd, asection *sec, bfd_vma addr, size_t count)
return TRUE;
}

+/* A second format for recording PC-relative hi relocations. This stores the
+ information required to relax them to GP-relative addresses. */
+
+typedef struct riscv_pcgp_hi_reloc riscv_pcgp_hi_reloc;
+struct riscv_pcgp_hi_reloc
+{
+ bfd_vma hi_sec_off;
+ bfd_vma hi_addend;
+ bfd_vma hi_addr;
+ unsigned hi_sym;
+ asection *sym_sec;
+ riscv_pcgp_hi_reloc *next;
+};
+
+typedef struct riscv_pcgp_lo_reloc riscv_pcgp_lo_reloc;
+struct riscv_pcgp_lo_reloc
+{
+ bfd_vma hi_sec_off;
+ riscv_pcgp_lo_reloc *next;
+};
+
+typedef struct
+{
+ riscv_pcgp_hi_reloc *hi;
+ riscv_pcgp_lo_reloc *lo;
+} riscv_pcgp_relocs;
+
+static bfd_boolean
+riscv_init_pcgp_relocs (riscv_pcgp_relocs *p)
+{
+ p->hi = NULL;
+ p->lo = NULL;
+ return TRUE;
+}
+
+static void
+riscv_free_pcgp_relocs (riscv_pcgp_relocs *p,
+ bfd *abfd ATTRIBUTE_UNUSED,
+ asection *sec ATTRIBUTE_UNUSED)
+{
+ riscv_pcgp_hi_reloc *c;
+ riscv_pcgp_lo_reloc *l;
+
+ for (c = p->hi; c != NULL;)
+ {
+ riscv_pcgp_hi_reloc *next = c->next;
+ free (c);
+ c = next;
+ }
+
+ for (l = p->lo; l != NULL;)
+ {
+ riscv_pcgp_lo_reloc *next = l->next;
+ free (l);
+ l = next;
+ }
+}
+
+static bfd_boolean
+riscv_record_pcgp_hi_reloc (riscv_pcgp_relocs *p, bfd_vma hi_sec_off,
+ bfd_vma hi_addend, bfd_vma hi_addr,
+ unsigned hi_sym, asection *sym_sec)
+{
+ riscv_pcgp_hi_reloc *new = bfd_malloc (sizeof(*new));
+ if (!new)
+ return FALSE;
+ new->hi_sec_off = hi_sec_off;
+ new->hi_addend = hi_addend;
+ new->hi_addr = hi_addr;
+ new->hi_sym = hi_sym;
+ new->sym_sec = sym_sec;
+ new->next = p->hi;
+ p->hi = new;
+ return TRUE;
+}
+
+static riscv_pcgp_hi_reloc *
+riscv_find_pcgp_hi_reloc(riscv_pcgp_relocs *p, bfd_vma hi_sec_off)
+{
+ riscv_pcgp_hi_reloc *c;
+
+ for (c = p->hi; c != NULL; c = c->next)
+ if (c->hi_sec_off == hi_sec_off)
+ return c;
+ return NULL;
+}
+
+static bfd_boolean
+riscv_delete_pcgp_hi_reloc(riscv_pcgp_relocs *p, bfd_vma hi_sec_off)
+{
+ bfd_boolean out = FALSE;
+ riscv_pcgp_hi_reloc *c;
+
+ for (c = p->hi; c != NULL; c = c->next)
+ if (c->hi_sec_off == hi_sec_off)
+ out = TRUE;
+
+ return out;
+}
+
+static bfd_boolean
+riscv_use_pcgp_hi_reloc(riscv_pcgp_relocs *p, bfd_vma hi_sec_off)
+{
+ bfd_boolean out = FALSE;
+ riscv_pcgp_hi_reloc *c;
+
+ for (c = p->hi; c != NULL; c = c->next)
+ if (c->hi_sec_off == hi_sec_off)
+ out = TRUE;
+
+ return out;
+}
+
+static bfd_boolean
+riscv_record_pcgp_lo_reloc (riscv_pcgp_relocs *p, bfd_vma hi_sec_off)
+{
+ riscv_pcgp_lo_reloc *new = bfd_malloc (sizeof(*new));
+ if (!new)
+ return FALSE;
+ new->hi_sec_off = hi_sec_off;
+ new->next = p->lo;
+ p->lo = new;
+ return TRUE;
+}
+
+static bfd_boolean
+riscv_find_pcgp_lo_reloc (riscv_pcgp_relocs *p, bfd_vma hi_sec_off)
+{
+ riscv_pcgp_lo_reloc *c;
+
+ for (c = p->lo; c != NULL; c = c->next)
+ if (c->hi_sec_off == hi_sec_off)
+ return TRUE;
+ return FALSE;
+}
+
+static bfd_boolean
+riscv_delete_pcgp_lo_reloc (riscv_pcgp_relocs *p ATTRIBUTE_UNUSED,
+ bfd_vma lo_sec_off ATTRIBUTE_UNUSED,
+ size_t bytes ATTRIBUTE_UNUSED)
+{
+ return TRUE;
+}
+
typedef bfd_boolean (*relax_func_t) (bfd *, asection *, asection *,
struct bfd_link_info *,
Elf_Internal_Rela *,
- bfd_vma, bfd_vma, bfd_vma, bfd_boolean *);
+ bfd_vma, bfd_vma, bfd_vma, bfd_boolean *,
+ riscv_pcgp_relocs *);

/* Relax AUIPC + JALR into JAL. */

@@ -2792,7 +2937,8 @@ _bfd_riscv_relax_call (bfd *abfd, asection *sec, asection *sym_sec,
bfd_vma symval,
bfd_vma max_alignment,
bfd_vma reserve_size ATTRIBUTE_UNUSED,
- bfd_boolean *again)
+ bfd_boolean *again,
+ riscv_pcgp_relocs *pcgp_relocs ATTRIBUTE_UNUSED)
{
bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;
bfd_signed_vma foff = symval - (sec_addr (sec) + rel->r_offset);
@@ -2875,7 +3021,8 @@ _bfd_riscv_relax_lui (bfd *abfd,
bfd_vma symval,
bfd_vma max_alignment,
bfd_vma reserve_size,
- bfd_boolean *again)
+ bfd_boolean *again,
+ riscv_pcgp_relocs *pcgp_relocs ATTRIBUTE_UNUSED)
{
bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;
bfd_vma gp = riscv_global_pointer_value (link_info);
@@ -2964,7 +3111,8 @@ _bfd_riscv_relax_tls_le (bfd *abfd,
bfd_vma symval,
bfd_vma max_alignment ATTRIBUTE_UNUSED,
bfd_vma reserve_size ATTRIBUTE_UNUSED,
- bfd_boolean *again)
+ bfd_boolean *again,
+ riscv_pcgp_relocs *prcel_relocs ATTRIBUTE_UNUSED)
{
/* See if this symbol is in range of tp. */
if (RISCV_CONST_HIGH_PART (tpoff (link_info, symval)) != 0)
@@ -3003,7 +3151,8 @@ _bfd_riscv_relax_align (bfd *abfd, asection *sec,
bfd_vma symval,
bfd_vma max_alignment ATTRIBUTE_UNUSED,
bfd_vma reserve_size ATTRIBUTE_UNUSED,
- bfd_boolean *again ATTRIBUTE_UNUSED)
+ bfd_boolean *again ATTRIBUTE_UNUSED,
+ riscv_pcgp_relocs *pcrel_relocs ATTRIBUTE_UNUSED)
{
bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;
bfd_vma alignment = 1, pos;
@@ -3051,6 +3200,118 @@ _bfd_riscv_relax_align (bfd *abfd, asection *sec,
/* Relax PC-relative references to GP-relative references. */

static bfd_boolean
+_bfd_riscv_relax_pc (bfd *abfd,
+ asection *sec,
+ asection *sym_sec,
+ struct bfd_link_info *link_info,
+ Elf_Internal_Rela *rel,
+ bfd_vma symval,
+ bfd_vma max_alignment,
+ bfd_vma reserve_size,
+ bfd_boolean *again ATTRIBUTE_UNUSED,
+ riscv_pcgp_relocs *pcgp_relocs)
+{
+ bfd_vma gp = riscv_global_pointer_value (link_info);
+
+ BFD_ASSERT (rel->r_offset + 4 <= sec->size);
+
+ /* Chain the _LO relocs to their cooresponding _HI reloc to compute the
+ * actual target address. */
+ riscv_pcgp_hi_reloc hi_reloc = {0};
+ switch (ELFNN_R_TYPE (rel->r_info))
+ {
+ case R_RISCV_PCREL_LO12_I:
+ case R_RISCV_PCREL_LO12_S:
+ {
+ riscv_pcgp_hi_reloc *hi = riscv_find_pcgp_hi_reloc (pcgp_relocs,
+ symval - sec_addr(sym_sec));
+ if (hi == NULL)
+ {
+ riscv_record_pcgp_lo_reloc (pcgp_relocs, symval - sec_addr(sym_sec));
+ return TRUE;
+ }
+
+ hi_reloc = *hi;
+ symval = hi_reloc.hi_addr;
+ sym_sec = hi_reloc.sym_sec;
+ if (!riscv_use_pcgp_hi_reloc(pcgp_relocs, hi->hi_sec_off))
+ (*_bfd_error_handler)
+ (_("%B(%A+0x%lx): Unable to clear RISCV_PCREL_HI20 reloc"
+ "for cooresponding RISCV_PCREL_LO12 reloc"),
+ abfd, sec, rel->r_offset);
+ }
+ break;
+
+ case R_RISCV_PCREL_HI20:
+ /* Mergeable symbols and code might later move out of range. */
+ if (sym_sec->flags & (SEC_MERGE | SEC_CODE))
+ return TRUE;
+
+ /* If the cooresponding lo relocation has already been seen then it's not
+ * safe to relax this relocation. */
+ if (riscv_find_pcgp_lo_reloc (pcgp_relocs, rel->r_offset))
+ return TRUE;
+
+ break;
+
+ default:
+ abort ();
+ }
+
+ if (gp)
+ {
+ /* If gp and the symbol are in the same output section, then
+ consider only that section's alignment. */
+ struct bfd_link_hash_entry *h =
+ bfd_link_hash_lookup (link_info->hash, RISCV_GP_SYMBOL, FALSE, FALSE, TRUE);
+ if (h->u.def.section->output_section == sym_sec->output_section)
+ max_alignment = (bfd_vma) 1 << sym_sec->output_section->alignment_power;
+ }
+
+ /* Is the reference in range of x0 or gp?
+ Valid gp range conservatively because of alignment issue. */
+ if (VALID_ITYPE_IMM (symval)
+ || (symval >= gp
+ && VALID_ITYPE_IMM (symval - gp + max_alignment + reserve_size))
+ || (symval < gp
+ && VALID_ITYPE_IMM (symval - gp - max_alignment - reserve_size)))
+ {
+ unsigned sym = hi_reloc.hi_sym;
+ switch (ELFNN_R_TYPE (rel->r_info))
+ {
+ case R_RISCV_PCREL_LO12_I:
+ rel->r_info = ELFNN_R_INFO (sym, R_RISCV_GPREL_I);
+ rel->r_addend += hi_reloc.hi_addend;
+ return riscv_delete_pcgp_lo_reloc (pcgp_relocs, rel->r_offset, 4);
+
+ case R_RISCV_PCREL_LO12_S:
+ rel->r_info = ELFNN_R_INFO (sym, R_RISCV_GPREL_S);
+ rel->r_addend += hi_reloc.hi_addend;
+ return riscv_delete_pcgp_lo_reloc (pcgp_relocs, rel->r_offset, 4);
+
+ case R_RISCV_PCREL_HI20:
+ riscv_record_pcgp_hi_reloc (pcgp_relocs,
+ rel->r_offset,
+ rel->r_addend,
+ symval,
+ ELFNN_R_SYM(rel->r_info),
+ sym_sec);
+ /* We can delete the unnecessary AUIPC and reloc. */
+ rel->r_info = ELFNN_R_INFO (0, R_RISCV_DELETE);
+ rel->r_addend = 4;
+ return riscv_delete_pcgp_hi_reloc (pcgp_relocs, rel->r_offset);
+
+ default:
+ abort ();
+ }
+ }
+
+ return TRUE;
+}
+
+/* Relax PC-relative references to GP-relative references. */
+
+static bfd_boolean
_bfd_riscv_relax_delete (bfd *abfd,
asection *sec,
asection *sym_sec ATTRIBUTE_UNUSED,
@@ -3059,7 +3320,8 @@ _bfd_riscv_relax_delete (bfd *abfd,
bfd_vma symval ATTRIBUTE_UNUSED,
bfd_vma max_alignment ATTRIBUTE_UNUSED,
bfd_vma reserve_size ATTRIBUTE_UNUSED,
- bfd_boolean *again ATTRIBUTE_UNUSED)
+ bfd_boolean *again ATTRIBUTE_UNUSED,
+ riscv_pcgp_relocs *pcgp_relocs ATTRIBUTE_UNUSED)
{
if (!riscv_relax_delete_bytes(abfd, sec, rel->r_offset, rel->r_addend))
return FALSE;
@@ -3083,6 +3345,7 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
bfd_boolean ret = FALSE;
unsigned int i;
bfd_vma max_alignment, reserve_size = 0;
+ riscv_pcgp_relocs pcgp_relocs;

*again = FALSE;

@@ -3094,6 +3357,8 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
&& info->relax_pass == 0))
return TRUE;

+ riscv_init_pcgp_relocs (&pcgp_relocs);
+
/* Read this BFD's relocs if we haven't done so already. */
if (data->relocs)
relocs = data->relocs;
@@ -3131,6 +3396,11 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
|| type == R_RISCV_LO12_I
|| type == R_RISCV_LO12_S)
relax_func = _bfd_riscv_relax_lui;
+ else if (!bfd_link_pic(info)
+ && (type == R_RISCV_PCREL_HI20
+ || type == R_RISCV_PCREL_LO12_I
+ || type == R_RISCV_PCREL_LO12_S))
+ relax_func = _bfd_riscv_relax_pc;
else if (type == R_RISCV_TPREL_HI20
|| type == R_RISCV_TPREL_ADD
|| type == R_RISCV_TPREL_LO12_I
@@ -3221,7 +3491,8 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
symval += rel->r_addend;

if (!relax_func (abfd, sec, sym_sec, info, rel, symval,
- max_alignment, reserve_size, again))
+ max_alignment, reserve_size, again,
+ &pcgp_relocs))
goto fail;
}

@@ -3230,6 +3501,7 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
fail:
if (relocs != data->relocs)
free (relocs);
+ riscv_free_pcgp_relocs(&pcgp_relocs, abfd, sec);

return ret;
}
diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index 1a15efc47985..189e40d11c7f 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -1885,7 +1885,6 @@ md_apply_fix (fixS *fixP, valueT *valP, segT seg ATTRIBUTE_UNUSED)
break;

case BFD_RELOC_RISCV_GOT_HI20:
- case BFD_RELOC_RISCV_PCREL_HI20:
case BFD_RELOC_RISCV_ADD8:
case BFD_RELOC_RISCV_ADD16:
case BFD_RELOC_RISCV_ADD32:
@@ -2078,8 +2077,12 @@ md_apply_fix (fixS *fixP, valueT *valP, segT seg ATTRIBUTE_UNUSED)
relaxable = TRUE;
break;

+ case BFD_RELOC_RISCV_PCREL_HI20:
case BFD_RELOC_RISCV_PCREL_LO12_S:
case BFD_RELOC_RISCV_PCREL_LO12_I:
+ relaxable = riscv_opts.relax;
+ break;
+
case BFD_RELOC_RISCV_ALIGN:
break;
--
2.13.6
H.J. Lu
2017-10-10 22:23:33 UTC
Permalink
Post by Palmer Dabbelt
I've had these two patches sitting in my tree for a few months. I've been
slowly going though and testing all our out-of-tree patches and getting them
upstream, with these two as the last major ones we have. I was hoping to go
convert all our relaxations over to using R_RISCV_DELETE, with the idea being
that we could avoid the O(n^2) copying we do when relaxing, but it looks like I
won't get time to do that for a while.
Despite the patches not being quite done, they do seem to work correctly
(they're been on the branch we at SiFive make releases from for a while),
enable a new optimization, and generally don't seem to hurt -- essentially they
just add some code that should be used to clean up a few more places.
I'll commit these to master unless anyone has any objections or notices
anything I screwed up :).
[PATCH 1/2] RISC-V: Add R_RISCV_DELETE, which marks bytes for
[PATCH 2/2] RISC-V: Relax RISCV_PCREL_* to RISCV_GPREL_*
For riscv32-linux target, I see:

FAIL: difference of two undefined symbols
FAIL: align
FAIL: .sleb128 tests (2)
FAIL: .sleb128 tests (4)
FAIL: .sleb128 tests (5)
FAIL: .sleb128 tests (7)
FAIL: relax .uleb128
FAIL: Disabling section padding
FAIL: DWARF2 10
FAIL: lns-duplicate
FAIL: lns-common-1
FAIL: ld-elf/compressed1d
FAIL: ld-elf/stab
FAIL: ld-scripts/fill
FAIL: ld-scripts/empty-address-2a
FAIL: ld-scripts/empty-address-2b
FAIL: PHDRS2
FAIL: ld-scripts/size-1

Please fix them.

Thanks.
--
H.J.
Palmer Dabbelt
2017-10-17 17:51:36 UTC
Permalink
Post by H.J. Lu
Post by Palmer Dabbelt
I've had these two patches sitting in my tree for a few months. I've been
slowly going though and testing all our out-of-tree patches and getting them
upstream, with these two as the last major ones we have. I was hoping to go
convert all our relaxations over to using R_RISCV_DELETE, with the idea being
that we could avoid the O(n^2) copying we do when relaxing, but it looks like I
won't get time to do that for a while.
Despite the patches not being quite done, they do seem to work correctly
(they're been on the branch we at SiFive make releases from for a while),
enable a new optimization, and generally don't seem to hurt -- essentially they
just add some code that should be used to clean up a few more places.
I'll commit these to master unless anyone has any objections or notices
anything I screwed up :).
[PATCH 1/2] RISC-V: Add R_RISCV_DELETE, which marks bytes for
[PATCH 2/2] RISC-V: Relax RISCV_PCREL_* to RISCV_GPREL_*
FAIL: difference of two undefined symbols
FAIL: align
FAIL: .sleb128 tests (2)
FAIL: .sleb128 tests (4)
FAIL: .sleb128 tests (5)
FAIL: .sleb128 tests (7)
FAIL: relax .uleb128
FAIL: Disabling section padding
FAIL: DWARF2 10
FAIL: lns-duplicate
FAIL: lns-common-1
FAIL: ld-elf/compressed1d
FAIL: ld-elf/stab
FAIL: ld-scripts/fill
FAIL: ld-scripts/empty-address-2a
FAIL: ld-scripts/empty-address-2b
FAIL: PHDRS2
FAIL: ld-scripts/size-1
Please fix them.
Thanks.
Oh, sorry about that -- it turns out we actually haven't been running the
binutils test suite (we just run the GCC test suite). It's something we really
should be doing, it's just hard to find the time to actually get through these
things with all the other issues we keep finding :). I've managed to get the
GAS test suite clean with these two tests, though the readelf one might be
no good. Are those two OK for master?

I've added the binutils test suites to our toolchain regression test suite so
we shouldn't be adding any new test failures on any commits from now on. I'll
keep pushing on the other test failures, but it might take a bit to get through
all of them.

Given that this doesn't add any new test failures, is it OK for master?

Thanks for pointing this out!

[PATCH 1/2] RISC-V: Fix readelf's R_RISCV_{ADD,SUB}{8,16,32,64}
[PATCH 2/2] RISC-V: Mark unsupported gas testcases
Palmer Dabbelt
2017-10-17 17:51:37 UTC
Permalink
I'm not entirely sure this is correct, but it does pass the test cases
(as well as a handful of small modifications I made to them). The
implementation is patterned off the existing ones, with the additional
wrinkle that we have more relocations than the other ports do.

It feels a bit odd to handle the relocations this way, as I'd expect
this to either be handled by BFD or at least to look up the relocations
via address (as opposed to just assuming tehy're in a particular order),
but I can't figure out a better way to do this.

binutils/ChangeLog

2017-10-16 Palmer Dabbelt <***@dabbelt.com>

* readelf.c (target_specific_reloc_handling) <EM_RISCV>: Handle
R_RISCV_{ADD,SUB}{8,16,32,64} relocations.

gas/ChangeLog

2017-10-16 Palmer Dabbelt <***@dabbelt.com>

* testsuite/gas/lns/lns.exp: Use lns-common-1-alt instead of
lns-common-1 for RISC-V targets.
---
binutils/readelf.c | 74 +++++++++++++++++++++++++++++++++++++++++++
gas/testsuite/gas/lns/lns.exp | 1 +
2 files changed, 75 insertions(+)

diff --git a/binutils/readelf.c b/binutils/readelf.c
index b2f75c0048bf..cfea265369a2 100644
--- a/binutils/readelf.c
+++ b/binutils/readelf.c
@@ -11993,6 +11993,80 @@ target_specific_reloc_handling (Elf_Internal_Rela * reloc,
}
break;
}
+
+ case EM_RISCV:
+ {
+ static bfd_vma saved_sym1 = 0;
+ static bfd_vma saved_sym2 = 0;
+ bfd_vma size_bytes, multiplier, value;
+
+ switch (reloc_type)
+ {
+ case 1: /* R_RISCV_32 */
+ size_bytes = 4;
+ multiplier = 0;
+ goto handle_abs_reloc;
+ case 2: /* R_RISCV_64 */
+ size_bytes = 8;
+ multiplier = 0;
+ goto handle_abs_reloc;
+
+ case 33: /* R_RISCV_ADD8 */
+ size_bytes = 1;
+ multiplier = 1;
+ goto handle_add_sub_reloc;
+ case 34: /* R_RISCV_ADD16 */
+ size_bytes = 2;
+ multiplier = 1;
+ goto handle_add_sub_reloc;
+ case 35: /* R_RISCV_ADD32 */
+ size_bytes = 4;
+ multiplier = 1;
+ goto handle_add_sub_reloc;
+ case 36: /* R_RISCV_ADD64 */
+ size_bytes = 8;
+ multiplier = 1;
+ goto handle_add_sub_reloc;
+ case 37: /* R_RISCV_SUB8 */
+ size_bytes = 1;
+ multiplier = -1;
+ goto handle_add_sub_reloc;
+ case 38: /* R_RISCV_SUB16 */
+ size_bytes = 2;
+ multiplier = -1;
+ goto handle_add_sub_reloc;
+ case 39: /* R_RISCV_SUB32 */
+ size_bytes = 4;
+ multiplier = -1;
+ goto handle_add_sub_reloc;
+ case 40: /* R_RISCV_SUB64 */
+ size_bytes = 8;
+ multiplier = -1;
+ goto handle_add_sub_reloc;
+
+ default:
+ return FALSE;
+ }
+
+ handle_abs_reloc:
+ /* This symbol is an absolute address, so just push it onto the
+ * stack. */
+ saved_sym2 = saved_sym1;
+ saved_sym1 = symtab[sym_index].st_value + reloc->r_addend;
+ return FALSE;
+
+ handle_add_sub_reloc:
+ /* This symbol is a sum or difference, so compute the original value
+ * of the relocation by looking at the previous two symbol values. */
+ saved_sym2 = saved_sym1;
+ saved_sym1 = symtab[sym_index].st_value + reloc->r_addend;
+
+ value = (multiplier * saved_sym1) + (-1 * multiplier * saved_sym2);
+ if (IN_RANGE (start, end, start + reloc->r_offset, size_bytes)) {
+ byte_put(start + reloc->r_offset, value, size_bytes);
+ }
+ return TRUE;
+ }
}

return FALSE;
diff --git a/gas/testsuite/gas/lns/lns.exp b/gas/testsuite/gas/lns/lns.exp
index 281b621f7243..02312c3e2c7e 100644
--- a/gas/testsuite/gas/lns/lns.exp
+++ b/gas/testsuite/gas/lns/lns.exp
@@ -38,6 +38,7 @@ if {
|| [istarget msp430-*-*]
|| [istarget nds32*-*-*]
|| [istarget pru-*-*]
+ || [istarget riscv*-*-*]
|| [istarget rl78-*-*]
|| [istarget xtensa*-*-*] } {
run_dump_test "lns-common-1-alt"
--
2.13.6
Alan Modra
2017-10-17 22:51:33 UTC
Permalink
Post by Palmer Dabbelt
I'm not entirely sure this is correct, but it does pass the test cases
(as well as a handful of small modifications I made to them). The
implementation is patterned off the existing ones, with the additional
wrinkle that we have more relocations than the other ports do.
It feels a bit odd to handle the relocations this way, as I'd expect
this to either be handled by BFD or at least to look up the relocations
readelf is supposed to be independent of BFD.
Post by Palmer Dabbelt
+ static bfd_vma saved_sym1 = 0;
+ static bfd_vma saved_sym2 = 0;
Ick, no. Use the section contents instead, just as you do in your
BFD code.
--
Alan Modra
Australia Development Lab, IBM
Palmer Dabbelt
2017-10-17 17:51:38 UTC
Permalink
There are individual comments that explain why each test isn't
supported, but the vast majority of them are due to RISC-V's aggressive
linker relaxation. The SLEB test cases should eventually be supported,
but the remaining ones probably won't ever be.

2017-10-16 Palmer Dabbelt <***@dabbelt.com>

* testsuite/gas/all/align.d: Mark as unsupported on RISC-V.
testsuite/gas/all/relax.d: Likewise.
testsuite/gas/all/sleb128-2.d: Likewise.
testsuite/gas/all/sleb128-4.d: Likewise.
testsuite/gas/all/sleb128-5.d: Likewise.
testsuite/gas/all/sleb128-7.d: Likewise.
testsuite/gas/elf/section11.d: Likewise.
testsuite/gas/all/gas.exp (diff1.s): Likewise.
---
gas/testsuite/gas/all/align.d | 4 +++-
gas/testsuite/gas/all/gas.exp | 1 +
gas/testsuite/gas/all/relax.d | 4 ++++
gas/testsuite/gas/all/sleb128-2.d | 4 ++++
gas/testsuite/gas/all/sleb128-4.d | 5 ++++-
gas/testsuite/gas/all/sleb128-5.d | 4 ++++
gas/testsuite/gas/all/sleb128-7.d | 4 ++++
gas/testsuite/gas/elf/section11.d | 4 +++-
8 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/gas/testsuite/gas/all/align.d b/gas/testsuite/gas/all/align.d
index dec2168aa6f0..eb08d8ddcda7 100644
--- a/gas/testsuite/gas/all/align.d
+++ b/gas/testsuite/gas/all/align.d
@@ -2,7 +2,9 @@
#name: align
# The RX port will always replace zeros in any aligned area with NOPs,
# even if the user requested that they filled with zeros.
-#not-target: m32c-* rx-*
+# RISC-V handles alignment via relaxation and therefor won't have object files
+# with the expected alignment.
+#not-target: m32c-* riscv*-* rx-*

# Test the alignment pseudo-op.

diff --git a/gas/testsuite/gas/all/gas.exp b/gas/testsuite/gas/all/gas.exp
index b5b0bebbf35a..38df380991b3 100644
--- a/gas/testsuite/gas/all/gas.exp
+++ b/gas/testsuite/gas/all/gas.exp
@@ -63,6 +63,7 @@ if { ![istarget alpha*-*-*vms*]
&& ![istarget microblaze-*-*]
&& ![istarget mn10300-*-*]
&& ![istarget msp430*-*-*]
+ && ![istarget riscv*-*-*]
&& ![istarget rl78-*-*]
&& ![istarget rx-*-*] } then {
gas_test_error "diff1.s" "" "difference of two undefined symbols"
diff --git a/gas/testsuite/gas/all/relax.d b/gas/testsuite/gas/all/relax.d
index 1e581c2695b0..08a9be178cc6 100644
--- a/gas/testsuite/gas/all/relax.d
+++ b/gas/testsuite/gas/all/relax.d
@@ -1,5 +1,9 @@
#objdump : -s -j .data -j "\$DATA\$"
#name : relax .uleb128
+# RISC-V doesn't support .sleb operands that are the difference of two symbols
+# because symbol values are not known until after linker relaxation has been
+# performed.
+#skip : riscv*-*-*

.*: .*

diff --git a/gas/testsuite/gas/all/sleb128-2.d b/gas/testsuite/gas/all/sleb128-2.d
index cce0b4c0334a..afffbed2a673 100644
--- a/gas/testsuite/gas/all/sleb128-2.d
+++ b/gas/testsuite/gas/all/sleb128-2.d
@@ -1,5 +1,9 @@
#objdump : -s -j .data -j "\$DATA\$"
#name : .sleb128 tests (2)
+# RISC-V doesn't support .sleb operands that are the difference of two symbols
+# because symbol values are not known until after linker relaxation has been
+# performed.
+#skip : riscv*-*-*

.*: .*

diff --git a/gas/testsuite/gas/all/sleb128-4.d b/gas/testsuite/gas/all/sleb128-4.d
index 89b956530186..7a881cc4c77e 100644
--- a/gas/testsuite/gas/all/sleb128-4.d
+++ b/gas/testsuite/gas/all/sleb128-4.d
@@ -1,6 +1,9 @@
#objdump : -s -j .data -j "\$DATA\$"
#name : .sleb128 tests (4)
-#skip: msp430*-*-*
+# RISC-V doesn't support .sleb operands that are the difference of two symbols
+# because symbol values are not known until after linker relaxation has been
+# performed.
+#skip: msp430*-*-* riscv*-*-*

.*: .*

diff --git a/gas/testsuite/gas/all/sleb128-5.d b/gas/testsuite/gas/all/sleb128-5.d
index 0accfb5a6e88..1ed21344ce16 100644
--- a/gas/testsuite/gas/all/sleb128-5.d
+++ b/gas/testsuite/gas/all/sleb128-5.d
@@ -1,5 +1,9 @@
#objdump : -s -j .data -j "\$DATA\$"
#name : .sleb128 tests (5)
+# RISC-V doesn't support .sleb operands that are the difference of two symbols
+# because symbol values are not known until after linker relaxation has been
+# performed.
+#skip : riscv*-*-*

.*: .*

diff --git a/gas/testsuite/gas/all/sleb128-7.d b/gas/testsuite/gas/all/sleb128-7.d
index 6fcbdefe6e93..62c545843f20 100644
--- a/gas/testsuite/gas/all/sleb128-7.d
+++ b/gas/testsuite/gas/all/sleb128-7.d
@@ -1,5 +1,9 @@
#objdump : -s -j .data -j "\$DATA\$"
#name : .sleb128 tests (7)
+# RISC-V doesn't support .sleb operands that are the difference of two symbols
+# because symbol values are not known until after linker relaxation has been
+# performed.
+#skip: riscv*-*-*

.*: .*

diff --git a/gas/testsuite/gas/elf/section11.d b/gas/testsuite/gas/elf/section11.d
index c1043db7a0b5..878a483fe830 100644
--- a/gas/testsuite/gas/elf/section11.d
+++ b/gas/testsuite/gas/elf/section11.d
@@ -2,7 +2,9 @@
#readelf: -S --wide
#name: Disabling section padding
# The RX port uses non standard section names.
-#skip: rx-*-*
+# RISC-V handles alignment via linker relaxation, so object files don't have
+# the expected alignment.
+#skip: rx-*-* riscv*-*-*

#...
\[ .\] .text[ ]+PROGBITS[ ]+0+00 0+[0-9a-f]+ 0+0(1|4|5) 00 AX 0 0 16
--
2.13.6
Alan Modra
2017-10-17 23:05:41 UTC
Permalink
Post by Palmer Dabbelt
--- a/gas/testsuite/gas/all/relax.d
+++ b/gas/testsuite/gas/all/relax.d
@@ -1,5 +1,9 @@
#objdump : -s -j .data -j "\$DATA\$"
#name : relax .uleb128
+# RISC-V doesn't support .sleb operands that are the difference of two symbols
+# because symbol values are not known until after linker relaxation has been
+# performed.
+#skip : riscv*-*-*
.*: .*
"skip" is the wrong option. See gas-defs.exp:run_dump_test. It
should be "not-target" here and elsewhere.
--
Alan Modra
Australia Development Lab, IBM
Palmer Dabbelt
2017-10-18 20:24:06 UTC
Permalink
Post by Alan Modra
Post by Palmer Dabbelt
--- a/gas/testsuite/gas/all/relax.d
+++ b/gas/testsuite/gas/all/relax.d
@@ -1,5 +1,9 @@
#objdump : -s -j .data -j "\$DATA\$"
#name : relax .uleb128
+# RISC-V doesn't support .sleb operands that are the difference of two symbols
+# because symbol values are not known until after linker relaxation has been
+# performed.
+#skip : riscv*-*-*
.*: .*
"skip" is the wrong option. See gas-defs.exp:run_dump_test. It
should be "not-target" here and elsewhere.
Thanks, I didn't know the difference. I went ahead and commited this patch,
I'll fix up the readelf one in a bit.
Palmer Dabbelt
2017-10-19 16:26:14 UTC
Permalink
Post by Palmer Dabbelt
Post by H.J. Lu
Post by Palmer Dabbelt
I've had these two patches sitting in my tree for a few months. I've been
slowly going though and testing all our out-of-tree patches and getting
them
upstream, with these two as the last major ones we have. I was hoping to
go
convert all our relaxations over to using R_RISCV_DELETE, with the idea
being
that we could avoid the O(n^2) copying we do when relaxing, but it looks
like I
won't get time to do that for a while.
Despite the patches not being quite done, they do seem to work correctly
(they're been on the branch we at SiFive make releases from for a while),
enable a new optimization, and generally don't seem to hurt -- essentially
they
just add some code that should be used to clean up a few more places.
I'll commit these to master unless anyone has any objections or notices
anything I screwed up :).
[PATCH 1/2] RISC-V: Add R_RISCV_DELETE, which marks bytes for
[PATCH 2/2] RISC-V: Relax RISCV_PCREL_* to RISCV_GPREL_*
FAIL: difference of two undefined symbols
FAIL: align
FAIL: .sleb128 tests (2)
FAIL: .sleb128 tests (4)
FAIL: .sleb128 tests (5)
FAIL: .sleb128 tests (7)
FAIL: relax .uleb128
FAIL: Disabling section padding
FAIL: DWARF2 10
FAIL: lns-duplicate
FAIL: lns-common-1
FAIL: ld-elf/compressed1d
FAIL: ld-elf/stab
FAIL: ld-scripts/fill
FAIL: ld-scripts/empty-address-2a
FAIL: ld-scripts/empty-address-2b
FAIL: PHDRS2
FAIL: ld-scripts/size-1
Please fix them.
Thanks.
Oh, sorry about that -- it turns out we actually haven't been running the
binutils test suite (we just run the GCC test suite). It's something we really
should be doing, it's just hard to find the time to actually get through these
things with all the other issues we keep finding :). I've managed to get the
GAS test suite clean with these two tests, though the readelf one might be
no good. Are those two OK for master?
I've added the binutils test suites to our toolchain regression test suite so
we shouldn't be adding any new test failures on any commits from now on. I'll
keep pushing on the other test failures, but it might take a bit to get through
all of them.
Given that this doesn't add any new test failures, is it OK for master?
Thanks for pointing this out!
[PATCH 1/2] RISC-V: Fix readelf's R_RISCV_{ADD,SUB}{8,16,32,64}
[PATCH 2/2] RISC-V: Mark unsupported gas testcases
I commited the original patch set, I'm still working through the test suite
failures.

Continue reading on narkive:
Loading...