Add proper handling for non-local references in nested functions

  On 07/26/2015 10:34 PM, Doug Evans wrote:
> Hi.
>
> Several nits and questions inline.  grep for ====.

Thank you for reviewing!

> One thing I still want to do is take this patch and run it through
> the perf testsuite.
>
> Also, I still need to look at follow_static_link, get_hosting_frame closer.

Okay.

> Cool stuff though, there's clearly missing functionality we need here.

Indeed, thanks! For the record, just like for the block_found business, 
I re-enabled Guile support in my builds and propagated the support for 
non-local references to the Guile API.

>> +/* Return a property to evaluate the static link associated to BLOCK.  Note
>> +   that only objfile-owned and function-level blocks can have a static link.
>> +   Return NULL if there is no such property.  */
>> +
>
> ====
> Add a comment here stating that the term "static_link" is derived from
> DW_AT_static_link.

Done. I also added one in objfiles.h, for struct objfile's static_links 
field.

>> -finish_block_internal (struct symbol *symbol, struct pending **listhead,
>> +finish_block_internal (struct symbol *symbol, struct dynamic_prop *static_link,
>> +		       struct pending **listhead,
>>   		       struct pending_block *old_blocks,
>
> ====
> Move the static_link property here.
> [Arguments to functions aren't in completely random order,
> and here static_link is among the collection of random things
> about the block like start,end. So it reads better to me if
> static_link appears with start,end]

Fine by me: done and callers updated.

>> +static CORE_ADDR
>> +block_op_get_frame_base (struct symbol *framefunc, struct frame_info *frame)
>> +{
>> +  if (SYMBOL_BLOCK_OPS (framefunc)->find_frame_base_location != NULL)
>> +    {
>> +      struct gdbarch *gdbarch = get_frame_arch (frame);
>> +      struct type *type = builtin_type (gdbarch)->builtin_data_ptr;
>> +      struct dwarf2_locexpr_baton *dlbaton = SYMBOL_LOCATION_BATON (framefunc);
>> +      const gdb_byte *start;
>> +      size_t length;
>> +      struct value *result;
>> +
>> +      SYMBOL_BLOCK_OPS (framefunc)->find_frame_base_location
>> +        (framefunc, get_frame_pc (frame), &start, &length);
>> +      result = dwarf2_evaluate_loc_desc (type, frame, start, length,
>> +					 dlbaton->per_cu);
>> +
>> +      /* The DW_AT_frame_base attribute contains a location description which
>> +	 computes the base address itself.  However, the call to
>> +	 dwarf2_evaluate_loc_desc returns a value representing a variable at
>> +	 that address.  The frame base address is thus this variable's
>> +	 address.  */
>> +      return value_address (result);
>> +    }
>> +  return 0;
>> +}
>
> ====
> If this is implemented on top of symbol_block_ops::find_frame_base_location,
> do we need another method? Or can we just have a wrapper that calls
> find_frame_base_location?
> For one, I see block_op_get_frame_base being used for both
> dwarf2_block_frame_base_locexpr_funcs and
> dwarf2_block_frame_base_loclist_funcs.

Correct me if I'm wrong, but the problem here is that if we aren't 
handling a "DWARF-described function", then we cannot assume that the 
find_frame_base_location method yields a location description. Mhm... 
maybe I should change it to return a dynamic property instead, then: 
what do you think?

>> +/* Given static link expression and the frame it lives in, look for the frame
>> +   the static links points to and return it.  Return NULL if we could not find
>> +   such a frame.   */
>> +
>> +static struct frame_info *
>> +follow_static_link (struct frame_info *frame,
>> +		    const struct dynamic_prop *static_link)
>> +{
>> +  CORE_ADDR upper_frame_base;
>> +
>> +  if (!dwarf2_evaluate_property (static_link, frame, NULL, &upper_frame_base))
>
> ====
> It gets harder and harder to reason about correctness the more
> we blur lines between foo-independent and foo-dependent parts of gdb.
> [In debug case "foo" == "debug-format".]

Agreed.

> I guess to begin with, why are we calling a dwarf-specific function here
> and what guarantees are in place (and easily discernable from reading
> the code!) that the right thing will happen for non-dwarf targets?

My interpretation is that even though dynamic properties seem to be used 
only with DWARF info., they should not be specific to it. In other 
words, dwarf2_evaluate_property should be called instead something like 
"evaluate_dynamic_prop" and moved elsewhere. After all, struct 
dynamic_prop isn't supposed to be itself DWARF-specific, and it is 
defined in gdbtypes.h.

>> +      /* Protect ourselves against bad things such as circular call stacks.  */
>
> ====
> Here's a good question.
> gdb has other mechanisms to catch corrupt (e.g., circular) stacks
> (e.g., UNWIND_INNER_ID). Is this QUIT here for protection or in case
> of really large stacks (e.g., due to infinite recursion)?
>
>> +      QUIT;

I wasn't aware of these mechanisms: thanks! Well, I had indeed circular 
stacks in mind, but this QUIT is also here to let users stop heavy 
computations in case of huge stacks (Joel advised me live to do this for 
this reason). So the comment is misleading and I have updated it. Thanks!

>> +      if (framefunc != NULL
>> +	  && SYMBOL_BLOCK_OPS (framefunc)->get_frame_base
>
> ====
> != NULL

Done.

>> +    /* Given a symbol VAR, the corresponding block VAR_BLOCK (if any) and a
>> +       stack frame id FRAME, read the value of the variable and return (pointer
>> +       to a) struct value containing the value.
>> +
>> +       VAR_BLOCK is needed if there's a possibility for VAR to be outside
>> +       FRAME.  This is what happens if FRAME correspond to a nested function
>> +       and VAR is defined in the outer function.  If callers know that VAR is
>> +       located in FRAME, NULL can be passed as VAR_BLOCK.
>
> ====
> "If callers know that VAR is located in FRAME or is global, ..." ?

Indeed. I added "or is global/static" instead.

>> +struct static_link_htab_entry
>> +{
>> +  const struct block *block;
>> +  struct dynamic_prop *static_link;
>
> ====
> It's too bad this isn't const struct dynamic_prop *static_link;
> I'm guessing it can't be that without fixing some laxness elsewhere
> (which I wouldn't impose on this patch), but can you check?

I thought it would indeed bring a lot of changes everywhere... but it's 
not actually! I could add const qualifiers everywhere on dynamic 
properties in objfiles.* without trouble at all and added a coulpe in 
buildsym.* as well. Thanks for suggesting this!

>> +static int
>> +static_link_htab_entry_eq (const void *p1, const void *p2)
>> +{
>> +  const struct static_link_htab_entry *entry
>> +    = (const struct static_link_htab_entry *) p1;
>> +  const struct block *block = (const struct block *) p2;
>
> ====
> blank line here

Done.

> Also, this is a non-standard implementation of an htab eq function.
> Generally both p1 and p2 point to an element in the hash table.
>
> I see hashtab.h has this:
>
> /* Compare a table entry with a possible entry.  The entry already in
>     the table always comes first, so the second element can be of a
>     different type (but in this case htab_find and htab_find_slot
>     cannot be used; instead the variants that accept a hash value
>     must be used).  */
> typedef int (*htab_eq) (const void *, const void *);
>
> so this eq function is possibly ok, except I also see calls to
> htab_find,htab_find_slot below. AIUC, one of these needs to change.

Yeah... I spent some time before having something that actually worked 
(first time I used this container) and got it wrong wrt. to doc anyway. 
Now, I eventually understood that all hash table lookups must be done 
with struct static_link_htab_entry * instead of struct block *, so I 
could fix the htab_eq function to be commutative.

>> +  if (entry == NULL)
>> +    return NULL;
>> +  else
>
> ====
> I don't know how others feel, but "else" clauses in particular situations
> like this are just noise. How about removing it?

Sure, removed.

>> +    struct htab *static_links;
>
> ====
> s/struct htab */htab_t /

Done.

>> +
>> +  /* Return the frame base address.  FRAME is the frame for which we want to
>> +     compute the base address while FRAMEFUNC is the symbol for the
>> +     corresponding function.
>> +
>> +     This method is designed to work with static links (nested functions
>> +     handling).  Static links are function properties whose evaluation return
>
> ====
> s/return/returns/

Fixed: thanks!

Updated patch is attached and tested again on x86_64-linux.

Add proper handling for non-local references in nested functions

Commit Message

Comments

Patch