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

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/LinkerScript.cpp:409-410
@@ +408,4 @@
+    if ((!B && !SS->Cmd->Provide) || (B && B->isUndefined())) {
+      Symbol *S = Symtab<ELFT>::X->addSynthetic(
+          SS->Cmd->Name, SS->OutSec, SS->Cmd->Expression(SS->OutSecOff));
+      S->Visibility = SS->Cmd->Hidden ? STV_HIDDEN : STV_DEFAULT;
----------------
Now you have a dummy input section. Is there any reason to use Synthetic symbols instead of Regular symbols?

================
Comment at: ELF/LinkerScript.cpp:813-816
@@ -755,1 +812,6 @@
+  else
+    // The section definition contains only symbols or nothing at all.
+    // Make all symbols global and discard the section.
+    std::move(Cmd->Commands.begin(), Cmd->Commands.end(),
+              std::back_inserter(Opt.Commands));
 }
----------------
Why do you need to handle this case as a special case?

================
Comment at: ELF/LinkerScript.cpp:821-822
@@ -758,3 +820,4 @@
   expect("(");
-  if (SymbolAssignment *Assignment = readAssignment(next())) {
+  std::unique_ptr<SymbolAssignment> Assignment = readAssignment(next());
+  if (Assignment) {
     Assignment->Provide = true;
----------------
readAsssignment will never return a null pointer, so you can remove `if`.

================
Comment at: ELF/LinkerScript.h:155
@@ -153,2 +154,3 @@
   uintX_t Dot;
+  std::vector<std::unique_ptr<InputSectionBase<ELFT>>> Synthetics;
 };
----------------
It seems you are storing only unique_ptr<SymbolInputSection> to this vector. Why don't you use SymbolInputSection instead of InputSectionBase?

================
Comment at: ELF/Writer.cpp:736
@@ -739,1 +735,3 @@
 
+  // Should be called after assignOffsets().
+  Script<ELFT>::X->addSyntheticSymbols();
----------------
People who are reading this code for the first time don't know what assignOffset did and what the following function is going to do, so writing only an exception wouldn't help them. Instead please write about what this is going to do.

================
Comment at: ELF/Writer.cpp:737
@@ +736,3 @@
+  // Should be called after assignOffsets().
+  Script<ELFT>::X->addSyntheticSymbols();
+
----------------
So, does this work? Adding new symbols changes the size of .symtab and .strtab, no?


https://reviews.llvm.org/D22683