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

[George Rimar via llvm-commits]

>>>> Though currently with Out::ElfHeader following cast asserts for me, because Out::ElfHeader is an input section,
>>>> not output: https://github.com/llvm-mirror/lld/blob/master/ELF/SymbolTable.cpp#L441
>>>> Is section needed there for something unobvious ?
>>>
>>>That is the thin lto case, no? That is where I think there is something
>>>odd with thin lto. It is producing a symbol we didn't ask for. Since the
>>>symbol is already defined, we end up trying to report a duplicated
>>>symbol error and crashing.
>>>
>>>If thin lto would omit that symbol like regular LTO does we would not
>>>get any errors.
>>>
>>>Cheers,
>>>Rafael
>>
>> Ok. Want me to try investigate what is wrong with thin lto case then ?
>> (I can try, though that probably be after 8th jan, we have a week+1 day of NY holidays starting
>> on next week here).
>
>If you don't mind that would be awesome. It is only thinlto, and from
>the linker perspective having thinlto not produce the extra symbol is
>exactly what we want. If it is not too hard that would be the perfect
>fix IMHO.
>
>Thanks,
>Rafael

I tried to investigate/debug it and think I do no not 
completely understand what behavior should be considered as correct, sorry :(

Though it seems I was able to fix thinLTO case, my thoughts are below.
(patch for LTO is attached, all LLD and llvm\test\LTO tests pass with it).

Part I. "Concerns about expected behavior".

Imagine we have first.ll:

target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

define hidden void @bar() {
  call void asm "nop", ""()
  ret void
}

and second.s:

.global bar
bar:
nop


If we try to link them as is using regular LTO (I am assuming it has correct behavior),
link will fail: 

llvm-as first.ll -o first.o
llvm-mc -filetype=obj -triple=x86_64-pc-linux second.s -o second.o
ld.lld first.o second.o -o out
ld.lld: error: duplicate symbol: bar

It looks reasonable and valid behavior, right ?

Now if we use -defsym instead:
ld.lld first.o -defsym=bar=11 -o out
It links fine and result output contains ABS bar with value of 0xb.

And it works only because defsym logic uses replaceSymbol<Defined>, so it replaces
already existent Defined symbol bar (from BC) instead of reporting multiple declaration.
If logic would use addDefined, it would fail.
That probably means that -defsym still does not work as "this makes the linkerscript defined symbol
just like .o defined symbols". And it is actually unclear for me why we should not just report error here.

Part II. "ThinLTO fix for defsym.ll"
(patch for below is in attachment)

Currently LTO sets WeakAnyLinkage for all symbols that are Prevailing and have LinkerRedefined flag set.
I am not sure I understand why it is not done just for all LinkerRedefined symbols. 
According to comment there "we want to switch the linkage to `weak` to prevent IPOs from happening",
so dont we need to do that for all linker redefined symbols, like symbols used in -defsym ?
Changing that here:
(https://github.com/llvm-mirror/llvm/blob/master/lib/LTO/LTO.cpp#L717)

stops `bar2` from being exported:
https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/FunctionImport.cpp#L181

and then LTO sets `bar2` linkage to internal here:
https://github.com/llvm-mirror/llvm/blob/master/lib/LTO/LTO.cpp#L329

So `bar2` never shows up and we have no multiple defenition error anymore.

George.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: thinLTO.patch
Type: application/octet-stream
Size: 48869 bytes
Desc: thinLTO.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180111/3046923a/attachment.obj>