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

Thu Feb 23 15:16:56 PST 2017

On Thu, Feb 23, 2017 at 3:08 PM Robinson, Paul <paul.robinson at sony.com>
wrote:

> For DWARF, all "line 0" records are semantically equivalent.  However
> there is a size cost in the encoded line table if an explicit "line 0" has
> a different file (or column) than the previous or following line record.
>

Yep, also a size cost if we encode it in a different scope from teh one
nearby - could cause scopes to have holes and thus need DW_AT_ranges,
etc...

> --paulr
>
>
>
> *From:* Taewook Oh [mailto:twoh at fb.com]
> *Sent:* Thursday, February 23, 2017 12:02 PM
> *To:* Adrian Prantl; David Blaikie
> *Cc:* reviews+D29833+public+d0b46dd83e56e03f at reviews.llvm.org;
> danielcdh at gmail.com; echristo at gmail.com; rnk at google.com;
> rob.lougher at gmail.com; Robinson, Paul; andrea.dibiagio at gmail.com;
> llvm-commits at lists.llvm.org; mehdi.amini at apple.com
>
>
> *Subject:* Re: [PATCH] D29833: Improve the API of
> DILocation::getMergedLocation()
>
>
>
> I think the existing implementation of getMergedLocation implies that if
> two debug locations can be discriminated, we should return line:0 location
> than keep the original location. The gist of this patch is returning line:0
> locations instead of nullptr for call instructions, I believe.
>
>
>
> Regarding API, I think “I1->setMergedDebugLoc(I1->getDebugLoc(),
> I2->getDebugLoc()” is better than
> “I1->setDebugLoc(DILocation::getMergedLocation(I1->getDebugLoc(),
> I2->getDebugLoc()))”, because it is more succinct, and we can handle
> CallInst separately.
>
>
>
> Another issue we’ve been discussed was what should be the “scope” of
> returning line:0 location, but I actually don’t have strong opinion about
> it, because with line:0 it is already clear that the DebugLoc is dummy
> (Maybe I missed some point here).
>
>
>
> Thanks,
>
> Taewook
>
>
>
> *From: *<aprantl at apple.com> on behalf of Adrian Prantl <aprantl at apple.com>
> *Date: *Thursday, February 23, 2017 at 10:52 AM
> *To: *David Blaikie <dblaikie at gmail.com>
> *Cc: *"reviews+D29833+public+d0b46dd83e56e03f at reviews.llvm.org" <
> reviews+D29833+public+d0b46dd83e56e03f at reviews.llvm.org>, "
> danielcdh at gmail.com" <danielcdh at gmail.com>, "echristo at gmail.com" <
> echristo at gmail.com>, "rnk at google.com" <rnk at google.com>, "
> rob.lougher at gmail.com" <rob.lougher at gmail.com>, "paul.robinson at sony.com" <
> paul.robinson at sony.com>, Taewook Oh <twoh at fb.com>, "
> andrea.dibiagio at gmail.com" <andrea.dibiagio at gmail.com>, "
> llvm-commits at lists.llvm.org" <llvm-commits at lists.llvm.org>, Mehdi AMINI <
> mehdi.amini at apple.com>
> *Subject: *Re: [PATCH] D29833: Improve the API of
> DILocation::getMergedLocation()
>
>
>
>
>
> 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
> <https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_D29833&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=kOsLCgQzH7N8ptZ7diJD9g&m=O6D-OeqGm2sNUSI30Qqz7sFjmurpqgcX9CVy6OgkNGw&s=_XNi_qHqSnfqrjV9rLI5uU6TiIsFhrAykuq40uy9pJQ&e=>, 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.
>
>
>
> 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/2aac2c4e/attachment.html>