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

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Siva Chandra wrote:
> On Mon, Nov 3, 2014 at 6:43 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> > 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?

Yes, having an additional flag in struct expression would fix the safety
issue.  Moving initialization to evalute_subexp if *pos == 0 would then
no longer be safety issue, but simply enabling use of temporaries in more
cases.

The other option I've been thinking of would be to move the whole handling
of temporaries to infcall.c, by providing two functions there, e.g. called:

  infcall_enable_on_stack_temporaries ()
  infcall_cleanup_on_stack_temporaries ()

These would use a new field in the inferior_thread () thread struct to
store the vector of currently in use on-stack temporaries.  Any call to
call_function_by_hand after infcall_enable_on_stack_temporaries (and
before infcall_cleanup_on_stack_temporaries) on the same thread would
be allowed to use temporaries.  [ There could be sanity checks like
having call_function_by_hand verify that the current SP still equals
the thread SP at the time infcall_enable_on_stack_temporaries was
called.  Also, infrun could assert that a thread that has temporaries
in use must only ever run during is_infcall, to catch missing cleanup
calls.  ]

The only thing evaluate_expression would then need to do is to call
infcall_enable_on_stack_temporaries and install a cleanup calling
infcall_cleanup_on_stack_temporaries.


> >>@@ -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).

So I'm thinking of the following sequence of events:

- We call the first call_function_by_hand with a current sp value SP
  (no pre-existing temporaries at this point).
- We have to reserve the red zone, setting sp e.g. to SP - 256.
- We now allocate a temporary, taking up e.g. SP - 256 .. SP - 512.
- First call_function_by_hand returns.
- We get a second call_function_by_hand call.
- Now we have a temporary on the stack from  SP - 256 .. SP - 512,
  so skip_current_expression_stack skips down to SP - 512.
- *Now* code allocates the red zone again, setting SP to SP - 768.
  Even though this is unnecessary; the red zone is actually already
  there in the range SP ... SP - 256.

Bascially, adding a red zone at any point other the original SP is
useless, since the only point of the red zone is to protect data that
the original inferior code might have written to its stack immediately
below the original SP.

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

You mean "Any two or more lines in code should be wrapped in braces, even
if they are comments, as they look like separate statements"?  I don't think
this is intended to apply to cases like the above where a single statement
just had to split into multiple lines since it doesn't fit into one.
This case will never "look like separate statements".  In any case, all
the existing precedent in GDB does not have extra braces in that case.

Bye,
Ulrich

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