Fix LTO unreferenced symbol bug due to alias renaming

[Yin Ma via llvm-commits]

Hi  Rafael,

Thanks a lot. I am so glad that another author has created the extra same fix with a test.

Yin 

-----Original Message-----
From: Rafael Espíndola [mailto:rafael.espindola at gmail.com] 
Sent: Wednesday, January 20, 2016 2:11 PM
To: Yin Ma
Cc: Tim Amini Golling; llvm-commits
Subject: Re: Fix LTO unreferenced symbol bug due to alias renaming

I just approved http://reviews.llvm.org/D16330 which is the same fix but has a testcase.

Sorry for taking so long to reply, I was on vacations at the start of the year and I am still catching up.

Cheers,
Rafael


On 19 January 2016 at 13:46, Yin Ma <yinma at codeaurora.org> wrote:
> Hello,
>
>
>
> Any update on this review?
>
>
>
> Thanks,
>
>
>
> Yin
>
>
>
>
>
> From: mehdi.amini at apple.com [mailto:mehdi.amini at apple.com]
> Sent: Friday, January 08, 2016 8:30 AM
>
>
> To: Yin Ma
> Cc: llvm-commits
> Subject: Re: Fix LTO unreferenced symbol bug due to alias renaming
>
>
>
> Hi,
>
>
>
> Let’s wait Monday, Rafael should be back, it may be easier for him to 
> come up with a simpler test (or he may just take the patch without any 
> tests, we’ll see).
>
>
>
> —
>
> Mehdi
>
>
>
> On Jan 7, 2016, at 5:20 PM, Yin Ma <yinma at codeaurora.org> wrote:
>
>
>
> Hi Mehdi,
>
>
>
> It is not straight forward to reduce it. If it is just
>
> Extern foo from 1.ll
>
> Define foo from 2.ll
>
> Extern foo from 3.ll
>
> It wouldn’t show the problem. The definition of foo have to be 
> recorded by
>
> scanning other codes before the definition is resolved.
>
>
>
> The current test case is about 1000 lines with a lot of
>
> c++ setup. It has to be
>
> in three files like
>
> llvm-link 1.ll 2.ll 3.ll –o lto.o
>
> llvm-nm lto.o
>
> it shows
>
>                U _ZN7cOfooD2Ev
>
> -------- T _ZN7cOfooD2Ev.1
>
> Which later causes _ZN7cOfooD2Ev unreferenced. Because the definition 
> of
>
> _ZN7cOfooD2Ev becomes ZN7cOfooD2Ev.1
>
>
>
> _ZN7cOfooD2Ev is renamed in copyGlobalValueProto(SGV, ShouldLink);
>
> Because there is already a value with the same name when 1.ll is processed.
>
>
>
> I hope you could come up a test case based on my description if you 
> are
>
> familiar with what IRMover.cpp is doing. Or you have an automatic tool 
> to
>
> have me to reduce it further.
>
>
>
> Thanks,
>
>
>
> Yin
>
>
>
>
>
> From: mehdi.amini at apple.com [mailto:mehdi.amini at apple.com]
> Sent: Thursday, January 07, 2016 4:39 PM
> To: Yin Ma
> Cc: llvm-commits
> Subject: Re: Fix LTO unreferenced symbol bug due to alias renaming
>
>
>
> Hi,
>
>
>
> I’d rather have a test case with it.
>
> Is it hard to “anonymize” the test case? I don’t expect it to be large 
> considering your description of the problem, you should be able to 
> discard almost all the body of all function, strip the metadata, strip 
> the function parameter, strip the custom types if any, and rename the 
> few remaining function to foo and bar?
>
>
>
> —
>
> Mehdi
>
>
>
>
>
> On Jan 7, 2016, at 4:29 PM, Yin Ma <yinma at codeaurora.org> wrote:
>
>
>
> Hi Mehdi,
>
>
>
> I have developed a test case for llvm-link. However, I just figure out
>
> The test case was developed from licensed code. I cannot released
>
> to the public. Could you review my change without test case.
>
>
>
> Thanks,
>
>
>
> Yin
>
>
>
>
>
> From: mehdi.amini at apple.com [mailto:mehdi.amini at apple.com]
> Sent: Thursday, January 07, 2016 1:34 PM
> To: Yin Ma
> Cc: llvm-commits
> Subject: Re: Fix LTO unreferenced symbol bug due to alias renaming
>
>
>
> Hi Yin,
>
>
>
> Do you have a test-case for that?
>
>
>
> Thanks,
>
>
>
> Mehdi
>
>
>
> On Jan 7, 2016, at 12:28 PM, Yin Ma via llvm-commits 
> <llvm-commits at lists.llvm.org> wrote:
>
>
>
> Hi,
>
>
>
> After 18f201b4db339145af8f340196943ef57b9c8d9f from Rafael was merged 
> in, we are experiencing
>
> unreferenced symbol in several LTO programs. I analyzed the problem 
> and find because 18f201 patch
>
> tries to discard the aliasee by renaming it. However, it will create a 
> problem
>
> If the renaming happens for external functions, where non byte code 
> may reference to the original
>
> name and cause unreferenced symbol at the end.
>
>
>
> So, we have to perform forceRenaming like before without 18f201 for 
> those symbols in ShouldLink.
>
> Like this to force those global symbols which may expose to other 
> module to be the original name.
>
>
>
> diff --git a/lib/Linker/IRMover.cpp b/lib/Linker/IRMover.cpp
>
> index 16de0ec..68c3346 100644
>
> --- a/lib/Linker/IRMover.cpp
>
> +++ b/lib/Linker/IRMover.cpp
>
> @@ -1077,7 +1077,7 @@ Constant 
> *IRLinker::linkGlobalValueProto(GlobalValue
> *SGV, bool ForAlias) {
>
>        return nullptr;
>
>      NewGV = copyGlobalValueProto(SGV, ShouldLink);
>
> -    if (!ForAlias)
>
> +    if (ShouldLink || !ForAlias)
>
>        forceRenaming(NewGV, SGV->getName());
>
>    }
>
>    if (ShouldLink || ForAlias) {
>
>
>
> Please review.
>
>
>
> Thanks,
>
>
>
> Yin
>
>
>
>
>
> <i.diff>_______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>