[Patch] (take 2) New EliminateAvailableExternally pass / Pass down -flto

[Reid Kleckner]

Back from vacation myself. It's that time of year. :)

lgtm

On Tue, Jun 23, 2015 at 11:39 AM, Teresa Johnson <tejohnson at google.com>
wrote:

>
>
> On Thu, Jun 11, 2015 at 8:17 PM, Teresa Johnson <tejohnson at google.com>
> wrote:
>
>> Hi Nico,
>>
>> Sorry about that. Since I am heading out on vacation for a week tomorrow
>> I went ahead and reverted for now.
>>
>> Teresa
>>
>> On Thu, Jun 11, 2015 at 6:07 PM, Nico Weber <thakis at chromium.org> wrote:
>>
>>> Hi Teresa,
>>>
>>> this (well, 239480 really) seems to break building dynamic libraries
>>> pretty decisively:
>>> https://code.google.com/p/chromium/issues/detail?id=499508#c3 Can you
>>> take a look, and if it takes a while to investigate, revert this for now?
>>>
>>> Thanks,
>>> Nico
>>>
>>
> I am back from vacation and found what was happening here. The attached
> patches are largely the same as the original ones but contain a fix for
> this issue (llvm patch) and a new test created from Nico's reduced test
> (clang patch).
>
> The broken code was compiled -fvisibility=hidden, and this caused
> the thunk to SyncMessageFilter::Send
> (_ZThn16_N17SyncMessageFilter4SendEP7Message), which is
> available_externally, to also be marked hidden.
>
> With my patch, we eliminated the function's body and turned it into a
> declaration, which was still marked hidden as I wasn't modifying
> visibility. During AsmPrinter::doFinalization, EmitVisibility is called on
> all function declarations in the module, which caused this symbol to get
> hidden visibility (via a resulting call to MCSymbolELF::setVisibility). The
> linker then complained because it was undefined and hidden.
>
> Before my patch, code gen suppressed the emission of this function since
> it was available externally, and as a result, EmitVisibility, and
> thus MCSymbolELF::setVisibility, were simply never called. The undefined
> symbol then ended up with the default visibility. It seems to me that this
> essentially worked by luck.
>
> I've fixed this by changing the visibility on globals to DefaultVisibility
> when we eliminate their definitions in the new EliminateAvailableExternally
> pass.
>
> Patches attached. Tests (including the new one) all pass. Ok to commit?
>
> Thanks,
> Teresa
>
>
>>> On Wed, Jun 10, 2015 at 10:49 AM, Teresa Johnson <tejohnson at google.com>
>>> wrote:
>>>
>>>> Author: tejohnson
>>>> Date: Wed Jun 10 12:49:45 2015
>>>> New Revision: 239481
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=239481&view=rev
>>>> Log:
>>>> Pass down the -flto option to the -cc1 job, and from there into the
>>>> CodeGenOptions and onto the PassManagerBuilder. This enables gating
>>>> the new EliminateAvailableExternally module pass on whether we are
>>>> preparing for LTO.
>>>>
>>>> If we are preparing for LTO (e.g. a -flto -c compile), the new pass is
>>>> not
>>>> included as we want to preserve available externally functions for
>>>> possible
>>>> link time inlining.
>>>>
>>>
> --
> Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150629/c130f047/attachment.html>