[PATCH] D55153: [ThinLTO] Implement index-based WPD

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

A couple of immediate questions/ideas below:

In D55153#1982013 <https://reviews.llvm.org/D55153#1982013>, @saudi wrote:

> When switching from clang 9 to clang 10, we encountered a freeze in one of our games built with thin-LTO.
>  git bisect pointed to this commit.
>  Unfortunately, it is a pretty large project, and I couldn't repro that behavior in other -smaller- contexts.
>
> Behavior:
>  In a call to a virtual function, a direct call is made to an address containing a single instruction : a short jump to that same instruction, causing an infinite loop:
>
>   00007FF72C7DA80C 5B                   pop         rbx  
>   00007FF72C7DA80D 5F                   pop         rdi  
>   00007FF72C7DA80E 5E                   pop         rsi  
>   00007FF72C7DA80F C3                   ret  
>   --- [path_to_file.h]
>   00007FF72C7DA810 EB FE                jmp         00007FF72C7DA810      # <- Infinite loop here
>   --- No source file -------------------------------------------------------------
>   00007FF72C7DA812 CC                   int         3  
>   00007FF72C7DA813 CC                   int         3  
>   00007FF72C7DA814 CC                   int         3  
>
>
> I noticed, looking into the output map file, that several other methods of that same class had that same address.


Do you know what the original virtual call was, and whether the devirtualization seems legitimate given the class hierarchy?

> Our project is built clang-cl, with options `-flto=thin` and `-Xclang -fwhole-program-vtables`, andl links with pre-compiled libs that are built with MSVC. 
>  Without `-fwhole-program-vtables` the bug disappears.

Are you building with -fno-split-lto-unit? If not, you are getting hybrid regular/Thin LTO devirtualization, and this patch doesn't really apply. By default with -fwhole-program-vtables and -flto=thin, the LTO units are split into regular and thin portions, and all vtables are in the regular portion. This patch simply extended WPD to work without splitting, hence ThinLTO "index only" - i.e. no regular LTO and no IR, and only kicks in with -fno-split-lto-unit on top of the other options. The original hybrid ThinLTO WPD was implemented by @pcc, who may have some ideas as well.

> Notes:
> 
> - We tried applying the patch from D77421 <https://reviews.llvm.org/D77421>, without any luck.
> - Our project links with pre-compiled libs (that were built without LTO). Would `-fwhole-program-vtables` be invalid to use in our case?

It depends. Are you also compiling with -fvisibility=hidden? I believe -fwhole-program-vtables should be safe on its own, but often is combined with -fvisibility=hidden to maximize the devirtualization candidates. Setting the visibility to hidden can be dangerous if you don't have full whole program scope for LTO. I.e. if any of those pre-compiled libs inherits from the classes in the LTO objects.

>   - The bug also occurs on the master branch.
>   - I tried with an assert-enabled build, no asserts were raised
>   - The class hierarchy has several virtual methods inlined; the others are built without LTO in a separate library.
> 
>     At this point, I would like to create a simple repro, but I'm running out of ideas, as I'm not very familiar with LTO detail. Our repro involves a pretty big compilation (resulting .exe file is about 100MB, link time ~8 min).
> 
>     I would be interested in suggestions about how to extract helpful information, for example:
> - specific command line arguments to `clang-cl.exe` or `lld-link.exe` that would give details about devirtualization;

Try enabling pass remarks in the LTO pipeline. For lld this would be -Wl,-mllvm,-pass-remarks=wholeprogramdevirt

> - a significant place to put a breakpoint in lld's code;

This sort of depends on what you are looking for. I would start with the pass remarks and figure out what is the bad devirtualization. The WPD code is in llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp.

> - command lines to run on the thin-lto generated object files;
> - ... ?
> 
>   Thanks!




Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55153/new/

https://reviews.llvm.org/D55153