[llvm] r311229 - [Inliner] Fix a nasty bug when inlining a non-recursive trace of

[Chandler Carruth via llvm-commits]

Probably worth it. IIRC, this results in miscompiling some old versions of
mplayer so it has actually been seen in the wild.

+Hans Wennborg <hans at chromium.org>

On Mon, Aug 21, 2017 at 11:34 AM Matthias Braun via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> Would this be worthy of llvm-5.0 or is it too late/risky/uncommon a case?
>
> - Matthias
>
> > On Aug 18, 2017, at 11:56 PM, Chandler Carruth via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
> >
> > Author: chandlerc
> > Date: Fri Aug 18 23:56:11 2017
> > New Revision: 311229
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=311229&view=rev
> > Log:
> > [Inliner] Fix a nasty bug when inlining a non-recursive trace of
> > a function into itself.
> >
> > We tried to fix this before in r306495 but that got reverted as the
> > assert was actually hit.
> >
> > This fixes the original bug (which we seem to have lost track of with
> > the revert) by blocking a second remapping when the function being
> > inlined is also the caller and the remapping could succeed but
> > erroneously.
> >
> > The included test case would actually load from an inlined copy of the
> > alloca before this change, failing to load the stored value and
> > miscompiling.
> >
> > Many thanks to Richard Smith for diagnosing a user miscompile to this
> > bug, and to Kyle for the first attempt and initial analysis and David Li
> > for remembering the issue and how to fix it and suggesting the patch.
> > I'm just stitching it together and landing it. =]
> >
> > Modified:
> >    llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp
> >    llvm/trunk/test/Transforms/Inline/recursive.ll
> >
> > Modified: llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp?rev=311229&r1=311228&r2=311229&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp (original)
> > +++ llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp Fri Aug 18
> 23:56:11 2017
> > @@ -341,8 +341,9 @@ void PruningFunctionCloner::CloneBlock(c
> >               SimplifyInstruction(NewInst,
> BB->getModule()->getDataLayout())) {
> >         // On the off-chance that this simplifies to an instruction in
> the old
> >         // function, map it back into the new function.
> > -        if (Value *MappedV = VMap.lookup(V))
> > -          V = MappedV;
> > +        if (NewFunc != OldFunc)
> > +          if (Value *MappedV = VMap.lookup(V))
> > +            V = MappedV;
> >
> >         if (!NewInst->mayHaveSideEffects()) {
> >           VMap[&*II] = V;
> >
> > Modified: llvm/trunk/test/Transforms/Inline/recursive.ll
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Inline/recursive.ll?rev=311229&r1=311228&r2=311229&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/test/Transforms/Inline/recursive.ll (original)
> > +++ llvm/trunk/test/Transforms/Inline/recursive.ll Fri Aug 18 23:56:11
> 2017
> > @@ -40,3 +40,34 @@ declare void @bar(i8* %in)
> >
> > declare i32 @foo(i32 %param)
> >
> > +; Check that when inlining a non-recursive path into a function's own
> body that
> > +; we get the re-mapping of instructions correct.
> > +define i32 @test_recursive_inlining_remapping(i1 %init, i8* %addr) {
> > +; CHECK-LABEL: define i32 @test_recursive_inlining_remapping(
> > +bb:
> > +  %n = alloca i32
> > +  br i1 %init, label %store, label %load
> > +; CHECK-NOT:     alloca
> > +;
> > +; CHECK:         %[[N:.*]] = alloca i32
> > +; CHECK-NEXT:    br i1 %init,
> > +
> > +store:
> > +  store i32 0, i32* %n
> > +  %cast = bitcast i32* %n to i8*
> > +  %v = call i32 @test_recursive_inlining_remapping(i1 false, i8* %cast)
> > +  ret i32 %v
> > +; CHECK-NOT:     call
> > +;
> > +; CHECK:         store i32 0, i32* %[[N]]
> > +; CHECK-NEXT:    %[[CAST:.*]] = bitcast i32* %[[N]] to i8*
> > +; CHECK-NEXT:    %[[INLINED_LOAD:.*]] = load i32, i32* %[[N]]
> > +; CHECK-NEXT:    ret i32 %[[INLINED_LOAD]]
> > +;
> > +; CHECK-NOT:     call
> > +
> > +load:
> > +  %castback = bitcast i8* %addr to i32*
> > +  %n.load = load i32, i32* %castback
> > +  ret i32 %n.load
> > +}
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170821/5bd072c1/attachment.html>