[PATCH] D53890: [LTO] Record whether LTOUnit splitting is enabled in index

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D53890#1336551 <https://reviews.llvm.org/D53890#1336551>, @pcc wrote:

> In D53890#1336521 <https://reviews.llvm.org/D53890#1336521>, @tejohnson wrote:
>
> > In D53890#1336239 <https://reviews.llvm.org/D53890#1336239>, @dexonsmith wrote:
> >
> > > In D53890#1335770 <https://reviews.llvm.org/D53890#1335770>, @pcc wrote:
> > >
> > > > This means that
> > > >
> > > >   clang -c -flto=thin foo.c
> > > >   [upgrade clang]
> > > >   clang -c -flto=thin bar.c
> > > >   clang foo.o bar.o
> > > >
> > > >
> > > > will fail unless you add `-fsplit-lto-unit` to the second invocation. Given that the alternative is that we potentially silently miscompile, maybe that's fine. It would be possible to handle this case without aborting or miscompiling by splitting `bar.o` inside the linker and treating it as if it had always been split,
> > >
> >
> >
> > In a distributed build context you'd also need to record something in the index to get the backend clang invocation to also split bar.o. I'm not sure it is worth doing all this.
> >
> > > but given that there's a workaround maybe that's not that critical.
> > > 
> > > I would have expected this to link successfully, but disable any optimizations that we don’t have enough information to perform safely.
>
>
> Yes, that sounds a lot better than my suggestion.
>
> > From what I understand, this wouldn't just be disabling the LTO link time whole program optimizations, but also flagging it in the index and doing some lowering in the backend of type tests (@pcc is that correct?).
>
> The way that I see this working is that if we see a mix of old and new bitcode (or a mix of split and non-split) we would need to set a flag in the index that would cause WholeProgramDevirt to be disabled, and would also cause LowerTypeTests to error out if there are any `llvm.type.test`s to be lowered. Given that the error behaviour would be restricted to users of `-fsanitize=cfi` (which is not a commonly used feature) and that you already need to split in order to use CFI, this seems quite reasonable to me.


Right - I forgot that we shouldn't have any type tests to lower in this case, so it becomes pretty straightforward. Since CFI causes splitting to continue to be enabled (in D53891 <https://reviews.llvm.org/D53891>), we shouldn't have any issues.

> 
> 
>> Is it unreasonable to expect that the user can simply add the flag indicated by the error? The downside of that though is that users may just add the flag to their build files and unnecessarily get splitting from there on out.
> 
> What do you think about the behaviour that I've described above? It seems relatively straightforward to implement to me, but I might be missing something.

SGTM, thanks


Repository:
  rL LLVM

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

https://reviews.llvm.org/D53890