And I'm out on vacation again this week. :). Thanks, will commit when I am back Mon Jul 5. Teresa
On Mon, Jun 29, 2015, 5:04 PM Duncan P. N. Exon Smith <dexonsm...@apple.com> wrote: > Whereas I just missed this go by and have no excuse :/. LGTM too, FWIW. > > > On 2015-Jun-29, at 16:54, Reid Kleckner <r...@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 <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