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

Thu Feb 23 15:08:39 PST 2017

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.
--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<mailto:aprantl at apple.com>> on behalf of Adrian Prantl <aprantl at apple.com<mailto:aprantl at apple.com>>
Date: Thursday, February 23, 2017 at 10:52 AM
To: David Blaikie <dblaikie at gmail.com<mailto:dblaikie at gmail.com>>
Cc: "reviews+D29833+public+d0b46dd83e56e03f at reviews.llvm.org<mailto:reviews+D29833+public+d0b46dd83e56e03f at reviews.llvm.org>" <reviews+D29833+public+d0b46dd83e56e03f at reviews.llvm.org<mailto:reviews+D29833+public+d0b46dd83e56e03f at reviews.llvm.org>>, "danielcdh at gmail.com<mailto:danielcdh at gmail.com>" <danielcdh at gmail.com<mailto:danielcdh at gmail.com>>, "echristo at gmail.com<mailto:echristo at gmail.com>" <echristo at gmail.com<mailto:echristo at gmail.com>>, "rnk at google.com<mailto:rnk at google.com>" <rnk at google.com<mailto:rnk at google.com>>, "rob.lougher at gmail.com<mailto:rob.lougher at gmail.com>" <rob.lougher at gmail.com<mailto:rob.lougher at gmail.com>>, "paul.robinson at sony.com<mailto:paul.robinson at sony.com>" <paul.robinson at sony.com<mailto:paul.robinson at sony.com>>, Taewook Oh <twoh at fb.com<mailto:twoh at fb.com>>, "andrea.dibiagio at gmail.com<mailto:andrea.dibiagio at gmail.com>" <andrea.dibiagio at gmail.com<mailto:andrea.dibiagio at gmail.com>>, "llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>" <llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>>, Mehdi AMINI <mehdi.amini at apple.com<mailto: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<mailto:dblaikie at gmail.com>> wrote:

On Thu, Feb 23, 2017 at 9:35 AM Adrian Prantl <aprantl at apple.com<mailto:aprantl at apple.com>> wrote:
On Feb 23, 2017, at 9:13 AM, David Blaikie <dblaikie at gmail.com<mailto: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/6a59f0b8/attachment-0001.html>