[PATCH] Consider the frame base isn't moving in calculateDbgValueHistory.

[Frédéric Riss]

> On 19 Sep 2014, at 02:18, Alexey Samsonov <vonosmas at gmail.com> wrote:
> 
> 
> On Thu, Sep 18, 2014 at 2:02 AM, Frederic Riss <friss at apple.com <mailto:friss at apple.com>> wrote:
> >>! In D5366#10, @samsonov wrote:
> > Why is RSP clobbered by the calls in the first place? If $rsp is used to address local variables, then does it mean the generated code actually has to spill $rsp before calling a function and restore $rsp after that? That looks weird.
> 
> I should have mentioned this is the initial message, but I actually investigated that out a few weeks ago: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140811/230791.html <http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140811/230791.html>
> The consensus seemed to be that we need to deal with it. From a purely theoretical POV, it is true that a call will write to the stack pointer, thus the annotation isn't wrong. I'm proposing a way to just deal with it.
> 
> I see the following note by Jakub in that email thread: "<....> As Tim pointed out, some calling conventions do actually modify the stack pointer. But even when they don't, it is normal to have instructions around a call that do modify the stack pointer to set up a call frame.  The PEI pass knows where to find stack variables even when the stack pointer is moving, and it seems to me that debug value tracking should do the same. We also have two stack slot coloring passes that know exactly where stack variables are live. "
> 
> I think that if call instruction is said to clobber %rsp, we should deal with it and not hack around it. If there are passes which track the location of local variables, then we can use a similar pass to update DBG_VALUE instructions in the machine function as needed.

I think we all understand that the proposed patch is a workaround for an issue in something that already looks like a hack. And adding hacks to hacks is not in general desirable.

I however did my homework before proposing this patch, and I of course looked at the stack liveness tracking code that is in tree. I looked at PEI and the 2 stack colouring passes and they didn’t seem relevant to the problem at hand. I could give more details about why I think so, but the main common denominator of these passes is that they run at a time where high level abstractions are still represented in the IR. The most important of these abstractions being the FrameIndex MachineOperands.

When calculateDbgValueHistory runs, all of this has disappeared. We only have registers and offsets. No high level abstraction to help us determine where a variable is written/read/killed. The only kinda high level abstraction that we have at this point are the DBG_VALUE instructions. I think that this function has to be somewhat hackish to be able to do his job (and it certainly already is).

Now let’s try to look at alternate solutions. I am truly interested in fixing this the right way, and I spent quite a bit of time thinking about this issue. I am proposing this patch only as an incremental improvement because it looks like a real solution is pretty far away.

As you say, we should try to insert more DBG_VALUE instructions to ease calculateDbgValueHistory’s job. 
First, if we want to do a real liveness analysis on DBG_VALUEs, it has to be at a high level. Once we have only registers, the %rsp issue prevents any accurate analysis. This shows a first issue: whatever the result at this time of the compilation flow, it might be modified by further optimisations. But let's not consider that for the moment.
If we have the global liveness problem solved at a high level, where should we put additional DBG_VALUE instructions? I think the most natural way to move forward would be at the beginning of all basic blocks where the described value is LiveIn. And then calculateDbgValueHistory would be able to do his basic-block local analysis. Except that you still hit the clobbered %rsp issue and would need the same kind of workaround (for the %rsp issue, the global set of changing regs could go away).

Another solution can think of is doing this liveness analysis at a high-level and inserting DBG_VALUEs that represent start of live ranges, but also something that represents the end of live-ranges. This way there is no guessing to do. This has however the drawback of requiring all the transformations done between the liveness annotation and the end of the flow to accurately maintain the annotations. I’d consider this just at the sweet spot between very hard and impossible. I’m just speaking out of prior experience, I don’t know much about LLVM’s backend, and I’d be happy to be proven wrong.


> > I considered adding a liveness analysis here, but it seems that DbgValueHistoryCalculator is a wrong place for this: it's pain to match the propagation of debug variables' locations in the control flow graph with the order the actual basic blocks are emitted in the object file (as this is the order we care about when we generate .debug_loc section). It is doable, but painful and can be pretty slow. Instead, it would be better to somehow reuse/fix LiveDebugVariables pass (which is currently used only by optimized register allocator), and track locations by inserting DBG_VALUEs where necessary.
> 
> And if we decided to do it on DbgValueHistoryCalculator, the RSP issue would still be there. Every liveness analysis relies on def/kill information.

LiveDebugVariables is very nice, but it wouldn’t solve anything with respect to the clobber of %rsp problem, because it would just emit start of liveness annotations at register allocation time. And then calculateDbgValueHistory would truncate the liveness at the first call. Maybe we could have the pass add DBG_VALUEs after calls to ‘reboot’ the liverange, but that also look like a hack.

Anyone can think of a ‘real’ solution? In the mean time the patch is still on the table for the people who want to debug ASAN instrumented code at -O0.

Fred

> http://reviews.llvm.org/D5366 <http://reviews.llvm.org/D5366>
> 
> 
> 
> -- 
> Alexey Samsonov
> vonosmas at gmail.com <mailto:vonosmas at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140919/d1836932/attachment.html>