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

[George Rimar via Phabricator via llvm-commits]

grimar 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;
----------------
ruiu wrote:
> 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?
Done. What do you think about new version ?


================
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);
----------------
ruiu wrote:
> 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?
Yeah, but them do not break any tests, so change for them can be prepared+committed separatelly.
(I did similar change in D36579 and it even has testcase, going to reuse it, but don't want to do that in this patch).


https://reviews.llvm.org/D38239