This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Define __bss_start symbol.
ClosedPublic

Authored by grimar on Feb 27 2017, 11:18 AM.

Details

Summary

GNU linkers define __bss_start symbol.
Patch teaches LLD to do that. This is PR32051.

Below is part of standart ld.bfd script:

.data1          : { *(.data1) }
  _edata = .; PROVIDE (edata = .);
  . = .;
  __bss_start = .;
  .bss            :
  {

Currently LLD can emit up to 3 .bss* sections as one of testcase shows.
Implementation inserts this symbol before first .bss* output section.

Diff Detail

Event Timeline

grimar created this revision.Feb 27 2017, 11:18 AM
ruiu edited edge metadata.Feb 27 2017, 11:33 AM

Generally looking fine, but Rafael might be working on removing DefinedSynthetic class, so he probably want to take a look.

test/ELF/bss-start.s
16 ↗(On Diff #89908)

Do you need this .global?

20–22 ↗(On Diff #89908)

You can remove this without changing the meaning of this test, no?

grimar added inline comments.Feb 28 2017, 1:11 AM
test/ELF/bss-start.s
16 ↗(On Diff #89908)

I believe yes. We do not want to add symbol when it is not used, right ?
Declaring it as global adds it as undefined to symbol table and
addOptionalSynthetic() then able to create DefinedSynthetic:

addOptionalSynthetic(StringRef Name, OutputSection *Sec,
                     typename ELFT::uint Val, uint8_t StOther = STV_HIDDEN) {
  if (SymbolBody *S = Symtab<ELFT>::X->find(Name))
    if (!S->isInCurrentDSO())
      return cast<DefinedSynthetic>(
          Symtab<ELFT>::X->addSynthetic(Name, Sec, Val, StOther)->body());
return nullptr;

Otherwize there will be no symbol in output.

20–22 ↗(On Diff #89908)

First test intention is not only to check that .bss.rel.ro can be used for _bss_start,
but also to show that _bss_start points to the first .bss section if there are multiple in output.
Lines were intentional.

grimar updated this revision to Diff 93463.Mar 30 2017, 4:16 AM
  • Rebased.
rafael edited edge metadata.Mar 30 2017, 6:20 AM

Rui, can you check with whatever is using this what the intended behavior would be when there is a .bss and a .bss.rel.ro? I would guess it wants to see .bss since .bss.rel.ro is ro at runtime.

ELF/Writer.cpp
849

just say ".bss". It is the name of the output section.

ruiu added a comment.Mar 30 2017, 12:12 PM

You don't really need three tests because in this feature you don't really need to (re-)verify that our section merging logic is working fine. Just reduce it to one test.

ELF/Writer.cpp
849

Yes, please. "... is the location of .bss section."

1682–1683

You shouldn't use Set. Just ElfSym::BssStart->Section = findSection(".bss"); should suffice.

grimar updated this revision to Diff 94054.Apr 4 2017, 5:17 AM
  • Addressed review comments.
grimar added a comment.Apr 4 2017, 5:19 AM
In D30419#714407, @ruiu wrote:

You don't really need three tests because in this feature you don't really need to (re-)verify that our section merging logic is working fine. Just reduce it to one test.

Done, though FYI AFAIK we do not have any test that chechs .bss merging logic. I can probably prepare testcases and push on review (probably based on testcases that were done in this patch).
Do we want it ?

ELF/Writer.cpp
1682–1683

That would not work so easy, but updated code works without Set.

grimar updated this revision to Diff 94159.Apr 4 2017, 9:35 PM
  • Addressed review comment properly (previously I changed naming ElfSym::BssStart to ElfSym::Bss,

but did not update comment as was requested, now it is fixed).

ruiu accepted this revision.Apr 4 2017, 9:40 PM

LGTM

This revision is now accepted and ready to land.Apr 4 2017, 9:40 PM
This revision was automatically updated to reflect the committed changes.