public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Andrew Burgess <andrew.burgess@embecosm.com>
To: Felix Willgerodt <felix.willgerodt@intel.com>
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH] gdb/fortran: Add 'LOC' intrinsic support.
Date: Thu, 4 Mar 2021 10:11:17 +0000	[thread overview]
Message-ID: <20210304101117.GA1720904@embecosm.com> (raw)
In-Reply-To: <20210304095044.3017369-1-felix.willgerodt@intel.com>

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

  reply	other threads:[~2021-03-04 10:11 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-04  9:50 Felix Willgerodt
2021-03-04 10:11 ` Andrew Burgess [this message]
2021-03-04 13:26   ` Willgerodt, Felix

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210304101117.GA1720904@embecosm.com \
    --to=andrew.burgess@embecosm.com \
    --cc=felix.willgerodt@intel.com \
    --cc=gdb-patches@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).