Page MenuHomePhabricator

[ThinLTO] Keep available_externally symbols live
ClosedPublic

Authored by vlad.tsyrklevich on Feb 23 2018, 12:37 PM.

Details

Summary

This change fixes PR36483. The bug was originally introduced by a change
that marked non-prevailing symbols dead. This broke LowerTypeTests
handling of available_externally functions, which are non-prevailing.
LowerTypeTests uses liveness information to avoid emitting thunks for
unused functions.

Marking available_externally functions dead is incorrect, the functions
are used though the function definitions are not. This change keeps them
live, and lets the EliminateAvailableExternally/GlobalDCE passes remove
them later instead.

I've also enabled EliminateAvailableExternally for all optimization
levels, I believe it being disabled for O1 was an oversight.

Diff Detail

Event Timeline

pcc added a comment.Feb 23 2018, 2:19 PM

This seems like a reasonable fix to me, but I'd like to know what Teresa thinks.

Sorry for the delay, I was OOO for awhile and am still catching up.

lib/Transforms/IPO/FunctionImport.cpp
620

Is it possible to have a symbol that is weak in one module and available_externally in another? If so, it would be more correct to return only if none of the copies is available_externally.

lib/Transforms/IPO/PassManagerBuilder.cpp
523

Thanks for this fix. Can you commit separately with a test?

pcc added inline comments.Mar 5 2018, 1:46 PM
lib/Transforms/IPO/FunctionImport.cpp
620

I suppose so. In the situation where we have at least one available-externally we could end up needing a symbol referenced by the available-externally as a result of a backend inlining it into a caller. So we would need to keep them live anyway.

Now that I think about it, this applies to linkonce-odr as well (i.e. we could convert them to available-externally as a result of non-prevailing, and then inline them in the backend and thus end up needing a symbol that was referenced by the available-externally but not necessarily by the prevailing copy of the global). But that's somewhat of an orthogonal problem to this.

lib/Transforms/IPO/FunctionImport.cpp
620

As I mentioned to @pcc, his example would not be true for ThinLTO as of D42107 because it deletes dead symbols in lto::thinBackend. I'm not sure if it's possible to reach that case with regular LTO.

@tejohnson I think that case is technically possible; however, it would re-introduce the same bug that D42107 originally tried to fix: https://bugs.llvm.org/show_bug.cgi?id=35938

Give that, I'm not sure if there's a way to reconcile that change with this one. I'm not sure if marking individual symbols live/not-live is feasible?

  • Change the logic to set symbols live to match what Teresa suggested
  • Remove PassManagerBuilder change
tejohnson accepted this revision.Mar 8 2018, 6:26 AM

LGTM with fixes noted below.

lib/Transforms/IPO/FunctionImport.cpp
629

else if can be used here.

630

This is only used in the assert, so I suspect you will need to guard with #ifndef NDEBUG to avoid a warning about unused variable.

This revision is now accepted and ready to land.Mar 8 2018, 6:26 AM

Address Teresa's comments

This revision was automatically updated to reflect the committed changes.
twoh added a subscriber: twoh.Aug 16 2018, 12:22 AM

Hello, I wonder if we need to keep linkonce_odr symbols live here as well. I observe a case that a vtable for template class initiated has linkonce_odr linkage and marked dead here, which results compiler crash at WholeProgramDevirt because the global variable for vtable doesn't have initializer (https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/WholeProgramDevirt.cpp#L676 assumes that GV has the initializer). Thanks!

mehdi_amini added inline comments.Aug 16 2018, 9:21 PM
llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
550 ↗(On Diff #137617)

This explanation is not clear to me. I understand that this is fixing a bug (crash), but it isn't clear as of *why* this is the right fix?
I'm not sure why would the simple fact that a function is defined somewhere as "available eternally" would be enough to make them "live" if we don't have any reference anywhere to make them live for another reason?

Hello, I wonder if we need to keep linkonce_odr symbols live here as well. I observe a case that a vtable for template class initiated has linkonce_odr linkage and marked dead here, which results compiler crash at WholeProgramDevirt because the global variable for vtable doesn't have initializer (https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/WholeProgramDevirt.cpp#L676 assumes that GV has the initializer). Thanks!

I'm not sure why a linkonce_odr GV would be marked non-prevailing. Do you have a minimized example to look at?

llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
550 ↗(On Diff #137617)

I believe the logic here is actually different. https://reviews.llvm.org/D42107 changed the live root analysis to mark non-prevailing symbols as not live even if they are reachable. I changed the logic here to continue to mark available_externally symbols live only if they're reachable.

mehdi_amini added inline comments.Aug 20 2018, 10:12 AM
llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
550 ↗(On Diff #137617)

OK, if so then I misunderstood what's going on, but I'm also a bit lost with why are these symbols special with respect to liveness. I.e. why is it OK to make other "non-prevailing" symbols not live but not these ones? The comment just says "to not-live breaks downstreams users of liveness information", which falls a bit short for me to really understand the "why" here.

mehdi_amini added inline comments.Aug 20 2018, 10:14 AM
llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
550 ↗(On Diff #137617)

The "mark non-prevailing symbols as not live even if they are reachable" isn't clear to me either, but at this point I'm probably not up-to-date on the subtleties of "prevailing" vs "live" concept (Apple does not use "prevailing" as a concept)

Hello, I wonder if we need to keep linkonce_odr symbols live here as well. I observe a case that a vtable for template class initiated has linkonce_odr linkage and marked dead here, which results compiler crash at WholeProgramDevirt because the global variable for vtable doesn't have initializer (https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/WholeProgramDevirt.cpp#L676 assumes that GV has the initializer). Thanks!

I'm not sure why a linkonce_odr GV would be marked non-prevailing. Do you have a minimized example to look at?

Hi @vlad.tsyrklevich, @twoh

I am working on creating a reduced test case. Will get back to you.
Thanks

Hello, I wonder if we need to keep linkonce_odr symbols live here as well. I observe a case that a vtable for template class initiated has linkonce_odr linkage and marked dead here, which results compiler crash at WholeProgramDevirt because the global variable for vtable doesn't have initializer (https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/WholeProgramDevirt.cpp#L676 assumes that GV has the initializer). Thanks!

I'm not sure why a linkonce_odr GV would be marked non-prevailing. Do you have a minimized example to look at?

llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
550 ↗(On Diff #137617)

Prevailing is used to describe which definition of a symbol (e.g. with interposable linkage where multiple definition are available) is to be included in the final link. If you look at D42107 it's fixing https://bugs.llvm.org/show_bug.cgi?id=35938: when LTO marked the ELF symbol as prevailing, previously the ThinLTO symbol would still be marked live and internalized. D42107 was meant to address that; however, available_externally symbols are never prevailing--they are never meant to be included in the final build output. This means that even if one is called it would not have been marked live, and downstream passes like LowerTypeTests would not know that that symbol is actually used and emit jumptable entries for that symbol. We add special handling for available_externally because as far as I'm aware it should be the only linkage type that could hit this edge case (it's not prevailing but references to it are live, they are just replaced with references to a DSO instead.)

Hello, I wonder if we need to keep linkonce_odr symbols live here as well. I observe a case that a vtable for template class initiated has linkonce_odr linkage and marked dead here, which results compiler crash at WholeProgramDevirt because the global variable for vtable doesn't have initializer (https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/WholeProgramDevirt.cpp#L676 assumes that GV has the initializer). Thanks!

I'm not sure why a linkonce_odr GV would be marked non-prevailing. Do you have a minimized example to look at?

Hi @vlad.tsyrklevich, @twoh

I am working on creating a reduced test case. Will get back to you.
Thanks

Hello, I wonder if we need to keep linkonce_odr symbols live here as well. I observe a case that a vtable for template class initiated has linkonce_odr linkage and marked dead here, which results compiler crash at WholeProgramDevirt because the global variable for vtable doesn't have initializer (https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/WholeProgramDevirt.cpp#L676 assumes that GV has the initializer). Thanks!

I'm not sure why a linkonce_odr GV would be marked non-prevailing. Do you have a minimized example to look at?

Hi @vlad.tsyrklevich @twoh

I file this bug report here. I would like to fix this bug. Can you help me ? In the bug, we have a file (main.cpp) that gets compiled to ELF directly, and (unknown.cpp) that gets compiled to IR object file. At link stage, the vtable for class B in the ELF file is the prevailing one and the one in the IR object files does not get picked and linked into Composite by the IRMover. WMD expects the the vtable to be a definition with initializer (we have a declaration for it after IR linking).
As for the solution, I wonder whether we should/could change the linkage info in the GV summary information here since we changed the GV's linkage. https://github.com/llvm-mirror/llvm/blob/master/lib/LTO/LTO.cpp#L664

https://bugs.llvm.org/show_bug.cgi?id=38672

Thanks