[PATCH] D29833: Improve the API of DILocation::getMergedLocation()

[David Blaikie via llvm-commits]

On Thu, Feb 23, 2017 at 9:35 AM Adrian Prantl <aprantl at apple.com> wrote:

> On Feb 23, 2017, at 9:13 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
> You were suggesting that the nice thing about the former was that the user
> couldn't fail to have a debug location:
>
> "The primary benefit of having the API on Instruction is that we can make
> it impossible to accidentally create a locationless call instruction in a
> function with debuginfo (which would crash/assert the backend when creating
> inline scopes)."
>
> Which I'm a bit fuzzy on - if a new call instruction is created it'd still
> have no location so I'm not sure this choice of API provides extra defense
> against that mistake.
>
> If, today, an existing call instruction is moved then the location zero'd
> out - it's going to need work to replace the zeroing with merging no matter
> whether the API for that merging is on DebugLoc or Instruction.
>
>
> Agreed.
>
>
> So I guess coming back to your point: could you explain in more detail how
> having the API on Instruction provides a stronger guarantee/safety than
> having it elsewhere? I'm not clear on what that's defending against.
>
>
> I want to make it harder to accidentally assign a null DebugLoc to a call.
> Currently getMergedDebugLoc() returns a null location (to save space in the
> line table) and at each call site we have to write extra code to create a
> line:0 location if the instruction we are creating the merged location for
> is a call. See https://reviews.llvm.org/D29833, lib/Transforms/Utils/SimplifyCFG.cpp:1280
> for an example — though upon closer inspection the original code was just
> keeping the original location of the call instead of assigning a line:0
> location.
> I though it would be nice to hide this detail in the API, but this really
> makes it look like we want to decide how to handle call instructions
> (assign line:0, keep original loc) on a case-by-case basis.
>

Oh, right, sorry - sure, seems reasonable to make it part of Instruction so
it can Do The Right Thing depending on whether the Instruction is a Call.
(maybe a "setMergedLocation(DebugLoc, DebugLoc)" that ignores the current
location - I suppose in theory it could be a variadic function to handle
more than two locations, but that probably doesn't come up often enough to
worry? Not sure))

You mentioned abandoning this in favor of the other change under review?
Seems the discussion is here so we might as well continue it here.

- Dave


>
> thanks,
> adrian
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170223/4b27f896/attachment-0001.html>