[PATCH] D22683: [ELF] Symbol assignment within input section list

[Rui Ueyama via llvm-commits]

ruiu accepted this revision.
ruiu added a comment.

LGTM


================
Comment at: ELF/LinkerScript.cpp:140
@@ +139,3 @@
+namespace {
+template <class ELFT> class LayoutInputSection : public InputSection<ELFT> {
+public:
----------------
I don't think it's a good idea to piggyback on InputSection<ELFT> because LayoutInputSection is (and should be) fairly different from InputSection. LayoutSection has no relocation information, no data, no thunks, etc.

In general, please try to make class hierarchy shallower if possible. I don't think you need to address it now, but I'll take a look after it's submitted.

================
Comment at: ELF/LinkerScript.cpp:339
@@ +338,3 @@
+  // OutputSectionBuilder<ELFT>::finalize
+  OutputSection<ELFT> *OutSec = dyn_cast<OutputSection<ELFT>>(Sec);
+  if (Sec->getSize() || !OutSec)
----------------
Use `auto`.

================
Comment at: ELF/LinkerScript.cpp:347
@@ +346,3 @@
+  for (InputSection<ELFT> *I : OutSec->Sections) {
+    if (LayoutInputSection<ELFT> *L =
+            dyn_cast<LayoutInputSection<ELFT>>(I)) {
----------------
Use `auto`.

================
Comment at: ELF/LinkerScript.cpp:359-361
@@ +358,5 @@
+    }
+    // Update section size, so that SIZEOF works correctly in the case below:
+    // .foo { ... a = SIZEOF(.foo) ... }
+    Sec->setSize(Off);
+  }
----------------
Do you have to do this in this for-loop? It seems that you can do this only once after this for-loop.


https://reviews.llvm.org/D22683