This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Added support for --emit-relocs.
AbandonedPublic

Authored by grimar on Jan 12 2017, 9:10 AM.

Details

Reviewers
ruiu
rafael
Summary

This is PR31579.

-q, --emit-relocs - Generate relocations in output

We used DynsymIndex for storing symtab index of symbol for -r
and I reused it in this patch for the same for --emit-relocs.
Had to add restriction that --emit-reloc and -pie/-shared/.so inputs
are not compatible together (so .dynsym is never created with --emit-reloc).
That seems fine as looks enough to fix PR31579.

Alternative probably can be another variable in SymbolBody for that,
but I am not sure we want it.

Diff Detail

Event Timeline

grimar updated this revision to Diff 84133.Jan 12 2017, 9:10 AM
grimar retitled this revision from to [ELF] - Added support for --emit-relocs..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar, evgeny777.
ruiu edited edge metadata.Jan 12 2017, 12:59 PM

Did you verify that this output is the same as ld.bfd's output?

ELF/Driver.cpp
166–169

Why do you need this restriction?

223–225

Why?

ELF/InputFiles.cpp
416–417

Use if instead of ?:.

In D28612#644422, @ruiu wrote:

Did you verify that this output is the same as ld.bfd's output?

Sure I did,
below is output from bfd:

Section Headers:
  [Nr] Name              Type             Address           Offset
       Size              EntSize          Flags  Link  Info  Align
  [ 0]                   NULL             0000000000000000  00000000
       0000000000000000  0000000000000000           0     0     0
  [ 1] .text             PROGBITS         0000000000400078  00000078
       0000000000000018  0000000000000000  AX       0     0     4
  [ 2] .rela.text        RELA             0000000000000000  000001f0
       0000000000000060  0000000000000018   I       4     1     8
  [ 3] .shstrtab         STRTAB           0000000000000000  00000250
       0000000000000026  0000000000000000           0     0     1
  [ 4] .symtab           SYMTAB           0000000000000000  00000090
       0000000000000108  0000000000000018           5     6     8
  [ 5] .strtab           STRTAB           0000000000000000  00000198
       0000000000000052  0000000000000000           0     0     1

Relocation section '.rela.text' at offset 0x1f0 contains 4 entries:
  Offset          Info           Type           Sym. Value    Sym. Name + Addend
00000040007a  00010000000a R_X86_64_32       0000000000400078 .text + 1
00000040007f  000a00000004 R_X86_64_PLT32    0000000000400078 fn - 4
000000400086  00010000000a R_X86_64_32       0000000000400078 .text + d
00000040008b  000700000004 R_X86_64_PLT32    0000000000400084 fn2 - 4

Symbol table '.symtab' contains 11 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND 
     1: 0000000000400078     0 SECTION LOCAL  DEFAULT    1 
     2: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS emit-relocs.s.tmp1.o
     3: 0000000000400079     0 NOTYPE  LOCAL  DEFAULT    1 bar
     4: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS emit-relocs.s.tmp2.o
     5: 0000000000400085     0 NOTYPE  LOCAL  DEFAULT    1 foo
     6: 0000000000601000     0 NOTYPE  GLOBAL DEFAULT    1 __bss_start
     7: 0000000000400084     0 FUNC    GLOBAL DEFAULT    1 fn2
     8: 0000000000601000     0 NOTYPE  GLOBAL DEFAULT    1 _edata
     9: 0000000000601000     0 NOTYPE  GLOBAL DEFAULT    1 _end
    10: 0000000000400078     0 FUNC    GLOBAL DEFAULT    1 fn

and our output:

Section Headers:
  [Nr] Name              Type             Address           Offset
       Size              EntSize          Flags  Link  Info  Align
  [ 0]                   NULL             0000000000000000  00000000
       0000000000000000  0000000000000000           0     0     0
  [ 1] .text             PROGBITS         0000000000201000  00001000
       0000000000000018  0000000000000000  AX       0     0     4
  [ 2] .rela.text        RELA             0000000000000000  00001018
       0000000000000060  0000000000000018           4     1     8
  [ 3] .comment          PROGBITS         0000000000000000  00001078
       0000000000000012  0000000000000001  MS       0     0     1
  [ 4] .symtab           SYMTAB           0000000000000000  00001090
       00000000000000a8  0000000000000018           6     5     8
  [ 5] .shstrtab         STRTAB           0000000000000000  00001138
       0000000000000035  0000000000000000           0     0     1
  [ 6] .strtab           STRTAB           0000000000000000  0000116d
       0000000000000011  0000000000000000           0     0     1

Relocation section '.rela.text' at offset 0x1018 contains 4 entries:
  Offset          Info           Type           Sym. Value    Sym. Name + Addend
000000201002  00020000000a R_X86_64_32       0000000000201000  + 1
000000201007  000500000004 R_X86_64_PLT32    0000000000201000 fn - 4
00000020100e  00040000000a R_X86_64_32       000000000020100c  + 1
000000201013  000600000004 R_X86_64_PLT32    000000000020100c fn2 - 4

Symbol table '.symtab' contains 7 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND 
     1: 0000000000201001     0 NOTYPE  LOCAL  DEFAULT    1 bar
     2: 0000000000201000     0 SECTION LOCAL  DEFAULT    1 
     3: 000000000020100d     0 NOTYPE  LOCAL  DEFAULT    1 foo
     4: 000000000020100c     0 SECTION LOCAL  DEFAULT    1 
     5: 0000000000201000     0 FUNC    GLOBAL DEFAULT    1 fn
     6: 000000000020100c     0 FUNC    GLOBAL DEFAULT    1 fn2

DIfference that bfd creates 1 section symbol, we create 2 with different values. But final values in '.rela.text'
are the same after that, so I think that should work fine.

Another difference that bfd marks ".rela.text" section with SHF_INFO_LINK flag (original objects does not have it),
I am not sure if it is critical to have, I think we can try without for now and add it separatelly if we need.

grimar updated this revision to Diff 84272.Jan 13 2017, 3:22 AM
grimar edited edge metadata.
  • Addressed review comments.
ELF/Driver.cpp
166–169

Like I mentioned in patch description, currently we use Symbol->DynsymIndex to store symbol index in .symtab for -relocatable output.
That works because -r means static linking, so .dynsym not exist.
We need to know symtab symbol index when perform fixup of relocations in InputSection<ELFT>::copyRelocations(),
because .rela.text is linked with .symtab.

This patch reuses Symbol->DynsymIndex for the same purpose for --emit-reloc.
That should work for linux kerner I think. So I had to add the same restrictions for now.

That restrictions probably can be fixed with adding one more variable to Symbol, like SymtabIndex instead of reusing
DynsymIndex. But I am not sure if it is worst to do ?

223–225

Reasons in comment above,
Out condition for creating .dynsymtab is:

bool HasDynSymTab = !Symtab<ELFT>::X->getSharedFiles().empty() || Config->Pic;
ELF/InputFiles.cpp
416–417

Done.

ruiu added a comment.Jan 13 2017, 10:22 AM

Can you please investigate the Linux kernel why they are using this option before proceeding with this patch? The fact that they are using the option is not strong enough to justify that we need to implement it, because the use might be easily replaced by other features. And if that's the case, we don't need this change. We need to understand use cases before checking in any new feature.

ELF/Driver.cpp
166–169

That wasn't good in the first place. We shouldn't have reused a field for different purposes in such a obscure way. That needs to be fixed first.

In D28612#645471, @ruiu wrote:

Can you please investigate the Linux kernel why they are using this option before proceeding with this patch? The fact that they are using the option is not strong enough to justify that we need to implement it, because the use might be easily replaced by other features. And if that's the case, we don't need this change. We need to understand use cases before checking in any new feature.

Sure. I'll try to do that.

ELF/Driver.cpp
166–169

Yep. I think we did it to reduce Symbol size, but I never liked reusing this variable, I am happy to prepare the patch for it :)

So after little investigation about how linux kerner works with this featue I found next:

  1. It links the real mode loader, and produces binary file realmode.bin using objcopy and realmode.relocs file with relocations using /x86/tools/relocs tool:

(http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/tree/arch/x86/realmode/rm/Makefile?id=f2604c141a00c00b92b7fd2f9d2455517fdd6c15#n43)

$(obj)/realmode.bin: $(obj)/realmode.elf
	$(call if_changed,objcopy)

quiet_cmd_relocs = RELOCS  $@
      cmd_relocs = arch/x86/tools/relocs --realmode $< > $@
$(obj)/realmode.relocs: $(obj)/realmode.elf FORCE
	$(call if_changed,relocs)
  1. /x86/tools/relocs --realmode do next:

(https://github.com/torvalds/linux/blob/5924bbecd0267d87c24110cbe2041b5075173a25/arch/x86/tools/relocs.c#L1012)

		write_reloc(relocs16.count, stdout);
		for (i = 0; i < relocs16.count; i++)
			write_reloc(relocs16.offset[i], stdout);

		write_reloc(relocs32.count, stdout);
		for (i = 0; i < relocs32.count; i++)
			write_reloc(relocs32.offset[i], stdout);

So its output is a custom binary that has amount the relocations and relocations itself following.
Also it do some relocations sorting sorting before that code.

  1. Then https://github.com/torvalds/linux/blob/master/arch/x86/realmode/rmpiggy.S packs the

result realmode.bin and realmode.relocs into single file, into .init.data section.
Also it creates symbol real_mode_relocs:

GLOBAL(real_mode_blob)
	.incbin	"arch/x86/realmode/rm/realmode.bin"
END(real_mode_blob)

GLOBAL(real_mode_blob_end);

GLOBAL(real_mode_relocs)
	.incbin	"arch/x86/realmode/rm/realmode.relocs"
END(real_mode_relocs)
  1. After that all init.c uses the all data from above to perform self-relocations:

(https://github.com/torvalds/linux/blob/5924bbecd0267d87c24110cbe2041b5075173a25/arch/x86/realmode/init.c#L69)

	/* 32-bit linear relocations. */
	count = *rel++;
	while (count--) {
		u32 *ptr = (u32 *) (base + *rel++);
		*ptr += phys_base;
	}

I think we should just support that feature.
It is specific, but not huge itself + looks stoned in linux kernel atm.
It is easier to support it than to force users to create workarounds. What do you think ?

grimar added inline comments.Jan 16 2017, 8:39 AM
ELF/Driver.cpp
166–169

I prepared demo patch to show one of the ways to avoid reusing it. It is D28773.
Actually it seems that introducing something like Symbol::SymtabIndex would be simpler.
Though that makes Symbol class larger and -r/--emit-relocs are not often used probably to do that.
What do you think we should prefer ?

grimar added inline comments.Jan 16 2017, 8:56 AM
ELF/Driver.cpp
166–169

Also, may be we should just rename this variable to something more common, like SymbolIndex and leave it as is with current restrictions implemented in this patch ?
Seems it should work fine for linux kernel, it does not link agains DSO and not uses -pie/-shared.

grimar updated this revision to Diff 85131.EditedJan 20 2017, 6:43 AM
  • Rebased
  • Fixed crash happened because rel/rela section refered to section discarded in script. Added testcase.
grimar updated this revision to Diff 85132.Jan 20 2017, 6:47 AM
  • Minor cleanup.
grimar updated this revision to Diff 85383.Jan 23 2017, 7:19 AM
  • Addressed review comment (removed command line combination restrictions, what became possible after D29021).
  • Rebased.
ruiu added inline comments.Jan 23 2017, 1:43 PM
ELF/InputFiles.cpp
403–404

Needs comment.

// Relocation sections processed by the linker are usually removed
// from the output, so returning `nullptr` for the normal case.
// However, if -emit-relocs is given, we need to leave them in the output.
// (Some post link analysis tools need this information.)
ELF/LinkerScript.cpp
266–277

What is this?

grimar updated this revision to Diff 85540.Jan 24 2017, 1:28 AM
  • Addressed review comments.
  • Added few comments.
ELF/InputFiles.cpp
403–404

Done. Thanks for comment.

ELF/LinkerScript.cpp
266–277

It fixes crash reported for PR31579, showed in emit-reloc-discard.s testcase.

If we have some section, for example .debug_info and discard it from script,
then we are not able to emit .rela.debug_info section.

Because "section to which the relocation applies" is not exist:

// sh_info for SHT_REL[A] sections should contain the section header index of
// the section to which the relocation applies.
InputSectionBase<ELFT> *S = Sections[0]->getRelocatedSection();
this->Info = S->OutSec->SectionIndex;

So that code discards .rel[a] sections in that case as well. That is consistent to what gnu linkers do.
I added comment.

ruiu added inline comments.Jan 24 2017, 12:52 PM
ELF/InputFiles.cpp
409

nit: remove this blank line.

ELF/LinkerScript.cpp
266–277

This seems like you are implement your own GC in some sense here. You probably should use DependentSection member of InputSection to do that automatically.

grimar updated this revision to Diff 85729.Jan 25 2017, 4:07 AM
  • Addressed comments.

(reimplemented logic relative to discarding target section, added support and testcase for --gc-sections --emit-relocs used together).

ELF/LinkerScript.cpp
266–277

Thanks for hit about DependentSection, I used it in --gc-sections case (updated code and added testcase to show how it works now).

That place of code is used for handling "/DISCARD/". Which is handled in script much later than GC runs and not relative to GC. Though using of DependentSection helped here as well. It is not what I would call "automatically", but since it is not relative to GC, code looks good for me now. What do you think ?

grimar updated this revision to Diff 85730.Jan 25 2017, 4:14 AM
  • Minor cleanup in testcase.
grimar added inline comments.Jan 25 2017, 8:36 AM
ELF/InputFiles.cpp
409

I am going to remove that fatal error in following patch.
It is needed for linux kernel, but since it involves move DependentSection from InputSection<ELFT> to somewhere and also adding more additional tests probably, I think it is better not to do that change in this patch.
Hope it's ok.

ruiu added inline comments.Jan 25 2017, 11:03 AM
ELF/InputFiles.cpp
409

I think what you really need to do is to make it a vector just like Rafael did in https://reviews.llvm.org/D28626.

ELF/InputSection.cpp
45–56

Exporting this as a global function is probably not a good idea. Whether a section could be discarded or never be discarded can be decided when the section is instantiated (because the decision depends only on Sec->Type, Config->GcSections and its section name, all of which are available at the constructor.)

So, the right way of doing it is to set Live bit in the ctor if it should never be GC'ed.

ELF/LinkerScript.cpp
265–270

I don't think this is correct. You shouldn't add any code this function, or there's a bug in GC.

If section S depends on section T, and if S is marked as dead, T must be marked as dead too before the control reaches this function. So both S and T will be discarded without doing anything special.

grimar added inline comments.Jan 26 2017, 12:56 AM
ELF/InputFiles.cpp
409

I do not see how vector helps. Issue here is that

DependentSection is a member of InputSection, but
EhInputSection inhireted from InputSectionBase<ELFT>

EhInputSection : public InputSectionBase<ELFT>

In following patch I plan to move DependentSection to InputSectionBase probably to solve that problem.

ELF/LinkerScript.cpp
265–270

It is not relative to GC at all.
Please look at emit-reloc-discard.s.

Call is:

# RUN: echo "SECTIONS { /DISCARD/ : { *(.debug*) } }" > %t.script
# RUN: ld.lld --emit-relocs --script %t.script %t.o -o %t1

So .debug_* sections are discarded from script, GC is not involved and not executed.
And since .debug_ are discarded, .rela.debug_* should be discarded as well. That is what that place do.

grimar added inline comments.Jan 26 2017, 1:38 AM
ELF/InputSection.cpp
45–56

That looks good idea, I prepared patch D29170.

tpimh added a subscriber: tpimh.Jan 31 2017, 7:42 AM
tpimh added inline comments.Feb 1 2017, 11:26 AM
ELF/InputFiles.cpp
406

Building Linux kernel with CONFIG_ACPI=y also fails because of this.

grimar abandoned this revision.Feb 2 2017, 1:05 AM

Simplified version of this was posted as D29273.

ELF/InputFiles.cpp
406

I know. But simplified version of this patch (D29273) is still on review. I am going to prepare separate patch supporting this when it be landed.