[v1] gdb/fortran: Align intrinsic/variable precedence

Message ID 20230713072008.1844497-1-richard.bunt@linaro.org
State New
Headers
Series [v1] gdb/fortran: Align intrinsic/variable precedence |

Commit Message

Richard Bunt July 13, 2023, 7:20 a.m. UTC
  Fortran allows variables to be named after existing intrinsics as they
are not reserved keywords.

The behavior before this patch was to favour the intrinsic, which meant
that any variables named, for example "allocated", could not be
inspected by GDB.

This patch inverts this priority to bring GDB's behaviour closer to the
Fortran language, where the user defined symbol can hide the intrinsic.

The sizeof intrinsic is now case insensitive. This is closer to the
Fortran language.  I believe this change is safe enough as it increases
the acceptance of the grammar, rather than restricts it. I.e. it should
not break any existing scripts which rely on it. Unless of course they
rely on SIZEOF being rejected.

GDB built with GCC 13.

No test suite regressions detected. Compilers: GCC, ACfL, Intel, Intel
LLVM, NVHPC; Platforms: x86_64, aarch64.

---
 gdb/f-exp.y                                   | 57 ++++++++++++-------
 .../gdb.fortran/intrinsic-precedence.exp      | 44 ++++++++++++++
 .../gdb.fortran/intrinsic-precedence.f90      | 24 ++++++++
 3 files changed, 106 insertions(+), 19 deletions(-)
 create mode 100644 gdb/testsuite/gdb.fortran/intrinsic-precedence.exp
 create mode 100644 gdb/testsuite/gdb.fortran/intrinsic-precedence.f90
  

Comments

Tom Tromey July 13, 2023, 4:32 p.m. UTC | #1
>>>>> "Richard" == Richard Bunt via Gdb-patches <gdb-patches@sourceware.org> writes:

Richard> Fortran allows variables to be named after existing intrinsics as they
Richard> are not reserved keywords.

Thank you for the patch.

I don't know Fortran but I had a couple questions about the patch
itself.

Richard> No test suite regressions detected. Compilers: GCC, ACfL, Intel, Intel
Richard> LLVM, NVHPC; Platforms: x86_64, aarch64.

I wonder whether the intrinsics are actually tested by the test suite.

Richard> +static const struct token f_intrinsics[] =
Richard> +{
Richard> +  /* The following correspond to actual functions in Fortran and are case
Richard> +     insensitive.  */
Richard> +  { "kind", KIND, OP_NULL, false },
Richard> +  { "abs", UNOP_INTRINSIC, UNOP_ABS, false },
...
Richard> +    /* This is post the symbol search as symbols can hide intrinsics.  Also,
Richard> +       give Fortran intrinsics priority over C symbols.  This prevents
Richard> +       non-Fortran symbols from hiding intrinsics e.g. abs.  */

Will this code wind up finding the libm "abs" function and essentially
never the intrinsic?

Richard> +if { ![fortran_runto_main] } {
Richard> +    perror "Could not run to main."
Richard> +    return

I think perror is basically never the right thing to use.  See, e.g.,
commit 30add7ee.

Richard> +foreach_with_prefix case {"LOC" "loc"} {
Richard> +    gdb_test "print $case" "17" \
Richard> +	     "user defined uppercase LOC hides the intrinsic"
Richard> +}

I don't think you need foreach_with_prefix here.  Just omit the test
name (which seems wrong-ish anyway?) and the test will be give the same
name as the command.

Richard> +gdb_test "print UBOUND" "79" \
Richard> +	 "user defined lowercase ubound hides the intrinsic"

Normally the Tcl code uses 4-space indent, though this isn't really done
super consistently throughout.

Tom
  
Richard Bunt July 14, 2023, 10:46 a.m. UTC | #2
Hi Tom,

Thanks for the comments. Responses are inline.

On 13/07/2023 17:32, Tom Tromey wrote:
>>>>>> "Richard" == Richard Bunt via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Richard> Fortran allows variables to be named after existing intrinsics as they
> Richard> are not reserved keywords.
> 
> Thank you for the patch.
> 
> I don't know Fortran but I had a couple questions about the patch
> itself.
> 
> Richard> No test suite regressions detected. Compilers: GCC, ACfL, Intel, Intel
> Richard> LLVM, NVHPC; Platforms: x86_64, aarch64.
> 
> I wonder whether the intrinsics are actually tested by the test suite.


There are several tests in gdb.fortran which cover Fortran intrinsics: 
intrinsics.exp, shape.exp, rank.exp, lbound-ubound.exp.

> 
> Richard> +static const struct token f_intrinsics[] =
> Richard> +{
> Richard> +  /* The following correspond to actual functions in Fortran and are case
> Richard> +     insensitive.  */
> Richard> +  { "kind", KIND, OP_NULL, false },
> Richard> +  { "abs", UNOP_INTRINSIC, UNOP_ABS, false },
> ...
> Richard> +    /* This is post the symbol search as symbols can hide intrinsics.  Also,
> Richard> +       give Fortran intrinsics priority over C symbols.  This prevents
> Richard> +       non-Fortran symbols from hiding intrinsics e.g. abs.  */
> 
> Will this code wind up finding the libm "abs" function and essentially
> never the intrinsic?


I first confirmed that the abs intrinsic is called in preference to the 
abs function by placing a breakpoint on eval_op_f_abs. This breakpoint 
was hit. Similarly, a breakpoint placed on abs in the inferior was not hit.

Importantly, if I force the language to C and perform a similar 
experiment, the function call to abs is made.

Digging a bit further, the lookup_symbol call that populates result does 
not find abs at all! When I first wrote this patch, I believe it was 
found, which is why the following condition is set up to allow 
intrinsics to take precedence over any non-fortran symbols.

if (!result.symbol || result.symbol->language () != language_fortran)

The fact that abs was not found by lookup_symbol struck me as odd, so I 
want to investigate this behavior a little more before putting together 
a v2. This is because I might be able to clean the above condition up.

> 
> Richard> +if { ![fortran_runto_main] } {
> Richard> +    perror "Could not run to main."
> Richard> +    return
> 
> I think perror is basically never the right thing to use.  See, e.g.,
> commit 30add7ee.
> 
> Richard> +foreach_with_prefix case {"LOC" "loc"} {
> Richard> +    gdb_test "print $case" "17" \
> Richard> +	     "user defined uppercase LOC hides the intrinsic"
> Richard> +}
> 
> I don't think you need foreach_with_prefix here.  Just omit the test
> name (which seems wrong-ish anyway?) and the test will be give the same
> name as the command.
> 
> Richard> +gdb_test "print UBOUND" "79" \
> Richard> +	 "user defined lowercase ubound hides the intrinsic"
> 
> Normally the Tcl code uses 4-space indent, though this isn't really done
> super consistently throughout.
> 

Thanks for catching these test issues. I will address these in a v2.

> Tom
  

Patch

diff --git a/gdb/f-exp.y b/gdb/f-exp.y
index c0afebc0589..cbae994749c 100644
--- a/gdb/f-exp.y
+++ b/gdb/f-exp.y
@@ -1278,6 +1278,28 @@  static const struct f77_boolean_val boolean_values[]  =
   { ".false.", 0 }
 };
 
+static const struct token f_intrinsics[] =
+{
+  /* The following correspond to actual functions in Fortran and are case
+     insensitive.  */
+  { "kind", KIND, OP_NULL, false },
+  { "abs", UNOP_INTRINSIC, UNOP_ABS, false },
+  { "mod", BINOP_INTRINSIC, BINOP_MOD, false },
+  { "floor", UNOP_OR_BINOP_INTRINSIC, FORTRAN_FLOOR, false },
+  { "ceiling", UNOP_OR_BINOP_INTRINSIC, FORTRAN_CEILING, false },
+  { "modulo", BINOP_INTRINSIC, BINOP_FORTRAN_MODULO, false },
+  { "cmplx", UNOP_OR_BINOP_OR_TERNOP_INTRINSIC, FORTRAN_CMPLX, false },
+  { "lbound", UNOP_OR_BINOP_OR_TERNOP_INTRINSIC, FORTRAN_LBOUND, false },
+  { "ubound", UNOP_OR_BINOP_OR_TERNOP_INTRINSIC, FORTRAN_UBOUND, false },
+  { "allocated", UNOP_INTRINSIC, UNOP_FORTRAN_ALLOCATED, false },
+  { "associated", UNOP_OR_BINOP_INTRINSIC, FORTRAN_ASSOCIATED, false },
+  { "rank", UNOP_INTRINSIC, UNOP_FORTRAN_RANK, false },
+  { "size", UNOP_OR_BINOP_OR_TERNOP_INTRINSIC, FORTRAN_ARRAY_SIZE, false },
+  { "shape", UNOP_INTRINSIC, UNOP_FORTRAN_SHAPE, false },
+  { "loc", UNOP_INTRINSIC, UNOP_FORTRAN_LOC, false },
+  { "sizeof", SIZEOF, OP_NULL, false },
+};
+
 static const token f_keywords[] =
 {
   /* Historically these have always been lowercase only in GDB.  */
@@ -1300,27 +1322,9 @@  static const token f_keywords[] =
   { "real_4", REAL_S4_KEYWORD, OP_NULL, true },
   { "real_8", REAL_S8_KEYWORD, OP_NULL, true },
   { "real_16", REAL_S16_KEYWORD, OP_NULL, true },
-  { "sizeof", SIZEOF, OP_NULL, true },
   { "single", SINGLE, OP_NULL, true },
   { "double", DOUBLE, OP_NULL, true },
   { "precision", PRECISION, OP_NULL, true },
-  /* The following correspond to actual functions in Fortran and are case
-     insensitive.  */
-  { "kind", KIND, OP_NULL, false },
-  { "abs", UNOP_INTRINSIC, UNOP_ABS, false },
-  { "mod", BINOP_INTRINSIC, BINOP_MOD, false },
-  { "floor", UNOP_OR_BINOP_INTRINSIC, FORTRAN_FLOOR, false },
-  { "ceiling", UNOP_OR_BINOP_INTRINSIC, FORTRAN_CEILING, false },
-  { "modulo", BINOP_INTRINSIC, BINOP_FORTRAN_MODULO, false },
-  { "cmplx", UNOP_OR_BINOP_OR_TERNOP_INTRINSIC, FORTRAN_CMPLX, false },
-  { "lbound", UNOP_OR_BINOP_OR_TERNOP_INTRINSIC, FORTRAN_LBOUND, false },
-  { "ubound", UNOP_OR_BINOP_OR_TERNOP_INTRINSIC, FORTRAN_UBOUND, false },
-  { "allocated", UNOP_INTRINSIC, UNOP_FORTRAN_ALLOCATED, false },
-  { "associated", UNOP_OR_BINOP_INTRINSIC, FORTRAN_ASSOCIATED, false },
-  { "rank", UNOP_INTRINSIC, UNOP_FORTRAN_RANK, false },
-  { "size", UNOP_OR_BINOP_OR_TERNOP_INTRINSIC, FORTRAN_ARRAY_SIZE, false },
-  { "shape", UNOP_INTRINSIC, UNOP_FORTRAN_SHAPE, false },
-  { "loc", UNOP_INTRINSIC, UNOP_FORTRAN_LOC, false },
 };
 
 /* Implementation of a dynamically expandable buffer for processing input
@@ -1663,7 +1667,22 @@  yylex (void)
 					pstate->gdbarch (), tmp.c_str ());
     if (yylval.tsym.type != NULL)
       return TYPENAME;
-    
+
+    /* This is post the symbol search as symbols can hide intrinsics.  Also,
+       give Fortran intrinsics priority over C symbols.  This prevents
+       non-Fortran symbols from hiding intrinsics e.g. abs.  */
+    if (!result.symbol || result.symbol->language () != language_fortran)
+      for (int i = 0; i < ARRAY_SIZE (f_intrinsics); i++)
+	{
+	  gdb_assert (!f_intrinsics[i].case_sensitive);
+	  if (strlen (f_intrinsics[i].oper) == namelen
+	      && strncasecmp (tokstart, f_intrinsics[i].oper, namelen) == 0)
+	    {
+	      yylval.opcode = f_intrinsics[i].opcode;
+	      return f_intrinsics[i].token;
+	    }
+	}
+
     /* Input names that aren't symbols but ARE valid hex numbers,
        when the input radix permits them, can be names or numbers
        depending on the parse.  Note we support radixes > 16 here.  */
diff --git a/gdb/testsuite/gdb.fortran/intrinsic-precedence.exp b/gdb/testsuite/gdb.fortran/intrinsic-precedence.exp
new file mode 100644
index 00000000000..eb0b7a0cec3
--- /dev/null
+++ b/gdb/testsuite/gdb.fortran/intrinsic-precedence.exp
@@ -0,0 +1,44 @@ 
+# Copyright 2023 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile .f90
+load_lib fortran.exp
+
+require allow_fortran_tests
+
+if {[prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \
+	 {debug f90}]} {
+    return -1
+}
+
+if { ![fortran_runto_main] } {
+    perror "Could not run to main."
+    return
+}
+
+gdb_breakpoint [gdb_get_line_number "all-assigned"]
+
+gdb_continue_to_breakpoint "all-assigned"
+
+foreach_with_prefix case {"LOC" "loc"} {
+    gdb_test "print $case" "17" \
+	     "user defined uppercase LOC hides the intrinsic"
+}
+
+gdb_test "print UBOUND" "79" \
+	 "user defined lowercase ubound hides the intrinsic"
+
+gdb_test "print ABS" "20" \
+	 "user defined ABS hides the intrinsic and any non-fortran symbol"
diff --git a/gdb/testsuite/gdb.fortran/intrinsic-precedence.f90 b/gdb/testsuite/gdb.fortran/intrinsic-precedence.f90
new file mode 100644
index 00000000000..78c47bc4715
--- /dev/null
+++ b/gdb/testsuite/gdb.fortran/intrinsic-precedence.f90
@@ -0,0 +1,24 @@ 
+! Copyright 2023 Free Software Foundation, Inc.
+!
+! This program is free software; you can redistribute it and/or modify
+! it under the terms of the GNU General Public License as published by
+! the Free Software Foundation; either version 2 of the License, or
+! (at your option) any later version.
+!
+! This program is distributed in the hope that it will be useful,
+! but WITHOUT ANY WARRANTY; without even the implied warranty of
+! MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+! GNU General Public License for more details.
+!
+! You should have received a copy of the GNU General Public License
+! along with this program; if not, write to the Free Software
+! Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+
+program intrinsic_precedence
+    implicit none
+    integer LOC, ABS, ubound
+    LOC = 17
+    ABS = 20
+    ubound = 79
+    print *, LOC, ABS, ubound !all-assigned
+end program intrinsic_precedence