[PATCH] D27276: [ELF] Allow defined symbols to be assigned from linker script

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/LinkerScript.cpp:111-118
+  if (!B || B->isUndefined()) {
+    if (Cmd->Expression.IsAbsolute())
+      Cmd->Sym = addRegular<ELFT>(Cmd);
+    else
+      Cmd->Sym = addSynthetic<ELFT>(Cmd);
+  } else {
+    Cmd->Sym = B;
----------------
meadori wrote:
> ruiu wrote:
> > meadori wrote:
> > > ruiu wrote:
> > > > I don't think you need to check if B exists. You can add a new symbol unconditionally.
> > > If you do it unconditionally, then duplicate symbols might be created, e.g. `foo` is defined in an input object *and* the linker script.
> > That shouldn't happen because of the architecture of LLD. Did you actually try that?
> I did, and several tests in the test suite fail.  Consider making unconditional with the following diff against my current patch:
> 
> ```
> -  if (!B || B->isUndefined()) {
> -    if (Cmd->Expression.IsAbsolute())
> -      Cmd->Sym = addRegular<ELFT>(Cmd);
> -    else
> -      Cmd->Sym = addSynthetic<ELFT>(Cmd);
> -  } else {
> -    Cmd->Sym = B;
> -  }
> +  if (Cmd->Expression.IsAbsolute())
> +    Cmd->Sym = addRegular<ELFT>(Cmd);
> +  else
> +    Cmd->Sym = addSynthetic<ELFT>(Cmd);
> ```
> 
> Then:
> 
> ```
> $ ./bin/llvm-nm test.o 
> 0000000000000001 A foo
> $ cat test.ld
> foo = 12;
> $ ./bin/ld.lld test.o -T test.ld
> ./bin/ld.lld: error: duplicate symbol 'foo' in test.o and (internal)
> ```
Got it. So addRegular and such check for conflicts. But what we want to do here is to replace symbols unconditionally. Otherwise, this code doesn't work for some cases. For example, what if an existing symbol is a common symbol?

I think you want directly call replaceBody<> to replace symbol body unconditionally.


https://reviews.llvm.org/D27276