[ELF] Check that Elf_Rela addends are always written with -r

[arichardson]

Summary

The fix was committed as part of rL324064. This adds an x86 testcase

[grimar]

So this intruduces -relocation-format=xxx option.
Is it documented somewhere ? I did not find the description.

Looks it is for MIPS only ? I wonder why do you want to
mix .rel and .rel[a] objects and if it is really legal thing to do first of all ?

+Simon, he knows about MIPS much more than me.

[arichardson]

I originally discovered this issue because in our CHERI fork of LLD we set Config->IsRela = false when linking MIPS64 code. We do this because the FreeBSD MIPS rtld only understands Elf_Rel relocations and does not process Elf_Rela.

The issue here is not speicific to MIPS. It only depends on an input file containing Elf_Rela for a target that has Elf_Rel as the default output format.

I added the command line flag since this makes it possible to test this issue without fabricating .o files. Additionally, it will make it possible to use LLD for operating systems where rtld doesn't use Rela and the IsRela = Config->Is64 || IsX32 || Config->MipsN32Abi || Machine == EM_PPC. check is not correct.

[grimar]

In D42790#994774, @arichardson wrote:

I originally discovered this issue because in our CHERI fork of LLD we set Config->IsRela = false when linking MIPS64 code. We do this because the FreeBSD MIPS rtld only understands Elf_Rel relocations and does not process Elf_Rela.

It sounds like FreeBSD MIPS rtld bug honestly and probably should be fixed on their side.

I guess it currently works with GNU linkers because they always write the addend to output, even when output is .rela.
We faced something like that before when worked on x64 target. And it was fixed on FreeBSD side.

I do not think we want to add command line option like that in LLD. One possible option in theory would be to have
the same behavior as gnu linkers do, but it is also unclear it is needed, because seem helps to hide an issue instead of fixing it.

Lets wait to hear what other people think.

[atanasyan]

In D42790#994808, @grimar wrote:

In D42790#994774, @arichardson wrote:

I originally discovered this issue because in our CHERI fork of LLD we set Config->IsRela = false when linking MIPS64 code. We do this because the FreeBSD MIPS rtld only understands Elf_Rel relocations and does not process Elf_Rela.

It sounds like FreeBSD MIPS rtld bug honestly and probably should be fixed on their side.

MIPS O32 ABI uses the Elf_Rel format in object files, while N32 and N64 ABIs uses the Elf_Rela. But originally in case of all ABIs output relocations in executable files and DSO are written in the Elf_Rel format. I think the goal was to reduce a file size. In case of embedded systems with small amount of memory it can be a benefit. Maybe there are some other reasons. When I implement MIPS LLD backend I used the same format for input and output files, i.e. Elf_Rel for O32 ABI and Elf_Rela for N32 and N64 ABIs. It works for Linux because the dynamic linker accepts all formats. It looks like FreeBSD linker is more stricter.

[arichardson]

In D42790#994808, @grimar wrote:

I guess it currently works with GNU linkers because they always write the addend to output, even when output is .rela.
We faced something like that before when worked on x64 target. And it was fixed on FreeBSD side.

I have been working on adding RELA support to the mips rtld but this is quite annoying and will take a while. For now I have been able to run code just by setting Config->IsRela to false.

I don't know if the MIPS specifies that Elf_Rela should to be used for n64. I believe this choice is up to the operating system but I could be wrong.

I do not think we want to add command line option like that in LLD. One possible option in theory would be to have
the same behavior as gnu linkers do, but it is also unclear it is needed, because seem helps to hide an issue instead of fixing it.

This is actually fixing an issue since compilers other than clang are not required to use the same relocation kind as LLD for the object files.
I can also add a patch to completely reject any Elf_Rel relocations if Config->IsRela is set but I don't think that is the right way forward. Right now we are producing broken object files if this is the case and I would very much like to avoid that.

[grimar]

In D42790#994846, @atanasyan wrote:

In D42790#994808, @grimar wrote:

In D42790#994774, @arichardson wrote:

I originally discovered this issue because in our CHERI fork of LLD we set Config->IsRela = false when linking MIPS64 code. We do this because the FreeBSD MIPS rtld only understands Elf_Rel relocations and does not process Elf_Rela.

It sounds like FreeBSD MIPS rtld bug honestly and probably should be fixed on their side.

MIPS O32 ABI uses the Elf_Rel format in object files, while N32 and N64 ABIs uses the Elf_Rela. But originally in case of all ABIs output relocations in executable files and DSO are written in the Elf_Rel format. I think the goal was to reduce a file size. In case of embedded systems with small amount of memory it can be a benefit. Maybe there are some other reasons. When I implement MIPS LLD backend I used the same format for input and output files, i.e. Elf_Rel for O32 ABI and Elf_Rela for N32 and N64 ABIs. It works for Linux because the dynamic linker accepts all formats. It looks like FreeBSD linker is more stricter.

So should/can we switch LLD to always emit Elf_Rel for all MIPS outputs then ?

[ruiu]

So should/can we switch LLD to always emit Elf_Rel for all MIPS outputs then ?

I think I agree with George. REL/RELA is defined by the ABI and making it configurable seems odd.

[arichardson]

In D42790#995391, @ruiu wrote:

So should/can we switch LLD to always emit Elf_Rel for all MIPS outputs then ?

I think I agree with George. REL/RELA is defined by the ABI and making it configurable seems odd.

I only added the flag to make the testing easier. I can try to create the various input files using yaml2obj but that will require 4 different input files instead of just one .s file.

[ruiu]

Please add test files to test your code instead of adding a user-visible command line flag. User-visible flags shouldn't be added without careful planning.

[arichardson]

Since the patch was committed change this to only a test case (depends on D42839)

[ruiu]

LGTM

[atanasyan]

Is this patch still needs to be applied?

[arichardson]

In D42790#1328399, @atanasyan wrote:

Is this patch still needs to be applied?

Probably not. I could rewrite the different inputs as YAML since the llvm-mc change doesn't seem that useful.

ELF/Driver.cpp

 using namespace llvm::sys;
 
 using namespace lld;
 using namespace lld::elf;
 
 Configuration *elf::Config;
 LinkerDriver *elf::Driver;
 
-static void setConfigs();
+static void setConfigs(opt::InputArgList &Args);
 
 bool elf::link(ArrayRef<const char *> Args, bool CanExitEarly,
                raw_ostream &Error) {
   errorHandler().LogName = Args[0];
   errorHandler().ErrorLimitExceededMsg =
       "too many errors emitted, stopping now (use "
       "-error-limit=0 to see all errors)";
   errorHandler().ErrorOS = &Error;
if (ErrOrWriter) {
             toString(ErrOrWriter.takeError()));
     }
   }
 
   readConfigs(Args);
   initLLVM(Args);
   createFiles(Args);
   inferMachineType();
-  setConfigs();
+  setConfigs(Args);
   checkOptions(Args);
   if (errorCount())
     return;
 
   switch (Config->EKind) {
   case ELF32LEKind:
     link<ELF32LE>(Args);
     return;
for (auto *Arg : Args.filtered(OPT_version_script))
     if (Optional<MemoryBufferRef> Buffer = readFile(Arg->getValue()))
       readVersionScript(*Buffer);
 }
 
 // Some Config members do not directly correspond to any particular
 // command line options, but computed based on other Config values.
 // This function initialize such members. See Config.h for the details
 // of these values.
-static void setConfigs() {
+static void setConfigs(opt::InputArgList &Args) {
   ELFKind Kind = Config->EKind;
   uint16_t Machine = Config->EMachine;
 
   // There is an ILP32 ABI for x86-64, although it's not very popular.
   // It is called the x32 ABI.
   bool IsX32 = (Kind == ELF32LEKind && Machine == EM_X86_64);
 
   Config->CopyRelocs = (Config->Relocatable || Config->EmitRelocs);
   Config->Is64 = (Kind == ELF64LEKind || Kind == ELF64BEKind);
   Config->IsLE = (Kind == ELF32LEKind || Kind == ELF64LEKind);
   Config->Endianness =
       Config->IsLE ? support::endianness::little : support::endianness::big;
   Config->IsMips64EL = (Kind == ELF64LEKind && Machine == EM_MIPS);
-  Config->IsRela =
+  bool IsRelaDefault =
       Config->Is64 || IsX32 || Config->MipsN32Abi || Machine == EM_PPC;
+  Config->IsRela = Args.hasFlag(OPT_relocation_format_rela,
+                                OPT_relocation_format_rel, IsRelaDefault);
   Config->Pic = Config->Pie || Config->Shared;
   Config->Wordsize = Config->Is64 ? 8 : 4;
 }
 
 // Returns a value of "-format" option.
 static bool getBinaryOption(StringRef S) {
   if (S == "binary")
     return true;

ELF/InputSection.cpp

void InputSection::copyRelocations(uint8_t *Buf, ArrayRef<RelTy> Rels) {
 
   for (const RelTy &Rel : Rels) {
     RelType Type = Rel.getType(Config->IsMips64EL);
     Symbol &Sym = getFile<ELFT>()->getRelocTargetSym(Rel);
 
     auto *P = reinterpret_cast<typename ELFT::Rela *>(Buf);
     Buf += sizeof(RelTy);
 
-    if (Config->IsRela)
+    if (RelTy::IsRela)
       P->r_addend = getAddend<ELFT>(Rel);
 
     // Output section VA is zero for -r, so r_offset is an offset within the
     // section, but for --emit-relocs it is an virtual address.
     P->r_offset = Sec->getOutputSection()->Addr + Sec->getOffset(Rel.r_offset);
     P->setSymbolAndType(InX::SymTab->getSymbolIndex(&Sym), Type,
                         Config->IsMips64EL);
 
if (Sym.Type == STT_SECTION) {
         continue;
       }
       SectionBase *Section = D->Section;
       if (Section == &InputSection::Discarded) {
         P->setSymbolAndType(0, 0, false);
         continue;
       }
 
-      if (Config->IsRela) {
+      if (RelTy::IsRela) {
         P->r_addend =
             Sym.getVA(getAddend<ELFT>(Rel)) - Section->getOutputSection()->Addr;
       } else if (Config->Relocatable) {
         const uint8_t *BufLoc = Sec->Data.begin() + Rel.r_offset;
         Sec->Relocations.push_back({R_ABS, Type, Rel.r_offset,
                                     Target->getImplicitAddend(BufLoc, Type),
                                     &Sym});
       }

ELF/Options.td

 
 defm reproduce: Eq<"reproduce">,
   HelpText<"Dump linker invocation and input files for debugging">;
 
 defm rpath: Eq<"rpath">, HelpText<"Add a DT_RUNPATH to the output">;
 
 def relocatable: F<"relocatable">, HelpText<"Create relocatable object file">;
 
+def relocation_format_rel: F<"relocation-format=rel">,
+  HelpText<"Use Elf_Rel dynamic relocations instead of the default type">;
+
+def relocation_format_rela: F<"relocation-format=rela">,
+  HelpText<"Use Elf_Rela dynamic relocations instead of the default type">;
+
 defm retain_symbols_file: Eq<"retain-symbols-file">,
   HelpText<"Retain only the symbols listed in the file">,
   MetaVarName<"<file>">;
 
 defm script: Eq<"script">, HelpText<"Read linker script">;
 
 def section_start: S<"section-start">, MetaVarName<"<address>">,
   HelpText<"Set address of section">;

test/ELF/relocatable-rel-rela-mismatch.s

+# REQUIRES: mips
+# Check that we correctly write addends with -r if the output use Elf_Rel but
+# the input uses Elf_Rela
+
+# RUN: llvm-mc -filetype=obj -triple=mips64-unknown-linux %s -o %t-rela.o
+# RUN: llvm-objdump --section=.data -s %t-rela.o | FileCheck -check-prefix DATA-RELA %s
+# DATA-RELA:      Contents of section .data:
+# DATA-RELA-NEXT: 0000 00000000 00000000 abcdef00 12345678  .............4Vx
+
+# RUN: llvm-mc -filetype=obj -triple=mips-unknown-linux %s -o %t-rel.o
+# RUN: llvm-objdump --section=.data -s %t-rel.o | FileCheck -check-prefix DATA-REL %s
+# Rel should have the relocation value 10 in .data:
+# DATA-REL:      Contents of section .data:
+# DATA-REL-NEXT: 0000 00000000 00000010 abcdef00 12345678  .............4Vx
+#                                    ^---- (addend 0x10)
+
+# RUN: ld.lld -r -o %t-rel-r.o %t-rel.o
+# RUN: llvm-readobj -h -s -section-data -relocations %t-rel-r.o | FileCheck -check-prefix RELOCATABLE-REL %s
+
+# Note: llvm-readobj prints 0x0 as the relocation addend since it doesn't parse
+# REL relocations. In order to verify the addend we need to check the contents of .data
+# RELOCATABLE-REL:  ElfHeader {
+# RELOCATABLE-REL:    Class: 32-bit
+# RELOCATABLE-REL:    DataEncoding: BigEndian (0x2)
+# RELOCATABLE-REL:  Section {
+# RELOCATABLE-REL:       Name: .data
+# RELOCATABLE-REL:       SectionData (
+# RELOCATABLE-REL-NEXT:    0000: 00000000 00000010 ABCDEF00 12345678  |.............4Vx|
+#                                               ^--- Addend 0x10 for the relocation below
+# RELOCATABLE-REL-NEXT:  )
+
+# RELOCATABLE-REL:     Relocations [
+# RELOCATABLE-REL-NEXT:  Section ({{.+}}) .rel.data {
+# RELOCATABLE-REL-NEXT:     0x0 R_MIPS_64 foo 0x0
+# RELOCATABLE-REL-NEXT:  }
+# RELOCATABLE-REL-NEXT: ]
+
+
+# RUN: ld.lld -r -o %t-rela-r.o %t-rela.o
+# RUN: llvm-readobj -h -s -section-data -relocations %t-rela-r.o | FileCheck -check-prefix RELOCATABLE-RELA %s
+# RELOCATABLE-RELA:  ElfHeader {
+# RELOCATABLE-RELA:    Class: 64-bit
+# RELOCATABLE-RELA:    DataEncoding: BigEndian (0x2)
+# RELOCATABLE-RELA:  Section {
+# RELOCATABLE-RELA:       Name: .data
+# RELOCATABLE-RELA:       SectionData (
+# RELOCATABLE-RELA-NEXT:    0000: 00000000 00000000 ABCDEF00 12345678  |.............4Vx|
+#                                               ^--- No addend written here since it is part of Elf_Rela
+# RELOCATABLE-RELA:     Relocations [
+# RELOCATABLE-RELA-NEXT:  Section ({{.+}}) .rela.data {
+# RELOCATABLE-RELA-NEXT:     0x0 R_MIPS_64/R_MIPS_NONE/R_MIPS_NONE foo 0x10
+# RELOCATABLE-RELA-NEXT:  }
+# RELOCATABLE-RELA-NEXT: ]
+
+# Previously we would lose the addend if the output uses REL but a .foo.rela was being
+# copied into the relocatable output. This happenend because addends were only added
+# to the output section when Config->IsRela was true instead of checking for RelTy::IsRela.
+# We can trigger this situation by using the -relocation-format= flag to not match the input format.
+# Check that the relacation addends are copied into the output as expected:
+# RUN: ld.lld -r -o %t-forced-rela-r.o -relocation-format=rela %t-rel.o
+# RUN: llvm-readobj -h -s -section-data -relocations %t-forced-rela-r.o | FileCheck -check-prefix RELOCATABLE-REL %s
+# RUN: ld.lld -r -o %t-forced-rel-r.o -relocation-format=rel %t-rela.o
+# RUN: llvm-readobj -h -s -section-data -relocations %t-forced-rel-r.o  | FileCheck -check-prefix RELOCATABLE-RELA %s
+
+.extern foo
+
+.data
+.quad foo + 0x10
+.quad 0xabcdef0012345678