This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Refactor synthetic sections and relocation processing. NFC.
ClosedPublic

Authored by sbc100 on May 10 2019, 4:43 PM.

Details

Summary

Major refactor to better match the structure of the ELF linker.

  • Split out relocation processing into scanRelocations
  • Split out synthetic sections into their own classes.

This is a major refactor but is mostly just moving code around. The
only non-trivial change is that to the way in which SectionSymbols
are generated. In order output section symbols we now track the
output section from each input section, and each output section can
have an options SectionSymbol.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

sbc100 created this revision.May 10 2019, 4:43 PM
Herald added a project: Restricted Project. · View Herald Transcript
sbc100 added a reviewer: ruiu.May 10 2019, 4:44 PM
sbc100 edited the summary of this revision. (Show Details)
sbc100 updated this revision to Diff 199110.May 10 2019, 5:01 PM
sbc100 edited the summary of this revision. (Show Details)
  • remove comments
sbc100 updated this revision to Diff 199332.May 13 2019, 2:39 PM
  • move work from OutputSection constructors to finalizeContent
sbc100 updated this revision to Diff 199345.May 13 2019, 4:19 PM
  • refactor all the things
sbc100 marked an inline comment as done.May 13 2019, 4:21 PM
sbc100 added inline comments.
lld/wasm/SyntheticSections.h
318 ↗(On Diff #199345)

I was wondering about the naming of this struct and global.

In ELF I think "In" might mean "Input" since there the synthetic sections are input sections. However we currently use OutputSections, so this name make less sense.

Perhaps I should call these "OutStruct" and "Out" or "Synth" perhaps?

Let me know if this is too much to review. I could try to break it down, but it is mostly a pure refactor.

Split out part of this change into a smaller CL: https://reviews.llvm.org/D61971

sbc100 updated this revision to Diff 199706.May 15 2019, 5:41 PM
  • rebase
sbc100 updated this revision to Diff 199947.May 16 2019, 5:42 PM
  • rebase
ruiu added inline comments.May 17 2019, 5:21 AM
lld/wasm/OutputSections.h
51 ↗(On Diff #199947)

nit: OutputSectionSymbol* -> OutputSectionSymbol *

lld/wasm/Relocations.cpp
38 ↗(On Diff #199947)

auto -> Symbol

58–59 ↗(On Diff #199947)

nit: you can merge the two ifs into one if.

75 ↗(On Diff #199947)

Isn't this the same as line 38?

86 ↗(On Diff #199947)

Ditto

lld/wasm/Symbols.h
20–21 ↗(On Diff #199947)

It would be nice to explain them as comment.

45 ↗(On Diff #199947)

It seems like a domain error that we have "output section kind" of a symbol, because symbols are not sections and vice versa. What is this?

201 ↗(On Diff #199947)

Ah, okay, so this class represents a section symbol... but it is not clear why we have two different section symbol types. Could you add comments?

sbc100 updated this revision to Diff 200116.May 17 2019, 4:26 PM
sbc100 marked 6 inline comments as done.
  • clang-format
  • feedback
sbc100 updated this revision to Diff 200242.May 20 2019, 4:00 AM
  • rename InStruct
sbc100 added inline comments.May 20 2019, 4:01 AM
lld/wasm/Symbols.h
20–21 ↗(On Diff #199947)

Done.

These are shared constants.. can you think of a better place to store them? Maybe Config?

ruiu added a comment.May 20 2019, 4:16 AM

Overall looking good. I didn't read each line that seems to be just a code move, though.

lld/wasm/Symbols.h
20–21 ↗(On Diff #199947)

I think just global variables are fine. Config are mainly for command line parameters.

lld/wasm/SyntheticSections.cpp
509 ↗(On Diff #200242)

nit: remove the empty line.

lld/wasm/SyntheticSections.h
318 ↗(On Diff #199345)

Either Out and Synth are fine. It's up to you.

lld/wasm/Writer.cpp
149–150 ↗(On Diff #200242)

Instead of skipping many sections that are not needed, does it make sense to add another loop to remove !isNeeded() sections from OutputSections?

532 ↗(On Diff #200242)

auto -> InputChunk?

sbc100 updated this revision to Diff 200373.May 20 2019, 4:09 PM
sbc100 marked 2 inline comments as done.
  • feedback
sbc100 updated this revision to Diff 200374.May 20 2019, 4:11 PM
  • cleanup
Harbormaster completed remote builds in B32186: Diff 200374.
ruiu accepted this revision.May 20 2019, 7:48 PM

LGTM

This revision is now accepted and ready to land.May 20 2019, 7:48 PM
This revision was automatically updated to reflect the committed changes.