Page MenuHomePhabricator

[ELF] - Change how -Ttext/-Tdata/-Tbss works.
AbandonedPublic

Authored by grimar on Dec 9 2016, 3:02 AM.

Details

Reviewers
ruiu
rafael
Summary

Previously user was unable to set -Ttext=0x0 for example,
it is PR31295. Result was a crash because of wrong calculation of file offsets offsets.
bfd sets the section address in that case, gold the segment address.

Patch implements bfd behavior, it places sections specified by -T* to be first in the load.
That way we can start assinging addresses from their VA and do not intersect with other sections.
Testcase shows what I mean.

Diff Detail

Event Timeline

grimar updated this revision to Diff 80876.Dec 9 2016, 3:02 AM
grimar retitled this revision from to [ELF] - Change now -Ttext/-Tdata/-Tbss works..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar, evgeny777.
grimar retitled this revision from [ELF] - Change now -Ttext/-Tdata/-Tbss works. to [ELF] - Change how -Ttext/-Tdata/-Tbss works..Dec 9 2016, 7:28 AM
ruiu added inline comments.Dec 9 2016, 11:26 AM
ELF/Driver.cpp
584–588

I do not get the meaning of this code. Can you give me an example?

ELF/Writer.cpp
1299–1308

What does this code do? Needs explanation.

1310

You shouldn't compute the same value twice. Use the previous result instead.

1353–1357

This seems too tricky. Is there any better way?

grimar updated this revision to Diff 81064.Dec 12 2016, 3:29 AM
  • Addressed review comments.
ELF/Driver.cpp
584–588

Sure, simplified from testcases, imagine we have next code:

.text
.globl _start
_start:
 nop

.section .rodata,"a"
 .quad 0

We have next loads then: R (headers, .rodata), RX(.text), RW(.data).
In that case if user gives -Ttext=0x0 we want to set .text VA to that value.
gnu linkers does not apply -rosegment by default how we do.
We want to do that It think because otherwise there is no easy way to make .text to have -Ttext VA.

After applying my lines we have: RX (headers, .rodata, .text), RO(.data). So We can place .text
to be first section and assign VA starting from it.

ELF/Writer.cpp
1299–1308

Moved to getImageBase(), added comment.

1353–1357

Correctness of that place is under discussions in llvm-mails at this moment, I leaved it as is for now.
If we deside to leave this logic, I can suggest just to move it upper as a "better way":

if (!Config->SectionStartMap.empty())
  return Off;

That is a bit simpler though does not align file offset for data segment. I do not know if that is important.
bfd ignores aligning for text segment, I guess we can do the same for .data as well then.

Another way can probably be:

if (First->Addr > Sec->Addr)
  return Off;
return First->Offset + Sec->Addr - First->Addr;

That should keep consistency with bfd here.

grimar abandoned this revision.Dec 20 2016, 1:58 AM

r290136 committed instead.