[Lldb-commits] [PATCH] D66638: Unwind: Add a stack scanning mechanism to support win32 unwinding

Fri Sep 6 04:19:03 PDT 2019

labath added a comment.

Thanks for the quick reply. My responses are inline:

In D66638#1659866 <https://reviews.llvm.org/D66638#1659866>, @amccarth wrote:

> I don't have any specific code comments, but I do have a couple general questions and points to consider.
>
> 1. `isFoundHeuristically` is very generic.  It's true that it's a heuristic approach, but it's a very specific heuristic.  Might there be other heuristic approaches in the future?  Should we name it something more specific like `isRaSearch`?

I wanted to avoid a specific name, because I though this approach might be useful elsewhere too. For instance, I can imagine doing the same kind of search as some sort of a last ditch attempt when we lack *any* kind of unwind information. However, thinking about it now, when I ignore the fact that the pdb construct is called rasearch, the name still pretty well describes what this "heuristic" does, so it is probably better to name it that way.

> 
> 
> 2. `max_iterations` means how many stack positions the heuristic will scan before giving up, right?  Are there any alignment issues here?  Should we assert that the return address hint is a multiple of the stack alignment?

I don't believe we're doing anything like that elsewhere in the unwinder, and x86 is pretty forgiving about memory access alignment, so if through some mishap, the function does end up with a misaligned stack, there is actually a good chance it would still work, and so we should be able to unwind from it.

> 
> 
> 3. The 100 for `max_iterations` is probably fine, but I wonder if there's a way to determine a more specific limit without just guessing.  What things could be on the stack between the hint and the actual return address?  It seems like only arguments for a call that the current function is preparing to make.  The standard says that the actual number of parameters is implementation-defined, but that it suggests a minimum of 256.  Should `max_iterations` be 256?  Is there much risk in making it bigger than it needs to be?

I don't think there's much risk in making this bigger. If everything goes according to plan, then we should get the return address only after a couple of iterations (most likely 1). Even if something goes wrong, then we will probably still return before completing all iterations, except that will find the return address a couple of frames higher (which is probably still better than giving no return address at all). So yeah, if the standard (which standard is that? C?) says 256, then that's probably the best number we can go with.

It's hard to say what can be on the stack except the function arguments. In theory it can be anything. The compiler could push any number of intermediate values to stash some temporary values. However, current compilers tend to avoid that, and instead prefer to allocate the maximum number of stack slots in the prologue.

That said, this made me realize that the win32 unwind format does allow one to express that the amount of stack taken up by the temporaries will be different at different points within the function. However, the API I am introducing here do not. It would probably be better to pass in an address in this case, so that one can return different values for different points within the function.

> 4. Is checking for executable permission slow?  Would it be worth doing some culling or caching?  I imagine a lot of non-return address values on the stack will be simple small numbers, like 0 or 1, which, for Windows, would never be a valid executable address.

The check should be pretty fast. The way it's implemented now, it only consults lldb's view of the process, which is all readily available (the loaded sections are in the target's section load list, and the executable bit is in the section object). I have considered going through the "memory region info" infrastructure, which consults the OS to get a list of mapped pages. That would be a bit slower (though this information is also cached), but it would have the advantage of detecting executable code which does not belong to any (known) module. It wasn't clear to me whether that is worth the trouble though..

Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66638/new/

https://reviews.llvm.org/D66638