Page MenuHomePhabricator

[ELF] Simpler scheme for handling common symbols
ClosedPublic

Authored by bd1976llvm on Sep 21 2017, 8:36 AM.

Details

Summary

Convert all common symbols to regular symbols after scan.

This means that the downstream code does not to handle common symbols as a special case.

Diff Detail

Repository
rL LLVM

Event Timeline

bd1976llvm created this revision.Sep 21 2017, 8:36 AM
ruiu edited edge metadata.Sep 21 2017, 10:38 AM

First of all, could you read https://llvm.org/docs/CodingStandards.html and follow that style? One of the easiest way is to run clang-format-diff on your patch so that everything in your patch is formatted automatically.

ELF/Driver.cpp
1088 ↗(On Diff #116196)

Remove lld::elf::. In general, please look around and make your new code looks like the same in style as other code.

ELF/MarkLive.cpp
67–68 ↗(On Diff #116196)

Just remove the code.

229–230 ↗(On Diff #116196)

Remove

ELF/SyntheticSections.cpp
64 ↗(On Diff #116196)

create -> Create

It's not a fake section. We call it a synthetic section.

Please add a full stop at end.

65 ↗(On Diff #116196)

auto * -> auto *

69–70 ↗(On Diff #116196)

Remove Pos and assert.

71 ↗(On Diff #116196)

We do not use auto unless its actual type is obvious from RHS.

73 ↗(On Diff #116196)

change -> Change

Add a full stop.

In D38137#877803, @ruiu wrote:

First of all, could you read https://llvm.org/docs/CodingStandards.html and follow that style? One of the easiest way is to run clang-format-diff on your patch so that everything in your patch is formatted automatically.

What a mess! Thanks for reviewing the patch despite how unstylish it is.
I agree with all the points you raised - will update the patch.

Updated the diff to address review comments.

bd1976llvm marked 7 inline comments as done.Sep 21 2017, 4:19 PM
ruiu added inline comments.Sep 22 2017, 11:25 AM
ELF/Driver.cpp
1084–1086 ↗(On Diff #116287)

Probably it is better to describe what this does, instead of explaining post-conditions.

// Create a .bss section for each common symbol and then replace the common symbol with a DefinedRegular symbol. As a result, all common symbols are "instantiated" as regular defined symbols, so that we don't need to care about common symbols beyond this point. Note that if -r is given, we just need to pass through common symbols as-is.
ELF/Symbols.cpp
103 ↗(On Diff #116287)

Messages for llvm_unreachable don't usually start with "unreachable: ", so remove it.

ELF/SyntheticSections.cpp
72 ↗(On Diff #116287)

I'd also describe the motivation of doing this.

// Replace all DefinedCommon symbols with DefinedRegular symbols so that we don't have to care about DefinedCommon symbols beyond this point.
ELF/SyntheticSections.h
744 ↗(On Diff #116287)

I think I'd keep the original function name because the function still creates common sections, in addition to replace DefinedCommon symbols with DefinedRegular symbols. It looks that the function creates common sections is more important than replacing symbols.

bd1976llvm marked 5 inline comments as done.

Updated as per review comments.
Apologies that it took some time to update the patch.

ruiu added inline comments.Sep 25 2017, 11:08 AM
ELF/SyntheticSections.cpp
57 ↗(On Diff #116551)

Please add a comment:

Create a .bss section for each common section and replace the common symbol with a DefinedRegular symbol.

64–70 ↗(On Diff #116551)

It is a bit more readable create an instance and then once it's done add it to InputSections.

auto *Section = make<BssSection>("COMMON");
Section->File = Sym->getFile();
Section->Live = !Config->GcSections;
Section->reserveSpace(Sym->Size, Sym->Alignment);
InputSections.push_back(Section);
74 ↗(On Diff #116551)

You are passing Section to replaceBody, but do you still need this?

75 ↗(On Diff #116551)

Do you need B? You already have Sym which is an instance of a subclass of SymbolBody.

76 ↗(On Diff #116551)

== 1 is odd; just like we don't do if (<some boolean expression> == 1), I'd omit it.

78 ↗(On Diff #116551)

0u -> 0 (0 is 0 however it is sign-extended.)

Do you actually need this comment?

updated with review comments.

bd1976llvm marked 5 inline comments as done.Sep 26 2017, 12:45 AM
ruiu added inline comments.Sep 27 2017, 9:16 PM
ELF/Symbols.cpp
282 ↗(On Diff #116648)

I don't think this is how clang-format formats.

ELF/SyntheticSections.cpp
73 ↗(On Diff #116648)

80 col.

ruiu added inline comments.Sep 27 2017, 9:52 PM
ELF/MarkLive.cpp
224–225 ↗(On Diff #116648)

Since you have removed the following code, you can remove this return.

auto MarkSymbol = [&](SymbolBody *Sym) {
  if (auto *D = dyn_cast_or_null<DefinedRegular>(Sym))
    if (auto *Sec = cast_or_null<InputSectionBase>(D->Section))
      Enqueue(Sec, D->Value);
};

Addressed review comments.

bd1976llvm marked 2 inline comments as done.

Missed a review comment. Now addressed.

bd1976llvm marked an inline comment as done.Sep 28 2017, 9:32 AM
bd1976llvm added inline comments.
ELF/Symbols.cpp
282 ↗(On Diff #116648)

I tried both the Windows and the Linux versions of the latest clang format. In both cases I got the same formatting. Am I doing something wrong? What do you expect the formatting to be here?

ruiu accepted this revision.Sep 28 2017, 1:08 PM

LGTM. Thanks!

ELF/MarkLive.cpp
221 ↗(On Diff #117010)

nit: remove {}

ELF/Symbols.cpp
282 ↗(On Diff #116648)

Oh sorry, then that's fine.

This revision is now accepted and ready to land.Sep 28 2017, 1:08 PM
This revision was automatically updated to reflect the committed changes.