Page MenuHomePhabricator

[ARM] Support .reloc *, R_ARM_NONE, *
ClosedPublic

Authored by MaskRay on May 15 2019, 10:51 PM.

Details

Summary

R_ARM_NONE can be used to create references among sections. When
--gc-sections is used, the referenced section will be retained if the
origin section is retained.

Add a generic MCFixupKind FK_NONE as this kind of no-op relocation is
ubiquitous on ELF and COFF, and probably available on many other binary
formats. See D62014.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.May 15 2019, 10:51 PM
MaskRay updated this revision to Diff 199751.May 15 2019, 10:56 PM

Delete the change to getFixupKindContainerSizeBytes()

MaskRay updated this revision to Diff 199756.May 15 2019, 11:33 PM

Set shouldForceRelocation to allow 0 symbol index, e.g. .reloc 0, R_ARM_NONE, 8

I get an assertion failure from this patch on the armv7eb-linux-gnueabi test case: llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp:872! (the llvm_unreachable)

/// getFixupKindContainerSizeBytes - The number of bytes of the
/// container involved in big endian.
static unsigned getFixupKindContainerSizeBytes(unsigned Kind) {
  switch (Kind) {
  default:
    llvm_unreachable("Unknown fixup kind!");

  case FK_Data_1:
    return 1;
  case FK_Data_2:
    return 2;
  case FK_Data_4:
    return 4;

Looks like you'll need to add case ARM::fixup_arm_NONE:

MaskRay updated this revision to Diff 199777.May 16 2019, 3:24 AM

Add back getFixupKindContainerSizeBytes() change

I think this is looking close. I've got a small concern about what happens on non-ELF platforms Windows and Macho where R_ARM_NONE is not defined. Other than that it is just a few suggestions.

lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
50 ↗(On Diff #199756)

Is there a way of making R_ARM_NONE only accepted on ELF. There are also Windows (COFF) and MachO users that may come through this code-path, we need to make sure that they don't get a fixup they can't handle.

test/MC/ARM/reloc-directive.s
8 ↗(On Diff #199756)

Would it be possible to expand the test case to include something like:

        .text
        .globl foo
        .type foo, %function
foo:    bx lr

        .data
        .reloc 0, R_ARM_NONE, foo

As this will more closely match the real world usage for .reloc.

It may also be worth a test case that doesn't use the filetype=obj to make sure the asmprinter code path works.

17 ↗(On Diff #199756)

I suggest adding the reason they are ignored. Something like # Addends are ignored for SHT_REL relocations with no addend field.

MaskRay updated this revision to Diff 199810.May 16 2019, 6:28 AM

Improve tests as peter.smith suggested

MaskRay updated this revision to Diff 199812.May 16 2019, 6:36 AM
MaskRay marked an inline comment as done.

Change to if (STI.getTargetTriple().isOSBinFormatELF() && Name == "R_ARM_NONE")

MaskRay marked 3 inline comments as done.May 16 2019, 6:38 AM
MaskRay added inline comments.
lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
50 ↗(On Diff #199756)

Good point! Added a check STI.getTargetTriple().isOSBinFormatELF()

% llvm-mc -filetype=obj -triple=armv7a-windows-itanium reloc-directive.s -o t.o
reloc-directive.s:17:13: error: unknown relocation name
  .reloc 4, R_ARM_NONE, foo+4

% llvm-mc -filetype=obj -triple=armv7-apple-macho reloc-directive.s -o t.o
reloc-directive.s:17:13: error: unknown relocation name
  .reloc 4, R_ARM_NONE, foo+4

Before, the diagnostic for coff is:

LLVM ERROR: unsupported relocation type: fixup_arm_NONE

macho:

reloc-directive.s:17:3: error: unsupported relocation on symbol                                                 
  .reloc 4, R_ARM_NONE, foo+4

I think X86 and MIPS need a similar fix..

peter.smith accepted this revision.May 16 2019, 6:47 AM

Thanks, this looks good to me. May be worth waiting before committing to see if there are any comments from the US timezone.

This revision is now accepted and ready to land.May 16 2019, 6:47 AM
MaskRay updated this revision to Diff 199960.Thu, May 16, 7:34 PM
MaskRay edited the summary of this revision. (Show Details)

Add generic FK_NONE as suggested by pcc in the R_{386,X86_64}_NONE patch

MaskRay updated this revision to Diff 199961.Thu, May 16, 7:48 PM

Fix a typo in the test

This revision was automatically updated to reflect the committed changes.