This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Linkerscript: implemented SIZEOF(section)
ClosedPublic

Authored by grimar on Jul 28 2016, 6:46 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 65924.Jul 28 2016, 6:46 AM
grimar retitled this revision from to [ELF] - Linkerscript: implemented SIZEOF(section).
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar, davide, evgeny777.
ruiu added inline comments.Jul 28 2016, 11:45 AM
ELF/LinkerScript.h
154 ↗(On Diff #65924)

I don't think it's beautiful as you are holding a pointer to a factory just to get sizes of sections. Factory is too powerful to be here.

LinkerScript creates output sections. Why don't you save it instead?

Also, as I described in https://reviews.llvm.org/D22740, this is yet another example that you are accessing something other than Dot to evaluate expressions. I do not see a reason to handle Dot as a special case. If you like to pass all variables as parameters, why don't you pass it as a Expr's parameter?

grimar added inline comments.Jul 28 2016, 2:08 PM
ELF/LinkerScript.h
154 ↗(On Diff #65924)

I thought that holding factory can help to simplify. We can get rid of arguments in methods in some pending patches and use this factory member instead.
LinkerScript creates some output sections, but that is the same what factory knows already, so I supposed it it better to reuse it than save what we created.
Anyways I just realized that factory is not enough either. I need to get somehow all sections including those which are added in addPredefinedSections(). So I guess I need to store Writer::OutputSections pointer instead. I`ll think what can be done here tomorrow.

grimar added inline comments.Jul 29 2016, 5:10 AM
ELF/LinkerScript.h
154 ↗(On Diff #65924)

I do not see a reason to handle Dot as a special case.

I think Dot is a bit different and special. For example D22683 has calculateSymbolValue() which relies on ability to pass any value as Dot.
Main problem of Dot is that it is a value that always changes. I do not really feel it is a some config attribute, it's behavior more like a temporarily variable. I suggest to land few more LS things and return to idea to do something with Dot when it will be more clear how it's using can be simplified.

grimar updated this revision to Diff 66109.Jul 29 2016, 5:25 AM
  • Do not store Factory, store a pointer to output sections list instead.
ruiu accepted this revision.Jul 29 2016, 9:08 AM
ruiu edited edge metadata.

LGTM

ELF/LinkerScript.cpp
950 ↗(On Diff #66109)

I started to really like the way how we handle expressions using lambdas. It's short, versatile and easy-to-write.

This revision is now accepted and ready to land.Jul 29 2016, 9:08 AM
grimar added inline comments.Jul 29 2016, 9:12 AM
ELF/LinkerScript.cpp
950 ↗(On Diff #66109)

Yes, it is very nice looking I think !

This revision was automatically updated to reflect the committed changes.