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

Duncan P. N. Exon Smith dexonsmith at apple.com
Mon Jun 29 17:04:14 PDT 2015


Whereas I just missed this go by and have no excuse :/.  LGTM too, FWIW.

> On 2015-Jun-29, at 16:54, Reid Kleckner <rnk at google.com> wrote:
> 
> 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
> 





More information about the llvm-commits mailing list