Re: [PATCH] gdb: fix regression in evaluate_funcall for non C++ like cases

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2021-06-21 7:39 p.m., Andrew Burgess wrote:
> This regression, as it is exposed by the test added in this commit,
> first became noticable with this commit:
> 
>   commit d182f2797922a305fbd1ef6a483cc39a56b43e02
>   Date:   Mon Mar 8 07:27:57 2021 -0700
> 
>       Convert c-exp.y to use operations
> 
> But, this commit only added converted the C expression parser to make
> use of code that was added in this commit:
> 
>   commit a00b7254fb614af557de7ae7cc0eb39a0ce0e408
>   Date:   Mon Mar 8 07:27:57 2021 -0700
> 
>       Implement function call operations
> 
> And it was this second commit that actually introduced the bugs (there
> are two).
> 
> In structop_base_operation::evaluate_funcall we build up an argument
> list in the vector vals.  Later in this function the argument list
> might be passed to value_struct_elt.
> 
> Prior to commit a00b7254fb614 the vals vector (or argvec as it used to
> be called) stored the value for the function callee in the argvec at

Do you mean the "this" value?  It's not clear which value "value" refers
to.

> index 0.  After commit a00b7254fb614 the callee value is now held in a
> separate variable.
> 
> What this means is that previous, when we called value_struct_elt we
> would pass the address of argvec[1] as this was the first argument.
> But now we should be passing the address of vals[0].  Unfortunately,
> we are still passing vals[1], effectively skipping the first
> argument.
> 
> The second issue is that, prior to commit a00b7254fb614, the argvec
> array was NULL terminated.  This is required as value_struct_elt
> calls search_struct_method, which calls typecmp, and typecmp requires
> that the array have a NULL at the end.
> 
> After commit a00b7254fb614 this NULL has been lost, and we are
> therefore violating the API requirements of typecmp.
> 
> This commit fixes both of these regressions.  I also extended the
> header comments on search_struct_method and value_struct_elt to make
> it clearer that the array required a NULL marker at the end.

I'm a little troubled by what you said in the bug, that the language is
set to C, because we are stopped in libc.  I understand why it's set to
C.  But the fact that calling a method works anyway _and_ the behavior
is different than what you'd get if you were stopped anywhere else in
GDB and the language was C++ (w.r.t overload resolution), that's a bit
scary.  It means my "print current_inferior_..." command could have
different results depending on where I'm stopped.

I don't have any idea for a better design, I just never realized this
before.

In the test, I'd suggest starting the expect value at something else
than 0, since 0 is a bit more likely to happen "by chance".  Maybe start
at a completely arbitrary value like 123.  But otherwise, the patch
LGTM, thanks.

Simon