[PATCH] Do not attach a debug location to code inserted by ARC / API for disabling DebugLocations

[Adrian Prantl]

Hi Eric!

On Apr 29, 2013, at 6:10 AM, Eric Christopher <echristo at gmail.com> wrote:

> Looks like it's working around the return value load/alloca eliding in
> the code generator? I definitely like a more general way of
> approaching the problem.

It’s possible that I misunderstood you (let me know) but I don’t think that’s an accurate description. Here’s how I would describe it:

If there is 
1) only a single, simple return stmt in the function,
2) and cleanup code,
3) prevent the the ret instruction from getting a debug location that is “above” the debug location of the clean code. If there is no cleanup code, however, we still want the return instruction to get the debug location from the elided alloca.

I agree that it looks like a rather specialized solution, but it’s also a rather special problem.

> I wonder if we can either localize this fix into EmitFunctionEpilog
> (couldn't come up with one) or if this has applicability in the > 1
> return statement condition. Open questions, this is a bit of a finicky
> and complicated area as you've no doubt realized. :)

No doubt :-)

I believe that we cannot localize the patch into EmitFunctionEpilog: If there is cleanup code, we (I?) want the cleanup code to get the line number of the return statement, because it will be the first breakpoint opportunity in the function, and we must keep it, because the user might want to inspect the value of arguments that will get cleaned up. We can only do this in FinishFunction.

In the case of multiple returns the problem with the cleanup code being the first breakpoint in the function should not exist, because there should be some other statement with a debug location before any return statement.

> One other comment on the patch:
> 
> +  if (CGDebugInfo *DI = getDebugInfo()) {
>     DI->EmitLocation(Builder, EndLoc);
> +    EndLocL = Builder.getCurrentDebugLocation();
> +    if ((NStopPoints == 1) && !FnRetTy->isVoidType()) {
> +      DI->EmitLocation(Builder, FirstStopPoint);
> +      EmitRetDbgLoc = false;
> 
> Do we need the two locations emitted here?

We don’t, of course :-)

> or the check for VoidType?

Removed together with RetDbgLoc.

I took the opportunity to cleanup and document the patch some more. It now accurately detects whether the single return expression could be evaluated to a constant expression. I also made the test case more comprehensive.

thanks,
Adrian

PS: To help visualizing the effects, I attached the output of a script that interleaves the source lines and the disassembler output based in the line table. (I ran it on the testcase).




-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Ensure-that-the-line-table-for-functions-with-cleanu.patch
Type: application/octet-stream
Size: 11626 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130429/f92bf6f2/attachment.obj>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: dwarf-linetable-debug.txt
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130429/f92bf6f2/attachment.txt>