Back from vacation myself. It's that time of year. :) lgtm
On Tue, Jun 23, 2015 at 11:39 AM, Teresa Johnson <tejohn...@google.com> wrote: > > > On Thu, Jun 11, 2015 at 8:17 PM, Teresa Johnson <tejohn...@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 <tha...@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 <tejohn...@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 | tejohn...@google.com | 408-460-2413 >
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
