This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v5] Make chained function calls in expressions work


Siva Chandra wrote:

> Link to v4: https://sourceware.org/ml/gdb-patches/2014-10/msg00697.html
> 
> I have made all the changes suggested by Ulrich Weigand in his comments for v4.

Thanks!  It looks nearly ready to commit now.  I still have a couple of
comments, but they're mostly cosmetic at this point ...  (Except for the
stack alignment issue, which is an actual bug.)

>  evaluate_subexp (struct type *expect_type, struct expression *exp,
>  		 int *pos, enum noside noside)
>  {
> -  return (*exp->language_defn->la_exp_desc->evaluate_exp) 
> +  struct cleanup *cleanups;
> +  struct value *retval;
> +  int cleanup_temps = 0;
> +
> +  if (*pos == 0 && inferior_ptid.pid != 0

We should avoid directly accessing subfields of ptid_t.  What is the
purpose of the .pid != 0 test here?  Is this simply to check whether
we can actually execute inferior calls?  In that case, it would be
preferable to check "target_has_execution", just as is done in
call_function_by_hand.  (This should end up doing basically the same
test, but properly abstracted.)

> +      && exp->language_defn->la_language == language_cplus)

I like the language check, this addresses one of my concerns about not
adding extra overhead when debugging plain C.  In the future, certain
other languages (Ada?) might also want to take advantage of the stack
temporary feature, in which case we might want to make this a property
of the language vector, instead of the explicit language_cplus check.
But that can wait until we actually have a second instance ...

> +/* Reads a value returned by an inferior function for those return
> +   values whose address is passed as the hidden first argument.
> +   TYPE is the type of value.  ADDR is the address from where to read it.
> +   If STACK_TEMPORARIES is non-zero, then the returned value will be on
> +   lval_memory type and will be stored as a temporary on the thread's 
> +   stack.  */
> +
> +static struct value *
> +get_return_value_from_memory (struct type *type, CORE_ADDR addr,
> +			      int stack_temporaries)
> +{
> +  struct value *retval;
> +  if (stack_temporaries)
> +    {
> +      retval = value_from_contents_and_address (type, NULL, addr);
> +      push_thread_stack_temporary (inferior_ptid, retval);
> +    }
> +  else
> +    {
> +      retval = allocate_value (type);
> +      read_value_memory (retval, 0, 1, addr, value_contents_raw (retval),
> +			 TYPE_LENGTH (type));
> +    }
> +
> +  return retval;
> +}

Due the the restructuring of the return value handling logic you implemented
in the patch, there is now only a single use of this routine.  Maybe it would
make sense to just inline the code at the call site to keep the overall logic
closer together  ...

> +    /* Skip over the stack temporaries that might have been generated during
> +       the evaluation of an expression.  */
> +    if (stack_temporaries)
> +      {
> +	if (gdbarch_inner_than (gdbarch, 1, 2))
> +	  sp = skip_thread_stack_temporaries (inferior_ptid, sp, 1);
> +	else
> +	  sp = skip_thread_stack_temporaries (inferior_ptid, sp, 0);
> +      }

As mentioned in one of the previous review mails, you now need to verify
that the new SP is still correctly aligned (and re-align if necessary).

The split of responsibilities between infcall.c and thread.c seems a bit
suboptimal; the skip_thread_stack_temporaries routine makes a lot of
assumptions that are only true when called from just this particular place.

Maybe it would be better to have a function in thread.c that simply returns
a range (low .. high) of addresses spanned by temporaries in the current
thread, and then have infcall.c use this range to implement the skipping,
taking into account gdbarch_inner_than, alignment rules, etc.


> +/* A simple struct to pass ptid data to a cleanup function.  */
> +
> +struct ptid_data
> +{
> +  ptid_t ptid;
> +};

I don't understand why this struct is needed ...  Can't you just
allocate a ptid_t directly?  I.e. use something like:

  data = (ptid_t *) xmalloc (sizeof (ptid_t));
  *data = ptid;

> +/* Disable storing stack temporaries for the thread whose id is
> +   stored in DATA.  */
> +
> +static void
> +disable_thread_stack_temporaries (void *data)
> +{
> +  struct ptid_data *pd = data;
> +  struct thread_info *tp = find_thread_ptid (pd->ptid);
> +
> +  if (tp != NULL)
> +    tp->stack_temporaries_enabled = 0;
> +
> +  xfree (pd);

Why doesn't this also free the stack_temporaries vector itself?
This would save a second cleanup (and also make sense anyway).

> +/* Return non-zero value if stack temporaies are enabled for the thread

Typo.


Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]