Bug 24896 - [powerpc] ld can probably drop R_PPC64_UADDR64 conversion
Summary: [powerpc] ld can probably drop R_PPC64_UADDR64 conversion
Status: RESOLVED WONTFIX
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: 2.33
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-08-12 03:35 UTC by Fangrui Song
Modified: 2019-08-14 14:17 UTC (History)
1 user (show)

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


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Fangrui Song 2019-08-12 03:35:48 UTC
Related to both gas and ld.

gas emits R_PPC_ADDR32 and R_PPC64_ADDR64 at unaligned locations.
ld converts R_PPC64_ADDR64 to R_PPC64_UADDR64 if unaligned, and R_PPC64_UADDR64 to R_PPC64_ADDR64 if aligned.

  bfd/elf64-ppc.c:ppc64_elf_relocate_section

    /* Optimize unaligned reloc use.  */
    if ((r_type == R_PPC64_ADDR64 && (out_off & 7) != 0)
        || (r_type == R_PPC64_UADDR64 && (out_off & 7) == 0))
      r_type ^= R_PPC64_ADDR64 ^ R_PPC64_UADDR64;
    else if ((r_type == R_PPC64_ADDR32 && (out_off & 3) != 0)
             || (r_type == R_PPC64_UADDR32 && (out_off & 3) == 0))
      r_type ^= R_PPC64_ADDR32 ^ R_PPC64_UADDR32;
    else if ((r_type == R_PPC64_ADDR16 && (out_off & 1) != 0)
             || (r_type == R_PPC64_UADDR16 && (out_off & 1) == 0))
      r_type ^= R_PPC64_ADDR16 ^ R_PPC64_UADDR16;

% d.c
char y;
struct{char _; const char *x; char *y;} __attribute__((packed)) _ = {'_', "a", &y};
void _start() {}

% powerpc64le-linux-gnu-gcc -c -fpic d.c; readelf -Wr d.o | grep R_PPC64
0000000000000001  0000000500000026 R_PPC64_ADDR64         0000000000000000 .rodata + 0
0000000000000009  0000000900000026 R_PPC64_ADDR64         0000000000000001 y + 0
% powerpc64le-linux-gnu-ld -pie d.o -o d; readelf -Wr d | grep R_PPC64
0000000000020001  000000010000002b R_PPC64_UADDR64        0000000000000298 .text + 28
0000000000020009  000000020000002b R_PPC64_UADDR64        0000000000020000 .data + 11
% powerpc64le-linux-gnu-ld -shared d.o -o d.so; readelf -Wr d.so | grep R_PPC64
0000000000020001  000000010000002b R_PPC64_UADDR64        0000000000000288 .text + 28
0000000000020009  000000040000002b R_PPC64_UADDR64        0000000000020011 y + 0

This is not the case on powerpc. ld doesn't do such unaligned reloc optimization:

% powerpc-linux-gnu-gcc -c -fpic d.c; readelf -Wr d.o | grep R_PPC
00000001  00000501 R_PPC_ADDR32           00000000   .rodata + 0
00000005  00000901 R_PPC_ADDR32           00000001   y + 0
% powerpc-linux-gnu-ld -pie d.o -o d; re -Wr d | grep R_PPC
00020001  00000016 R_PPC_RELATIVE                    1b8
00020005  00000016 R_PPC_RELATIVE                    2001c
% powerpc-linux-gnu-ld -shared d.o -o d.so; re -Wr d.so | grep R_PPC
00020001  00000016 R_PPC_RELATIVE                    1c4
00020005  00000301 R_PPC_ADDR32           0002001c   y + 0


* gcc/as: R_PPC_UADDR32 and R_PPC64_UADDR64 should be produced.
  If sh_addralign is sufficiently aligned, gas should emit UADDR at an unaligned location.
  Otherwise (e.g. sh_addralign=1), gas probably can't reasonably decide whether ADDR/UADDR should be used at an unaligned location. Maybe gcc should emit more information to help gas?
* ld: I am still not clear whether the ADDR -> UADDR conversion is reasonable,
      but 64-bit UADDR -> ADDR conversion can probably be dropped, it has very little benefit.

The expected result:

% powerpc-linux-gnu-gcc -c -fpic d.c; readelf -Wr d.o | grep R_PPC
...  R_PPC_UADDR32
...  R_PPC_UADDR32
% powerpc-linux-gnu-ld -pie d.o -o d; re -Wr d | grep R_PPC
...  R_PPC_UADDR32   # ADDR can use RELATIVE if the target symbol is non-preemptable but UADDR can't.
...  R_PPC_UADDR32
% powerpc-linux-gnu-ld -shared d.o -o d.so; re -Wr d.so | grep R_PPC
...  R_PPC_UADDR32
...  R_PPC_UADDR32
Comment 1 Fangrui Song 2019-08-13 15:00:34 UTC
Thinking more. ADDR/UADDR distinction probably makes little sense.
I withdraw my proposal that teachs gas to emit UADDR.

Some ld conversion rules can probably be deleted:

Aligned R_PPC64_UADDR64 -> R_PPC64_ADDR64 is definitely not needed
Misaligned R_PPC64_ADDR64 -> R_PPC64_UADDR64 I am not clear on this one, but I guess it probably doesn't hurt if misaligned R_PPC64_ADDR64 or R_PPC64_RELATIVE is generated, especially on powerpc64le (defaults to -mcpu=power8)
Comment 2 Alan Modra 2019-08-14 08:30:34 UTC
ld selects R_PPC64_UADDR*/R_PPC64_ADDR* depending on their alignment for dynamic relocations, and glibc ld.so applies them taking into account that R_PPC64_UADDR* may not be aligned whereas all other dynamic relocs (including R_PPC64_RELATIVE) are assumed to be aligned.  This mattered when we were developing the ppc64le support on power7 processors.  (Yes, ppc64le was originally supported on power7 even though the hardware support for little-endian was somewhat lacking, leading to lots of alignment traps on mis-aligned memory accesses.)

It's true that a strict interpretation of the relevant ABI documents would require assemblers to generate UADDR relocs in a lot more cases, but we haven't done that and I don't see any good reason to modify gas to do that now.  At worst it might cause some linkers running on old hardware to take alignment traps when processing object files, slowing down the linker a little.  Slightly slower tools don't matter.

On power8 and later, with glibc ld.so compiled for power8, you should see the R_PPC64_UADDR* relocs being processed just like the corresponding R_PPC64_ADDR* reloc (ie. using full width memory writes rather than multiple byte writes).  So there isn't much reason to avoid R_PPC64_UADDR* relocs.
Comment 3 Fangrui Song 2019-08-14 10:28:09 UTC
Thanks for the information.

* Aligned R_PPC64_UADDR64 -> R_PPC64_ADDR64  => the code is there and it doesn't hurt anyone, so let's keep it
* Misaligned R_PPC64_ADDR64 -> R_PPC64_UADDR64 => this matters on power7 and previous processors (assumably they may trap for a misaligned 64-bit access)

Am I correct? There is probably a bug in glibc powerpc32. For a dynamic relocation R_PPC_UADDR32 to a STB_LOCAL STT_SECTION symbol, the relocated value is wrong.
The offending code was added in 2003 but there seems no way for gas to emit UADDR32, so ld won't emit dynamic UADDR32, and this bug gets unnoticed for years...


// glibc/sysdeps/power/powerpc32/dl-machine.h
auto inline void __attribute__ ((always_inline))
elf_machine_rela (struct link_map *map, const Elf32_Rela *reloc,
		  const Elf32_Sym *sym, const struct r_found_version *version,
		  void *const reloc_addr_arg, int skip_ifunc)
{
  ...
  /* binutils on ppc32 includes st_value in r_addend for relocations
     against local symbols.  */
  if (__builtin_expect (ELF32_ST_BIND (sym->st_info) == STB_LOCAL, 0)
      && sym->st_shndx != SHN_UNDEF)
    {
      sym_map = map;
      value = map->l_addr; /////// st_value is not considered
    }
  else
    {
      sym_map = RESOLVE_MAP (&sym, version, r_type);
      value = SYMBOL_ADDRESS (sym_map, sym, true);
    }
  value += reloc->r_addend;
Comment 4 Alan Modra 2019-08-14 14:17:39 UTC
Yes, st_value is not added by ppc32 ld.so because st_value is included in r_addend for STB_LOCAL symbols by ld.  This goes back further than 2003, in fact to 1997.  The 2003 patch you found reverted an earlier 2003 patch.  We can't fix this without changing both binutils and glibc in sync.