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

[David Blaikie via llvm-commits]

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

> On Feb 23, 2017, at 10:08 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> 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))
>
>
> Yes, but then I realized that it is not obvious what The Right Thing is.
> When the instruction is a call, should we return a line:0 location, should
> we keep the original location? Is there one answer that fits all use-cases?
> I’m afraid now that we would be hiding the fact that the user needs to do
> make an informed decision here behind the new API.
>

When merging there's no option to keep the original (due to profile
requirements, at the very least (and arguably debug quality - but that's
harder to be sure of)). I believe the zero location (does DWARF say that
the file is still relevant when the line is zero? I hope not).

Scope is still unclear, to be sure. Really a null scope would be great (so
it'd just be whatever scope was nearby - like having no location works
today), but that might take some working.


>
> 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.
>
>
> Sure, works for me.
> -- adrian
>
> - Dave
>
>
>
> thanks,
> adrian
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170223/22f2cec1/attachment.html>