Page MenuHomePhabricator

Fix crash in bounds checking
ClosedPublic

Authored by jgalenson on Jul 27 2018, 3:53 PM.

Details

Summary

In r337830 I added SCEV checks to enable us to insert fewer bounds checks. Unfortunately, this sometimes crashes when multiple bounds checks are added due to SCEV caching issues. This patch splits the bounds checking pass into two phases, one that computes all the conditions (using SCEV checks) and the other that adds the new instructions.

Diff Detail

Event Timeline

jgalenson created this revision.Jul 27 2018, 3:53 PM
eugenis added inline comments.Jul 27 2018, 4:52 PM
lib/Transforms/Instrumentation/BoundsChecking.cpp
176

Move GetTrapBB definition near the end of the function, where it is first used.

197–198

It's very uncommon to store builders in containers. Replace with an instruction pointer.

TrapInfo can be combined with WorkList, and both loops can be merged in one.

Ka-Ka added a comment.Jul 30 2018, 3:25 AM

I found another reproducer for the problem. This one is slightly different where a different assert blow, but I guess its only a different symptom for the same fault. As this one is slightly smaller you might want to use this one as a testcase instead (or use both)? Run with "opt -bounds-checking -S".

@d = dso_local local_unnamed_addr global [1 x i16] zeroinitializer, align 1
@e = dso_local local_unnamed_addr global [1 x i16] zeroinitializer, align 1

define dso_local void @test2() local_unnamed_addr {
entry:

br label %while.cond1.preheader

while.cond1.preheader:

%0 = phi i16 [ undef, %entry ], [ %inc, %while.end ]
%1 = load i16, i16* undef, align 1
br label %while.end

while.end:

%inc = add nsw i16 %0, 1
%arrayidx = getelementptr inbounds [1 x i16], [1 x i16]* @e, i16 0, i16 %0
%2 = load i16, i16* %arrayidx, align 1
br i1 false, label %while.end6, label %while.cond1.preheader

while.end6:

ret void

}

jgalenson updated this revision to Diff 157976.Jul 30 2018, 8:48 AM
jgalenson marked an inline comment as done.

I found another reproducer for the problem. This one is slightly different where a different assert blow, but I guess its only a different symptom for the same fault. As this one is slightly smaller you might want to use this one as a testcase instead (or use both)?

Thanks! I'll use both it and the first one you gave me. Luckily, it doesn't crash with this patch.

jgalenson added inline comments.Jul 30 2018, 8:48 AM
lib/Transforms/Instrumentation/BoundsChecking.cpp
197–198

How did you want me to combine TrapInfo with WorkList? To insert the traps, I need both the original instruction (to create the builder) and the newly-created Or. I could change Worklist to hold a tuple with an optional Or, but that seems more complicated than using two lists like this.

eugenis added inline comments.Jul 30 2018, 12:56 PM
lib/Transforms/Instrumentation/BoundsChecking.cpp
173

The loop over worklist can be replaced with a loop over instructions(F).

It would work because we no longer create new basic blocks in the loop, and new instructions are added before the current iterator position, so they will not be observed in the following iterations.

jgalenson updated this revision to Diff 158057.Jul 30 2018, 1:20 PM
jgalenson added inline comments.
lib/Transforms/Instrumentation/BoundsChecking.cpp
173

Ah, you meant combine this loop with the previous one, not the new one I added below? Sorry, I misunderstood. That makes sense.

eugenis accepted this revision.Aug 3 2018, 9:50 AM

LGTM, thanks!

Sorry for the delay, I did not notice that you've updated the patch with your last comment.

This revision is now accepted and ready to land.Aug 3 2018, 9:50 AM
This revision was automatically updated to reflect the committed changes.

No problem, and thanks for the review!