Re: [PATCH] Logical short circuiting with Fortran argument lists

[Richard Bunt <richard.bunt@arm.com>]

On 08/03/2018 08:24 PM, Simon Marchi wrote:
> On 2018-08-03 05:32, Richard Bunt wrote:
>> Logical short circuiting with argument lists
>>
>> When evaluating Fortran expressions such as the following:
>>
>> 	print truth_table(1, 1) .OR. truth_table(2, 1)
>>
>> where truth_table(1, 1) evaluates to true, the debugger would report
>> that it could not perform substring operations on this type. This patch
>> addresses this issue.
>>
>> Investigation revealed that EVAL_SKIP was not being handled correctly
>> for all types serviced by the OP_F77_UNDETERMINED_ARGLIST case in
>> evaluate_subexp_standard. While skipping an undetermined argument
>> list the type is resolved to be an integer (as this is what
>> evaluate_subexp returns when skipping) and so it was not possible to
>> delegate to the appropriate case (e.g. array, function call).
>>
>> The solution proposed here is to hoist the skip from the function call
>> case to apply to all types of argument list.
>>
>> While this patch allows a wider range of expressions to be evaluated, 
>> it
>> should be noted that this patch does not allow the skipping of arrays
>> which use Fortran array slicing, due to the inability of the debugger
>> to skip OP_RANGE.
>>
>> This patch has been tested with GCC 7.3 on x86_64, aarch64 and ppc64le.
> 
> Hi Richard,
> 
> Thanks for the patch, hat sounds reasonnable.  Just one question:
> 
>> diff --git a/gdb/eval.c b/gdb/eval.c
>> index
>> 9db6e7c69dad9e674f66991e2aee7dd8d66d80c7..2350c7faaf6221a5990cf5264fe7fe93b4f4be4c
>> 100644
>> --- a/gdb/eval.c
>> +++ b/gdb/eval.c
>> @@ -1902,6 +1902,17 @@ evaluate_subexp_standard (struct type 
>> *expect_type,
>>
>>        /* First determine the type code we are dealing with.  */
>>        arg1 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
>> +
>> +      /* Allow the short-circuiting of array accesses, function calls
>> +	 and substring operations in logical expressions.  */
>> +      if (noside == EVAL_SKIP)
>> +	{
>> +	  /* Skip all the array subscripts.  */
>> +	  for (int i = 0; i < nargs; ++i)
>> +	    evaluate_subexp (NULL_TYPE, exp, pos, noside);
>> +	  return eval_skip_value (exp);
>> +	}
>> +
>>        type = check_typedef (value_type (arg1));
>>        code = TYPE_CODE (type);
>>
>> @@ -1952,8 +1963,6 @@ evaluate_subexp_standard (struct type 
>> *expect_type,
>>  	  for (; tem <= nargs; tem++)
>>  	    argvec[tem] = evaluate_subexp_with_coercion (exp, pos, noside);
>>  	  argvec[tem] = 0;	/* signal end of arglist */
>> -	  if (noside == EVAL_SKIP)
>> -	    return eval_skip_value (exp);
> 
> Is this change needed?  Your test passes even when leaving that part 
> as-is.

Hi Simon,

Many thanks for the review.

To clarify, do you observe the test passing with no changes to eval.c or
just when the additions are applied?

If the former is the case, may I ask which compiler are you using? I
have retested with 953473375500 and I see 13 tests fail without the
additions. If it's the latter, the deletions are not strictly needed but
my analysis determined that this code was unreachable after this patch.

My analysis consisted of checking for regressions in the test suite (of
which there were none) and examining all uses of noside from the new
early termination to eval_call in TYPE_CODE_FUNC. There are no
assignments and it's passed to all functions by value.

I can separate this removal into a second patch if this is desirable.

Rich

> 
> Thanks,
> 
> Simon
>