Whereas I just missed this go by and have no excuse :/. LGTM too, FWIW. > On 2015-Jun-29, at 16:54, Reid Kleckner <[email protected]> wrote: > > Back from vacation myself. It's that time of year. :) > > lgtm > > On Tue, Jun 23, 2015 at 11:39 AM, Teresa Johnson <[email protected]> wrote: > > > On Thu, Jun 11, 2015 at 8:17 PM, Teresa Johnson <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 | [email protected] | > 408-460-2413 >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
