[PATCH 1/2] gdb: improve error reporting from expression parser

[Andrew Burgess <aburgess@redhat.com>]

This commits changes how errors are reported from the expression
parser.  Previously, parser errors were reported like this:

  (gdb) p a1 +}= 432
  A syntax error in expression, near `}= 432'.
  (gdb) p a1 +
  A syntax error in expression, near `'.

The first case is fine, a user can figure out what's going wrong, but
the second case is a little confusing; as the error occurred at the
end of the expression GDB just reports the empty string to the user.

After this commit the errors are now reported like this:

  (gdb) p a1 +}= 432
  A syntax error in expression near '<HERE>' marker in: `a1 +<HERE>}= 432'.
  (gdb) p a1 +
  A syntax error in expression near '<HERE>' marker in: `a1 +<HERE>'.

Now GDB reports the entire expression along with a <HERE> marker to
indicate where the error occurred.

I did consider trying to have multi-line errors here, in the style
that gcc produces, with some kind of '~~~~~^' marker on the second
line to indicate where the error occurred; but I rejected this due to
the places in GDB where we catch an error and repackage the message
within some longer string, I don't think multi-line error messages
would work well in that case.

Adding the '<HERE>' marker was actually a later extension I made.  My
first implementation simply tried to address the empty string
case (where GDB reports "..., near `'").  Originally I just changed
this to be:

  (gdb) p a1 +
  A syntax error in expression, at the end of `a1 +'.

But then I thought adding the '<HERE>' markers was pretty neat, so I
did that instead.  However, if folk feel the '<HERE>' marker is more
confusing than helpful I can always fall back to my original plan, and
just fix the empty string case.

I originally wanted to try and style the '<HERE>' marker, I thought
that would help make it clear that it wasn't part of the expression.
However, GDB's error function doesn't support styling right now.  We
could possibly add this in the future, in which case the <HERE> marker
could be given maybe the metadata style to help it stand out.

I've updated the small number of tests that check for a syntax error,
and add a couple of extra tests in gdb.base/exprs.exp.
---
 gdb/ada-exp.y                               |  2 +-
 gdb/c-exp.y                                 |  5 +----
 gdb/d-exp.y                                 |  5 +----
 gdb/f-exp.y                                 |  5 +----
 gdb/go-exp.y                                |  5 +----
 gdb/m2-exp.y                                |  5 +----
 gdb/p-exp.y                                 |  5 +----
 gdb/parse.c                                 | 16 ++++++++++++++++
 gdb/parser-defs.h                           |  9 +++++++++
 gdb/testsuite/gdb.ada/bp_c_mixed_case.exp   |  4 ++--
 gdb/testsuite/gdb.base/exprs.exp            |  7 +++++++
 gdb/testsuite/gdb.base/quit.exp             |  2 +-
 gdb/testsuite/gdb.base/settings.exp         |  4 ++--
 gdb/testsuite/gdb.base/watch_thread_num.exp |  2 +-
 gdb/testsuite/gdb.cp/local-static.exp       |  2 +-
 gdb/testsuite/gdb.dlang/expression.exp      |  2 +-
 16 files changed, 47 insertions(+), 33 deletions(-)

diff --git a/gdb/ada-exp.y b/gdb/ada-exp.y
index fcb5aa4379b..2a1cff50887 100644
--- a/gdb/ada-exp.y
+++ b/gdb/ada-exp.y
@@ -1212,7 +1212,7 @@ ada_parse (struct parser_state *par_state)
 static void
 yyerror (const char *msg)
 {
-  error (_("Error in expression, near `%s'."), pstate->lexptr);
+  pstate->parse_error (msg);
 }
 
 /* Emit expression to access an instance of SYM, in block BLOCK (if
diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 2b4c21850d3..6697b3b2278 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -3482,8 +3482,5 @@ c_print_token (FILE *file, int type, YYSTYPE value)
 static void
 yyerror (const char *msg)
 {
-  if (pstate->prev_lexptr)
-    pstate->lexptr = pstate->prev_lexptr;
-
-  error (_("A %s in expression, near `%s'."), msg, pstate->lexptr);
+  pstate->parse_error (msg);
 }
diff --git a/gdb/d-exp.y b/gdb/d-exp.y
index e2507982d50..627c681d895 100644
--- a/gdb/d-exp.y
+++ b/gdb/d-exp.y
@@ -1631,9 +1631,6 @@ d_parse (struct parser_state *par_state)
 static void
 yyerror (const char *msg)
 {
-  if (pstate->prev_lexptr)
-    pstate->lexptr = pstate->prev_lexptr;
-
-  error (_("A %s in expression, near `%s'."), msg, pstate->lexptr);
+  pstate->parse_error (msg);
 }
 
diff --git a/gdb/f-exp.y b/gdb/f-exp.y
index e4e2171d641..88a95bccb11 100644
--- a/gdb/f-exp.y
+++ b/gdb/f-exp.y
@@ -1736,8 +1736,5 @@ f_language::parser (struct parser_state *par_state) const
 static void
 yyerror (const char *msg)
 {
-  if (pstate->prev_lexptr)
-    pstate->lexptr = pstate->prev_lexptr;
-
-  error (_("A %s in expression, near `%s'."), msg, pstate->lexptr);
+  pstate->parse_error (msg);
 }
diff --git a/gdb/go-exp.y b/gdb/go-exp.y
index c9b9c0b1ab7..561a3bef1b0 100644
--- a/gdb/go-exp.y
+++ b/gdb/go-exp.y
@@ -1545,8 +1545,5 @@ go_language::parser (struct parser_state *par_state) const
 static void
 yyerror (const char *msg)
 {
-  if (pstate->prev_lexptr)
-    pstate->lexptr = pstate->prev_lexptr;
-
-  error (_("A %s in expression, near `%s'."), msg, pstate->lexptr);
+  pstate->parse_error (msg);
 }
diff --git a/gdb/m2-exp.y b/gdb/m2-exp.y
index 092a8be248d..9a8767f5ac7 100644
--- a/gdb/m2-exp.y
+++ b/gdb/m2-exp.y
@@ -1006,8 +1006,5 @@ m2_language::parser (struct parser_state *par_state) const
 static void
 yyerror (const char *msg)
 {
-  if (pstate->prev_lexptr)
-    pstate->lexptr = pstate->prev_lexptr;
-
-  error (_("A %s in expression, near `%s'."), msg, pstate->lexptr);
+  pstate->parse_error (msg);
 }
diff --git a/gdb/p-exp.y b/gdb/p-exp.y
index b0f334897ad..9dfa8c5fd4f 100644
--- a/gdb/p-exp.y
+++ b/gdb/p-exp.y
@@ -1660,8 +1660,5 @@ pascal_language::parser (struct parser_state *par_state) const
 static void
 yyerror (const char *msg)
 {
-  if (pstate->prev_lexptr)
-    pstate->lexptr = pstate->prev_lexptr;
-
-  error (_("A %s in expression, near `%s'."), msg, pstate->lexptr);
+  pstate->parse_error (msg);
 }
diff --git a/gdb/parse.c b/gdb/parse.c
index b57d112fafd..53f8a761c40 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -51,6 +51,7 @@
 #include <algorithm>
 #include <optional>
 #include "c-exp.h"
+#include "cli/cli-style.h"
 
 static unsigned int expressiondebug = 0;
 static void
@@ -244,6 +245,21 @@ parser_state::push_dollar (struct stoken str)
     (create_internalvar (copy.c_str () + 1));
 }
 
+/* See parser-defs.h.  */
+
+void
+parser_state::parse_error (const char *msg)
+{
+  if (this->prev_lexptr)
+    this->lexptr = this->prev_lexptr;
+
+  error (_("A %s in expression near '<HERE>' marker in: `%.*s<HERE>%s'."),
+	 msg,
+	 ((int) (this->lexptr - this->start_of_input)),
+	 this->start_of_input,
+	 this->lexptr);
+}
+
 \f
 
 const char *
diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
index 93ebdf5c061..34673787ef0 100644
--- a/gdb/parser-defs.h
+++ b/gdb/parser-defs.h
@@ -152,6 +152,7 @@ struct parser_state : public expr_builder
       expression_context_block (context_block),
       expression_context_pc (context_pc),
       lexptr (input),
+      start_of_input (input),
       block_tracker (tracker),
       comma_terminates ((flags & PARSER_COMMA_TERMINATES) != 0),
       parse_completion (completion),
@@ -262,6 +263,11 @@ struct parser_state : public expr_builder
     push (expr::make_operation<T> (std::move (lhs), std::move (rhs)));
   }
 
+  /* Function called from various the various parsers yyerror functions to
+     throw an error.  The error will include a message identifying the
+     location of the error within the current expression.  */
+  void parse_error (const char *msg);
+
   /* If this is nonzero, this block is used as the lexical context for
      symbol names.  */
 
@@ -283,6 +289,9 @@ struct parser_state : public expr_builder
      Currently used only for error reporting.  */
   const char *prev_lexptr = nullptr;
 
+  /* A pointer to the start of the full input, used for error reporting.  */
+  const char *start_of_input = nullptr;
+
   /* Number of arguments seen so far in innermost function call.  */
 
   int arglist_len = 0;
diff --git a/gdb/testsuite/gdb.ada/bp_c_mixed_case.exp b/gdb/testsuite/gdb.ada/bp_c_mixed_case.exp
index d87b185c0c3..90a6dc867ee 100644
--- a/gdb/testsuite/gdb.ada/bp_c_mixed_case.exp
+++ b/gdb/testsuite/gdb.ada/bp_c_mixed_case.exp
@@ -83,11 +83,11 @@ gdb_test "continue" \
 # Try printing again using the "<...>" notation.  This shouldn't work
 # now, since the current frame is a C function.
 gdb_test "p <MixedCaseFunc>" \
-         "A syntax error in expression, near `<MixedCaseFunc>'\\." \
+         "A syntax error in expression near '<HERE>' marker in: `<HERE><MixedCaseFunc>'\\." \
          "p <MixedCaseFunc>, in C"
 
 gdb_test "p <NoDebugMixedCaseFunc>" \
-         "A syntax error in expression, near `<NoDebugMixedCaseFunc>'\\." \
+         "A syntax error in expression near '<HERE>' marker in: `<HERE><NoDebugMixedCaseFunc>'\\." \
          "p <NoDebugMixedCaseFunc>, in C"
 
 set test "break <MixedCaseFunc>, in C"
diff --git a/gdb/testsuite/gdb.base/exprs.exp b/gdb/testsuite/gdb.base/exprs.exp
index 79ae905fccf..d6773f8dfd9 100644
--- a/gdb/testsuite/gdb.base/exprs.exp
+++ b/gdb/testsuite/gdb.base/exprs.exp
@@ -275,3 +275,10 @@ gdb_test "print null_t_struct && null_t_struct->v_int_member == 0" \
 # Regression test for unusual function-call parse that caused a crash.
 gdb_test "print v_short++(97)" \
     "cast the call to its declared return type"
+
+# Some simple syntax errors in expressions.
+gdb_test "print v_short ==" \
+    "A syntax error in expression near '<HERE>' marker in: `v_short ==<HERE>'\\."
+
+gdb_test "print v_short =}{= 3" \
+    "A syntax error in expression near '<HERE>' marker in: `v_short =<HERE>}{= 3'\\."
diff --git a/gdb/testsuite/gdb.base/quit.exp b/gdb/testsuite/gdb.base/quit.exp
index 23418074c39..9cc4c91880d 100644
--- a/gdb/testsuite/gdb.base/quit.exp
+++ b/gdb/testsuite/gdb.base/quit.exp
@@ -19,7 +19,7 @@ clean_restart
 
 # Test that a syntax error causes quit to abort.
 # Regression test for PR gdb/20604.
-gdb_test "quit()" "A syntax error in expression, near .*" \
+gdb_test "quit()" "A syntax error in expression near '<HERE>' marker in: .*" \
     "quit with syntax error"
 
 # Test that an expression can be used to set the error code.
diff --git a/gdb/testsuite/gdb.base/settings.exp b/gdb/testsuite/gdb.base/settings.exp
index dc96f85c1bb..6943501a180 100644
--- a/gdb/testsuite/gdb.base/settings.exp
+++ b/gdb/testsuite/gdb.base/settings.exp
@@ -134,11 +134,11 @@ proc test-integer {variant} {
 
     # Valid value followed by garbage.
     gdb_test "$set_cmd 1 1" \
-	"A syntax error in expression, near `1'\\."
+	"A syntax error in expression near '<HERE>' marker in: `1 <HERE>1'\\."
 
     # Valid value followed by garbage.
     gdb_test "$set_cmd 1 x" \
-	"A syntax error in expression, near `x'\\."
+	"A syntax error in expression near '<HERE>' marker in: `1 <HERE>x'\\."
 
     if {$variant == "zuinteger-unlimited"} {
 	# -1 means unlimited.  Other negative values are rejected.  -1
diff --git a/gdb/testsuite/gdb.base/watch_thread_num.exp b/gdb/testsuite/gdb.base/watch_thread_num.exp
index 28b8581ba5a..b745f64fc14 100644
--- a/gdb/testsuite/gdb.base/watch_thread_num.exp
+++ b/gdb/testsuite/gdb.base/watch_thread_num.exp
@@ -38,7 +38,7 @@ if {![runto_main]} {
 }
 
 gdb_test "watch shared_var thread 0" "Invalid thread ID: 0" "watchpoint on invalid thread"
-gdb_test "watch shared_var thread" "A syntax error in expression, near `thread'\." "invalid watch syntax"
+gdb_test "watch shared_var thread" "A syntax error in expression near '<HERE>' marker in: `shared_var <HERE>thread'\." "invalid watch syntax"
 
 set bpexitline [gdb_get_line_number "all threads started"]
 gdb_breakpoint "$bpexitline"
diff --git a/gdb/testsuite/gdb.cp/local-static.exp b/gdb/testsuite/gdb.cp/local-static.exp
index 8d967c4b293..5ccf72081a0 100644
--- a/gdb/testsuite/gdb.cp/local-static.exp
+++ b/gdb/testsuite/gdb.cp/local-static.exp
@@ -20,7 +20,7 @@
 standard_testfile .c
 
 # A few expected errors.
-set syntax_re "A syntax error in expression, near.*"
+set syntax_re "A syntax error in expression near '<HERE>' marker in: .*"
 set cannot_resolve_re "Cannot resolve method S::method to any overloaded instance"
 
 # Build an "Cannot resolve method ..." expected error string for
diff --git a/gdb/testsuite/gdb.dlang/expression.exp b/gdb/testsuite/gdb.dlang/expression.exp
index c7aeec52d25..3a4af396041 100644
--- a/gdb/testsuite/gdb.dlang/expression.exp
+++ b/gdb/testsuite/gdb.dlang/expression.exp
@@ -110,7 +110,7 @@ proc test_d_expressions {} {
     # Test expression behaviour specific to D.
 
     # Comparison and order expressions have same precedence.
-    gdb_test "print 1 == 2 > 0" "A syntax error in expression, near `> 0'\."
+    gdb_test "print 1 == 2 > 0" "A syntax error in expression near '<HERE>' marker in: `1 == 2 <HERE>> 0'\."
     gdb_test "print (1 == 2) > 0" " = false"
 
     # Exponent expressions
-- 
2.25.4