[llvm-dev] [RFC] Setting the current debug loc when the insertion point changes

[Davide Italiano via llvm-dev]

On Mon, Nov 6, 2017 at 2:19 PM, Vedant Kumar <vsk at apple.com> wrote:
> Hi everybody,
>
> I'm investigating some correctness issues with debug line table information
> in optimized code. I've noticed something problematic in IRBuilder. Setting
> the insertion point of an IRBuilder sets the builder’s current debug loc to
> the one attached to the insertion point. IMO this isn't always a correct or
> convenient thing to do, and it shouldn't be the default behavior.
>
> # Correctness
>
> The IRBuilder shouldn't assume that the debug loc at its insertion point
> applies to any instructions it emits. It doesn't have enough information to
> make that connection. This assumption leads to bugs like llvm.org/PR25630.
> I.e it creates situations in which we silently push around incorrect
> DILocations without realizing it, because the debug locs in the IR look nice
> and contiguous.
>

I remember that one, and I got to your same conclusion at the time
when analyzing the bug, so thanks for writing this down :)

> To fix the issue, clients of IRBuilder should be required to either:
>
> 1. Explicitly set the correct debug location before inserting instructions,
> or
> 2. Opt-in to the current behavior of propagating the debug loc from the
> insertion point.
>
> # Convenience
>
> The current behavior of IRBuilder can be convenient when it's correct. But
> it can be *really* inconvenient when it's incorrect.
>
> If a client sets an IRBuilder's debug loc correctly and makes a call that
> changes the builder's insertion point, all generated instructions will pick
> up a bogus debug loc from the IP. E.g this is what happens with
> SCEVExpander::expandCodeFor(). There's no way to fix this bug without
> changing IRBuilder's behavior.
>
> # Solutions
>
> 1. Add a method called SetInsertPointAndDebugLoc() which behaves identically
> to the current SetInsertPoint(). Change SetInsertPoint() so that it doesn't
> change the current debug loc.
> 2. Add a boolean argument to SetInsertPoint() called UpdateDbgLoc which
> defaults to false. Only change the current debug loc if UpdateDbgLoc is
> true.
>
> I have a mild preference for idea #1 because it's easier to read, IMO.
>
> Whichever solution we pick, my plan is to first migrate all in-tree users of
> SetInsertPoint() to the explicit API, and to then gradually fix passes which
> rely on the current incorrect behavior.
>

My preference is as well for 1. I kind of dislike adding bool
parameters to API as it becomes easier to get them wrong.
Your plan makes sense to me, regardless. I'll be happy to review the patches.

Thanks!

--
Davide