This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Make Bss and BssRelRo sections to be synthetic (#3).
ClosedPublic

Authored by grimar on Mar 13 2017, 7:26 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Mar 13 2017, 7:26 AM
ruiu added inline comments.Mar 13 2017, 11:55 AM
ELF/Relocations.cpp
483 ↗(On Diff #91558)

I remember that last time I had to change this type from size_t to uintX_t because your previous change broke buildbots.

ELF/SyntheticSections.h
156–157 ↗(On Diff #91558)

BssRelSection sections? This needs updating.

"BssSection is used to reserve space for copy relocations. We create two instances of this class for .bss and .bss.rel.ro. .bss is use for writable symbols, and .bss.rel.ro is used for read-only symbols."

165 ↗(On Diff #91558)

Rafael made a change to rename AddrAlign Alignment. Please take a look at that change and make your change consistent with that.

grimar updated this revision to Diff 91725.Mar 14 2017, 7:41 AM
  • Addressed review comments.
  • New BssSection is no more <ELFT> templated.
ELF/Relocations.cpp
483 ↗(On Diff #91558)

Right, thanks.

ELF/SyntheticSections.h
165 ↗(On Diff #91558)

Done.

ruiu accepted this revision.Mar 14 2017, 10:17 AM

LGTM

ELF/SyntheticSections.h
166 ↗(On Diff #91725)

Make this private if you are always accessing this via getSize.

This revision is now accepted and ready to land.Mar 14 2017, 10:17 AM
This revision was automatically updated to reflect the committed changes.
grimar reopened this revision.Mar 16 2017, 9:07 AM
This revision is now accepted and ready to land.Mar 16 2017, 9:07 AM
grimar updated this revision to Diff 92006.Mar 16 2017, 9:11 AM
  • Fixed one more issue that hopefully should be enough now for aarch64 bot.

Issue was relative to symbol alignment. If we had common symbol and ".bss" section for it with alignment 4
and then created copy relocation for symbol with alignment == 8 then final layout was wrong.

It was AAAABBBBBBBB when should be AAAA0000BBBBBBBB, where A - space for common symbol and
B - space for copy relocated.

That happened because previously we would create 2 .bss sections (one for commons, one for relocations),
but now create single.

Testcase is provided.

This revision was automatically updated to reflect the committed changes.