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

[Siva Chandra <sivachandra at google dot com>]

On Mon, Nov 3, 2014 at 6:43 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
>> struct value *
>> evaluate_expression (struct expression *exp)
>> {
>>+  int i, pc = 0;
>>+  struct value *res, *val;
>>+  struct cleanup *cleanups;
>>+  value_vec *vec = exp->on_stack_temporaries_vec;
>>+
>>+  cleanups = make_cleanup (VEC_cleanup (value_ptr),
>>+                         &exp->on_stack_temporaries_vec);
>>+  res = evaluate_subexp (NULL_TYPE, exp, &pc, EVAL_NORMAL);
>>+  /* If the result is on the expression stack, fetch it and mark it as
>>+     not_lval.  */
>>+  for (i = 0; VEC_iterate (value_ptr, vec, 1, val); i++)
>>+    {
>>+      if (res == val)
>>+      {
>>+        if (value_lazy (res))
>>+          value_fetch_lazy (res);
>>+        VALUE_LVAL (res) = not_lval;
>>+        break;
>>+      }
>>+    }
>>+  do_cleanups (cleanups);
>
> There's two issues I see with that:
>
> The minor issue is that modifying a value's lval status by simply assigning
> to VALUE_LVAL has been deprecated for a long time.  While this is another one
> of those partial transformations in GDB code, it would still be better to
> avoid making it worse; if we do need to modify a value's lval, it ought to
> be done via a defined interface implemented in value.c (the implementation
> can simply assign to ->lval direcly).  See also below ...
>
> The more significant problem is that this code is unsafe as is, in the sense
> that it may happen that a temporary is allocated on the stack, but the
> cleanup above never runs for it.  The problem is that all the subroutines
> of evaluate_subexp will pass an expression to call_function_by_hand, but
> only if evaluate_subexp was called via evaluate_expression will the cleanup
> be performed.
>
> Unfortunately, there are several code paths where evaluate_subexp or one of
> its subroutines is called directly.  For example, there's print_object_command
> in objc-lang.c, there's fetch_subexp_value (used from watchpoint code), and
> there is even stap_evaluate_probe_argument calling evaluate_subexp_standard
> directly (I may have missed some further calls).
>
> This is critical not just to make the sure the values are made non_lval
> (on in the future, have destructors called), but also to make sure the
> exp->on_stack_temporaries_vec field is cleared -- if we were to evalutate
> that expression a second time, we'd have invalid values in that vector:
> they might have already been deleted and we access invalid memory, or even
> if not, the skip_current_expression_stack may come up with completely wrong
> SP values (say, from a different thread's stack).
>
> The question is how to the fix that.  One way would be to enfore a rule that
> expression evaluation must go through one (or a set of) particular routines
> and fix all callers that currently violate that rule.  (For example, one
> could imagine doing the cleanup in evaluate_subexp whenever called with
> pc == 0, and changing stap_evaluate_probe_argument to call evaluate_subexp
> instead of evaluate_subexp_standard.)  There is a certain risk of future
> changes re-introducing violations of that rule if it cannot be enforced
> by the compiler ...
>
> Another way might be to continue allowing calls into any subroutine of
> expression evaluation, but set things up so that call_function_by_hand
> will only allow creation of stack temporaries *if* the call came through
> evaluate_expression.  This would require evaluate_expression to take
> some action to enable temporary creation, which could e.g. take the
> form of setting a flag in the expression struct, allocating some other
> structure to hold temporary data and install it into a pointer in the
> expression struct, or even storing the information elsewhere (like in
> the thread struct).

I knew this was coming ... :)

One thing that I did think about was to put this code that I have in
evaluate_expression in evaluate_subexp under if (*pos == 0). This
would also require that we have a flag in struct expression to
indicate to call_function_by_hand that we are evaluating a real/full
expression and that it needs to store temporary values on the stack.
This will make all callers of evaluate_subexp (including
evaluate_expression and evaluate_type) getting the temporary
management right. But, I am not sure if this is complete. WDYT?

>>+/* Return true is T is a class or a union.  False otherwise.  */
>>+
>>+int
>>+class_or_union_p (const struct type *t)
>>+{
>>+  return (TYPE_CODE (t) == TYPE_CODE_STRUCT
>>+        || TYPE_CODE (t) == TYPE_CODE_UNION);
>>+}
>
> I understand we need to do this for classes with member functions (so that
> f().g() will work) -- do we really need it for classes without member
> functions (or plain C structs)?

We could have a struct like this in C++:

struct Derived : public virtual Base
{
 ...
};

Do you mean we should have a language check before reserving space on the stack?

>>@@ -532,6 +563,16 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
>>   {
>>     CORE_ADDR old_sp = get_frame_sp (frame);
>>
>>+    /* Skip over the stack mirrors that might have been generated during the
>>+       evaluation of the current expression.  */
>>+    if (exp != NULL)
>>+      {
>>+      if (gdbarch_inner_than (gdbarch, 1, 2))
>>+        old_sp = skip_current_expression_stack (exp, old_sp, 1);
>>+      else
>>+        old_sp = skip_current_expression_stack (exp, old_sp, 0);
>>+      }
>>+
>>     if (gdbarch_frame_align_p (gdbarch))
>>       {
>>       sp = gdbarch_frame_align (gdbarch, old_sp);
>
> Skipping over existing temporaries *here* may cause the red zone to be added
> a second time on platforms that use it.  This will not necessarily cause any
> problems, but still seems wasteful.  Might be better to skip over temporaries
> *after* the red zone is added, but then need to enforce alignment afterwards
> again.

This is doing a logical "skip" and not really placing the temporaries
before the red zone. Am I missing some other place where red zone is
being "added"? AFAICT, red zone is added right after the above piece
and only at that place. The actuall address of the temporary is
computed further down after the red zone is added and after the
temporaries are skipped.

The only reason I have added the "skip" part here is because the
alignment is done anyway after the red zone is added (or not).

>>@@ -1059,13 +1112,8 @@ When the function is done executing, GDB will silently stop."),
>>        At this stage, leave the RETBUF alone.  */
>>     restore_infcall_control_state (inf_status);
>>
>>-    /* Figure out the value returned by the function.  */
>>-    retval = allocate_value (values_type);
>>-
>>     if (hidden_first_param_p)
>>-      read_value_memory (retval, 0, 1, struct_addr,
>>-                       value_contents_raw (retval),
>>-                       TYPE_LENGTH (values_type));
>>+      retval = get_return_value_from_memory (values_type, struct_addr, exp);
>>     else if (TYPE_CODE (target_values_type) != TYPE_CODE_VOID)
>>       {
>>       /* If the function returns void, don't bother fetching the
>>@@ -1076,16 +1124,30 @@ When the function is done executing, GDB will silently stop."),
>>         case RETURN_VALUE_REGISTER_CONVENTION:
>>         case RETURN_VALUE_ABI_RETURNS_ADDRESS:
>>         case RETURN_VALUE_ABI_PRESERVES_ADDRESS:
>>+          retval = allocate_value (values_type);
>>           gdbarch_return_value (gdbarch, function, values_type,
>>                                 retbuf, value_contents_raw (retval), NULL);
>>+          if (exp != NULL && class_or_union_p (values_type))
>>+            {
>>+              /* Values of class type returned in registers are copied onto
>>+                 the stack and their lval_type set to lval_memory.  This is
>>+                 required because further evaluation of the expression
>>+                 could potentially invoke methods on the return value
>>+                 requiring GDB to evaluate the "this" pointer.  To evaluate
>>+                 the this pointer, GDB needs the memory address of the
>>+                 value.  */
>>+              write_value_to_memory (retval, struct_addr);
>>+              add_value_to_expression_stack (exp, retval);
>>+            }
>>           break;
>>         case RETURN_VALUE_STRUCT_CONVENTION:
>>-          read_value_memory (retval, 0, 1, struct_addr,
>>-                             value_contents_raw (retval),
>>-                             TYPE_LENGTH (values_type));
>>+          retval = get_return_value_from_memory (values_type, struct_addr,
>>+                                                 exp);
>>           break;
>>         }
>>       }
>>+    else
>>+      retval = allocate_value (values_type);
>>
>>     do_cleanups (retbuf_cleanup);
>
> For RETURN_VALUE_ABI_RETURNS_ADDRESS and RETURN_VALUE_ABI_PRESERVES_ADDRESS,
> we in fact have already allocated the return value on the stack (i.e.
> struct_return will be true), so there is no need to write anything back
> to the stack.  This didn't matter with current code, since for those two
> scenarios, gdbarch_return_value will in effect to the same as the
> read_value_memory call in the RETURN_VALUE_STRUCT_CONVENTION case, but
> with your patch, it seems better to avoid the extra store.

This part is confusing to me as, with my little experience with
different archs, I did not really understand the implications of
RETURN_VALUE_ABI_RETURNS_ADDRESS and
RETURN_VALUE_ABI_PRESERVES_ADDRESS. The comments describing them are
confusing (to me atleast) as they talk about return address in
registers et al. But if you say that the following code that you
suggest is good enough, I will go with it.

> Might be simplest to remove the switch (gdbarch_return_value) and just do
>
>   if (TYPE_CODE (values_type) == TYPE_CODE_VOID)
>     retval = allocate_value (values_type);
>   else if (struct_return || hidden_first_param_p)
>     retval = get_return_value_from_memory (values_type, struct_addr, exp);
>   else
>     {
>       retval = allocate_value (values_type);
>       gdbarch_return_value (gdbarch, function, values_type,
>                             retbuf, value_contents_raw (retval), NULL);
>       ...
>     }

...

>>         if (binop_user_defined_p (op, arg1, arg2))
>>-          res_val = value_x_binop (arg1, arg2, op, OP_NULL, EVAL_NORMAL);
>>+          {
>>+            res_val = value_x_binop (arg1, arg2, op, OP_NULL,
>>+                                     EVAL_NORMAL, NULL);
>>+          }
>
> We don't need to add the braces here.

I could be reading wrong, but there is an entry dealing with this
specific point here:
https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards

I will go with your suggestions for other pieces I did not talk about here.

Thanks a lot for reviewing.
Siva Chandra