This is an archive of the discontinued LLVM Phabricator instance.

Mitigate relocation overflow [part 2 of 2]
ClosedPublic

Authored by shenhan on Apr 27 2018, 10:32 AM.

Details

Summary

This CL is to mitigate R_X86_64_PC32 relocation overflow problems for huge binaries that has near 2G allocated sections.

By examining those binaries, I found these 2 issues contributes to the problem:
1). huge ".dynsym" and ".dynstr" stands in the way between .rodata and .text
2). _init_array_start/end are placed at 0 if no ".init_array" presents, this causes .text relocation against them become more prone to overflow.

This CL is to address 2).
( 1) is addressed in D45788

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

shenhan created this revision.Apr 27 2018, 10:32 AM
shenhan updated this revision to Diff 147135.May 16 2018, 11:14 AM
shenhan edited the summary of this revision. (Show Details)
shenhan edited reviewers, added: grimar; removed: espindola.

Hi George, mind take a look? (This is a follow up of D45788.)

It looks a bit hacky. But I think it should work. Rui, what do you think?

ELF/Writer.cpp
1710

We do not write like that I think: !(DefaultOutSec = findSection(".text").
Please move the assignment to the DefaultOutSec outside of the if.

I think ".text" can be a default for both Pic and !Pic probably.

shenhan updated this revision to Diff 147333.May 17 2018, 9:28 AM
shenhan marked an inline comment as done.
shenhan added inline comments.
ELF/Writer.cpp
1710

Revised. Thanks.

ruiu added inline comments.May 17 2018, 10:45 AM
ELF/Writer.cpp
1704–1721

I'd think that this comment could be improved by making it clear what is the default behavior, what your problem is, and how to solve it. The important thing is that future readers will be able to understand this comment without too much context. I'd write something like this.

If a section does not exist, there's ambiguity as to how we define _start and _end symbols for an init/fini section. Since the loader assume that the symbols are always defined, we need to always define them. But what value? The loader iterates over all pointers between _start and _end to run global ctors/dtors, so if the section is empty, their symbol values don't actually matter as long as _start and _end point to the same location.

That said, we don't want to set the symbols to 0 (which is probably the simplest value) because that could cause some program to fail to link due to relocation overflow, if their program text is above 2 GiB. We use the address of the .text section instead to prevent that failure.

1709

Since this function is very small, we probably should use shorter names. I'd name this just Text.

grimar added inline comments.May 17 2018, 11:16 AM
ELF/Writer.cpp
1709

But Text does not look correct for Out::ElfHeader case. Maybe OutSec?

grimar added inline comments.May 17 2018, 11:19 AM
ELF/Writer.cpp
1709

Oops. OutSec is confusing since we have OS. DefaultSec?

ruiu added a comment.May 17 2018, 11:31 AM

I think Text is fine, but if you don't like it, I'd name it Default.

grimar added a comment.EditedMay 17 2018, 11:36 AM

I think Text is fine, but if you don't like it, I'd name it Default.

Default sounds/fits better IMO. Thanks!

shenhan updated this revision to Diff 147388.May 17 2018, 2:10 PM
shenhan marked 2 inline comments as done.

Thanks. (Comments revised and DefaoutOutSec -> Default)

ruiu accepted this revision.May 17 2018, 2:11 PM

LGTM

ELF/Writer.cpp
1711

Add an empty comment line //.

This revision is now accepted and ready to land.May 17 2018, 2:11 PM
grimar accepted this revision.May 18 2018, 2:08 AM

LGTM.

grimar closed this revision.May 18 2018, 2:19 AM

Was committed as r332688

Hint: If you will put "Differential revision: https://reviews.llvm.org/DXXXXX" to the end of your commit message, reviews will be closed automatically.