[PATCH] D38239: [ELF] - Define linkerscript symbols early.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/LinkerScript.cpp:128-131
+  // If we reach here and symbol uses PROVIDE() that means symbol satisfies
+  // providing conditions. Since we are calling addSymbol() multiple times and
+  // will define symbol below, we want to disable futher checks.
+  Cmd->Provide = false;
----------------
grimar wrote:
> ruiu wrote:
> > This seems like an optimization that you can remove.
> I think I can't remove it. It is used for following case. If we have script like:
> "PROVIDE_HIDDEN(newsym = __ehdr_start + 5);" (linkerscript/symbol-reserved.s)
> 
> When `addSymbol` is called first time to define `newsym` early, `newsym` is undefined, so
> `if (Cmd->Provide && (!B || B->isDefined()))` check passes through and we define
> dummy `newsym`.
> 
> Next time when `addSymbol` called to set proper symbol value, `if` condition will not pass
> because symbol is defined. To let it pass I had to drop `Provide` to disable check.
That's too complicated. It seems you are making this function do too many different things, so you need to dispatch inside the function. Can you redesign?


================
Comment at: ELF/Writer.cpp:786-790
+    SymbolBody *Gp = Symtab->find("_gp");
+    if (!Gp || !isa<DefinedRegular>(Gp))
+      ElfSym::MipsGp = Symtab->addAbsolute<ELFT>("_gp", STV_HIDDEN, STB_LOCAL);
+    else
+      ElfSym::MipsGp = dyn_cast<DefinedRegular>(Gp);
----------------
grimar wrote:
> ruiu wrote:
> > It is not clear to me why you need to change this.
> Because otherwise I would have "duplicate symbol: _gp" error with script like
> "SECTIONS { .text : { *(.text) }   _gp = ABSOLUTE(.) + 0x100;   .got  : { *(.got) } }"
> I add `_gp` early, so `addAbsolute` would trigger the error without this check.
But if that's the case, that is applicable to all symbols. What if you linker scripts defines _gp_disp, for example?


https://reviews.llvm.org/D38239