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

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

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`?

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?

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?

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.



================
Comment at: include/lldb/Symbol/SymbolFile.h:254
+  /// variables and spilled registers, but it should not include paramenters, as
+  /// they are considered to be a part of the callers frame.
+  virtual llvm::Expected<lldb::addr_t> GetOwnFrameSize(Symbol &symbol) {
----------------
Typos:

paramenters -> parameters
callers -> caller's



Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66638