RE: [PATCH] gdb/fortran: Add 'LOC' intrinsic support.

["Willgerodt, Felix" <felix.willgerodt@intel.com>]

Thanks for the pointers, Fortran isn't my strong point nor is GDB's parsing logic. This is a pretty old internal patch written by someone else and I thought it would be low hanging fruit to rebase and upstream.
Hence I didn't put as much thought into it as I probably should have.

> GDB's Fortran expression parser already supports `&`, so I'm wondering why you felt we should diverge from the spec here?

Agreed, this shouldn't diverge, I rewrote it to follow your expectation and just print an int.

> I wonder if this would be better handled as a Fortran exclusive expression?

I rewrote the parsing using your example.

I can wait until Tom's series has landed or rely on him adjusting this during his rebasing. I don't mind either.
I will post a v2 soon anyway, to get feedback on my rewrite.

Thanks,
Felix

-----Original Message-----
From: Andrew Burgess <andrew.burgess@embecosm.com> 
Sent: Donnerstag, 4. März 2021 11:11
To: Willgerodt, Felix <felix.willgerodt@intel.com>
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH] gdb/fortran: Add 'LOC' intrinsic support.

* Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org> [2021-03-04 10:50:44 +0100]:

> LOC(X) returns the address of X as an integer:
> https://gcc.gnu.org/onlinedocs/gfortran/LOC.html
> 
> Before:
> (gdb) p LOC(l)
> No symbol "LOC" in current context.
> 
> After:
> (gdb) p LOC(l)
> $1 = (PTR TO -> ( logical(kind=4) )) 0x7fffffffccfc

Thanks doing this.

I'm confused by the results you have included here.  The description that you have included for the operator says the result is returned as an integer, but the result you have here is a pointer.

I would have expected this:

  (gdb) p LOC(l)
  $1 = 0x7fffffffccfc
  (gdb) ptype $1
  type = integer

GDB's Fortran expression parser already supports `&`, so I'm wondering why you felt we should diverge from the spec here?

> 
> gdb/ChangeLog:
> 2021-03-04  Felix Willgerodt  <felix.willgerodt@intel.com>
> 	    Abhishek Aggarwal  <abhishek.a.aggarwal@intel.com>
> 
> 	* ax-gdb.c (gen_expr): Add case for UNOP_LOC.
> 	* eval.c (evaluate_subexp_standard): Add case for UNOP_LOC.
> 	* expprint.c (dump_subexp_body_standard): Add case for UNOP_LOC.
> 	* f-exp.y (exp): Add LOC.
> 	(pytpe): Same.
> 	(f77_keywords): Same.
> 	* std-operator.def (OP): Same.

I wonder if this would be better handled as a Fortran exclusive expression?

Additionally, I don't think your changes in f-exp.y need to be as extensive.  Could I recommend this commit as a template for how a single operand expression might be added:

  commit 96df3e28b835ccb5804bcca96f417761e5e8be67
  Date:   Thu Feb 11 13:34:06 2021 +0000

      gdb/fortran: support ALLOCATED builtin

Alternatively you should probably mention why this approach doesn't work in this case.

**WARNING** - Next Monday Tom Tromey is going to be committing a big
  rewrite of GDB's expression handling code.  Which will have some
  impact on how new expressions are implemented.  See:

    https://sourceware.org/pipermail/gdb-patches/2021-March/176721.html

Thanks,
Andrew


> 
> gdb/testsuite/ChangeLog:
> 2020-07-04  Felix Willgerodt  <felix.willgerodt@intel.com>
> 
> 	* gdb.fortran/intrinsics.exp: Add LOC test.
> ---
>  gdb/ax-gdb.c                             | 1 +
>  gdb/eval.c                               | 8 ++++++++
>  gdb/expprint.c                           | 1 +
>  gdb/f-exp.y                              | 7 ++++++-
>  gdb/std-operator.def                     | 1 +
>  gdb/testsuite/gdb.fortran/intrinsics.exp | 4 ++++
>  6 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c index 
> fa777281c1e..3596f856c73 100644
> --- a/gdb/ax-gdb.c
> +++ b/gdb/ax-gdb.c
> @@ -2185,6 +2185,7 @@ gen_expr (struct expression *exp, union exp_element **pc,
>        gen_deref (value);
>        break;
>  
> +    case UNOP_LOC:
>      case UNOP_ADDR:
>        (*pc)++;
>        gen_expr (exp, pc, ax, value);
> diff --git a/gdb/eval.c b/gdb/eval.c
> index 7ba3ee59522..fffb409f6e5 100644
> --- a/gdb/eval.c
> +++ b/gdb/eval.c
> @@ -2490,6 +2490,14 @@ evaluate_subexp_standard (struct type *expect_type,
>  	return value_from_longest (size_type, align);
>        }
>  
> +    case UNOP_LOC:
> +      if (noside == EVAL_SKIP)
> +	{
> +	  evaluate_subexp (nullptr, exp, pos, EVAL_SKIP);
> +	  return eval_skip_value (exp);
> +	}
> +      return evaluate_subexp_for_address (exp, pos, noside);
> +
>      case UNOP_CAST:
>        (*pos) += 2;
>        type = exp->elts[pc + 1].type;
> diff --git a/gdb/expprint.c b/gdb/expprint.c index 
> d95835fc47d..ae66eceff30 100644
> --- a/gdb/expprint.c
> +++ b/gdb/expprint.c
> @@ -866,6 +866,7 @@ dump_subexp_body_standard (struct expression *exp,
>      case UNOP_POSTDECREMENT:
>      case UNOP_SIZEOF:
>      case UNOP_ALIGNOF:
> +    case UNOP_LOC:
>      case UNOP_PLUS:
>      case UNOP_CAP:
>      case UNOP_CHR:
> diff --git a/gdb/f-exp.y b/gdb/f-exp.y index f5360c10ef6..3d0dd2de93b 
> 100644
> --- a/gdb/f-exp.y
> +++ b/gdb/f-exp.y
> @@ -160,7 +160,7 @@ static int parse_number (struct parser_state *, 
> const char *, int,
>  
>  %token <ssym> NAME_OR_INT
>  
> -%token SIZEOF KIND
> +%token SIZEOF KIND LOC
>  %token ERROR
>  
>  /* Special type cases, put in to allow the parser to distinguish different
> @@ -243,6 +243,10 @@ exp	:	SIZEOF exp       %prec UNARY
>  			{ write_exp_elt_opcode (pstate, UNOP_SIZEOF); }
>  	;
>  
> +exp	:	LOC exp       %prec UNARY
> +			{ write_exp_elt_opcode (pstate, UNOP_LOC); }
> +	;
> +
>  exp	:	KIND '(' exp ')'       %prec UNARY
>  			{ write_exp_elt_opcode (pstate, UNOP_FORTRAN_KIND); }
>  	;
> @@ -1038,6 +1042,7 @@ static const struct token f77_keywords[] =
>    { "precision", PRECISION, BINOP_END, true },
>    /* The following correspond to actual functions in Fortran and are case
>       insensitive.  */
> +  { "loc", LOC, BINOP_END, false },
>    { "kind", KIND, BINOP_END, false },
>    { "abs", UNOP_INTRINSIC, UNOP_ABS, false },
>    { "mod", BINOP_INTRINSIC, BINOP_MOD, false }, diff --git 
> a/gdb/std-operator.def b/gdb/std-operator.def index 
> 99b5d90381a..afd3dbc1598 100644
> --- a/gdb/std-operator.def
> +++ b/gdb/std-operator.def
> @@ -227,6 +227,7 @@ OP (UNOP_PREDECREMENT)		/* -- before an expression */
>  OP (UNOP_POSTDECREMENT)		/* -- after an expression */
>  OP (UNOP_SIZEOF)		/* Unary sizeof (followed by expression) */
>  OP (UNOP_ALIGNOF)		/* Unary alignof (followed by expression) */
> +OP (UNOP_LOC)			/* Unary loc (followed by expression) */
>  
>  OP (UNOP_PLUS)			/* Unary plus */
>  
> diff --git a/gdb/testsuite/gdb.fortran/intrinsics.exp 
> b/gdb/testsuite/gdb.fortran/intrinsics.exp
> index d0ac1944aab..1d426e3dfb6 100644
> --- a/gdb/testsuite/gdb.fortran/intrinsics.exp
> +++ b/gdb/testsuite/gdb.fortran/intrinsics.exp
> @@ -84,3 +84,7 @@ gdb_test "ptype MODULO (3.0,2.0)" "type = real\\*8"
>  # Test CMPLX
>  
>  gdb_test "p CMPLX (4.1, 2.0)" " = \\(4.$decimal,2\\)"
> +
> +# Test LOC
> +
> +gdb_test "p LOC(l)" "(logical|LOGICAL).*$hex"
> --
> 2.25.4
> 
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de> Managing 
> Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva 
> Chairperson of the Supervisory Board: Nicole Lau Registered Office: 
> Munich Commercial Register: Amtsgericht Muenchen HRB 186928
> 
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928