[PATCH 0/2] Changes to error reporting from the expression parser

[PATCH 0/2] Changes to error reporting from the expression parser

[Andrew Burgess]

While working on another patch I wished that the error reporting from
the expression parser was better.  This series is my attempt to make
it better.

---

Andrew Burgess (2):
  gdb: improve error reporting from expression parser
  gdb: don't try to style content in error calls

 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/procfs.c                                |  6 ++----
 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 +-
 17 files changed, 49 insertions(+), 37 deletions(-)


base-commit: 316e74cec34cde2468e50aadafc62ce86d1971a6
-- 
2.25.4

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

[Andrew Burgess]

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

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

[John Baldwin]

On 12/23/23 11:56 AM, Andrew Burgess wrote:
> 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.

Hmm, I do kind of find the <HERE> approach somewhat verbose (or at least
I feel like it's a bit shouty).  I might prefer either your simple fix
for end of string (which I think is a perfectly fine fix), or the more
complicated multi-line error similar to what GCC/clang do for compile
errors.  It does sound like the multi-line case might be too complicated
to implement.

-- 
John Baldwin

[PATCH 2/2] gdb: don't try to style content in error calls

[Andrew Burgess]

Working on the previous commit I realised that we can't style output
in error calls.  I took a look and found one place where we do
currently try to style something within an error call, if this ever
triggers then it's just going to print a pointer rather than the
styled string.

Fixed by removing the styling.
---
 gdb/procfs.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/gdb/procfs.c b/gdb/procfs.c
index 1410bbc0d7d..0eafc2eddcc 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -605,10 +605,8 @@ static void
 proc_error (procinfo *pi, const char *func, int line)
 {
   int saved_errno = errno;
-  error ("procfs: %s line %d, %ps: %s",
-	 func, line, styled_string (file_name_style.style (),
-				    pi->pathname),
-	 safe_strerror (saved_errno));
+  error ("procfs: %s line %d, %s: %s",
+	 func, line, pi->pathname, safe_strerror (saved_errno));
 }
 
 /* Updates the status struct in the procinfo.  There is a 'valid'
-- 
2.25.4

[PATCHv2 0/3] Changes to error reporting from the expression parser

[Andrew Burgess]

In V2:

  - Have split the "merging of error handling" into its own patch,
    adding the new error handling is in its own patch.

  - Reordered patches, the minor bug fix is now first, follow by the
    refactor, with the new functionality placed in the last patch.

  - Have dropped the whole <HERE> marker idea, and gone with the
    simpler 'error at end of ....' style message.  This means that no
    tests need updating -- I have added a couple of new tests, but
    everything else should continue to pass as before.

---

While working on another patch I wished that the error reporting from
the expression parser was better.  This series is my attempt to make
it better.

---

Andrew Burgess (3):
  gdb: don't try to style content in error calls
  gdb: merge error handling from different expression parsers
  gdb: improve error reporting from expression parser

 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                      | 15 +++++++++++++++
 gdb/parser-defs.h                |  9 +++++++++
 gdb/procfs.c                     |  6 ++----
 gdb/testsuite/gdb.base/exprs.exp |  8 ++++++++
 11 files changed, 41 insertions(+), 29 deletions(-)


base-commit: 90827b4eefb06f6e0ab6cbac9eb94922e2cc8aee
-- 
2.25.4

[PATCHv2 1/3] gdb: don't try to style content in error calls

[Andrew Burgess]

While working on a later commit in this series I realised that the
error() function doesn't support output styling.  Due to the way that
output from error() calls is passed around within the exception
object and often combined with other output, it's not immediately
obvious to me if we should be trying to support styling in this
context or not.

On inspection, I found two places in GDB where we apparently try to
apply styling within the error() output, however, both of these places
are in infrequently used (and likely untested) code.

So, rather than try to implement styling in the error() output, right
now I'm proposing to just remove these two attempts to style error()
output.

This doesn't mean that someone shouldn't add error() styling in the
future, but right now, I'm not planning to do that, so I just wanted
to fix these two mistakes as I saw them.
---
 gdb/procfs.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/gdb/procfs.c b/gdb/procfs.c
index 1410bbc0d7d..0eafc2eddcc 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -605,10 +605,8 @@ static void
 proc_error (procinfo *pi, const char *func, int line)
 {
   int saved_errno = errno;
-  error ("procfs: %s line %d, %ps: %s",
-	 func, line, styled_string (file_name_style.style (),
-				    pi->pathname),
-	 safe_strerror (saved_errno));
+  error ("procfs: %s line %d, %s: %s",
+	 func, line, pi->pathname, safe_strerror (saved_errno));
 }
 
 /* Updates the status struct in the procinfo.  There is a 'valid'
-- 
2.25.4

Re: [PATCHv2 1/3] gdb: don't try to style content in error calls

[John Baldwin]

On 1/2/24 6:43 AM, Andrew Burgess wrote:
> While working on a later commit in this series I realised that the
> error() function doesn't support output styling.  Due to the way that
> output from error() calls is passed around within the exception
> object and often combined with other output, it's not immediately
> obvious to me if we should be trying to support styling in this
> context or not.
> 
> On inspection, I found two places in GDB where we apparently try to
> apply styling within the error() output, however, both of these places
> are in infrequently used (and likely untested) code.
> 
> So, rather than try to implement styling in the error() output, right
> now I'm proposing to just remove these two attempts to style error()
> output.
> 
> This doesn't mean that someone shouldn't add error() styling in the
> future, but right now, I'm not planning to do that, so I just wanted
> to fix these two mistakes as I saw them.
> ---
>   gdb/procfs.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/procfs.c b/gdb/procfs.c
> index 1410bbc0d7d..0eafc2eddcc 100644
> --- a/gdb/procfs.c
> +++ b/gdb/procfs.c
> @@ -605,10 +605,8 @@ static void
>   proc_error (procinfo *pi, const char *func, int line)
>   {
>     int saved_errno = errno;
> -  error ("procfs: %s line %d, %ps: %s",
> -	 func, line, styled_string (file_name_style.style (),
> -				    pi->pathname),
> -	 safe_strerror (saved_errno));
> +  error ("procfs: %s line %d, %s: %s",
> +	 func, line, pi->pathname, safe_strerror (saved_errno));
>   }
>   
>   /* Updates the status struct in the procinfo.  There is a 'valid'

One nit: the log mentions two places in GDB, but the patch seems to only
fix one such place?

-- 
John Baldwin

[PATCHv2 2/3] gdb: merge error handling from different expression parsers

[Andrew Burgess]

Many (all?) of the expression parsers implement yyerror to handle
parser errors, and all of these functions are basically identical.

This commit adds a new parser_state::parse_error() function, which
implements the common error handling code, this function can then be
called from all the different yyerror functions.

The benefit of this is that (in a future commit) I can improve the
error output, and all the expression parsers will benefit.

This commit is pure refactoring though, and so, there should be no
user visible changes after this commit.
---
 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       | 11 +++++++++++
 gdb/parser-defs.h |  5 +++++
 9 files changed, 23 insertions(+), 25 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..efac0dee1af 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -244,6 +244,17 @@ 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 `%s'."), msg, this->lexptr);
+}
+
 \f
 
 const char *
diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
index 93ebdf5c061..4d40245228b 100644
--- a/gdb/parser-defs.h
+++ b/gdb/parser-defs.h
@@ -262,6 +262,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.  */
 
-- 
2.25.4

Re: [PATCHv2 2/3] gdb: merge error handling from different expression parsers

[Lancelot SIX]

Hi Andrew,

Just a nit below

> diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
> index 93ebdf5c061..4d40245228b 100644
> --- a/gdb/parser-defs.h
> +++ b/gdb/parser-defs.h
> @@ -262,6 +262,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

I think the first "various" came from some previous wording and should
be dropped.

Best,
Lancelot.

> +     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.  */
>  
> -- 
> 2.25.4
> 

Re: [PATCHv2 2/3] gdb: merge error handling from different expression parsers

[Andrew Burgess]

Lancelot SIX <lsix@lancelotsix.com> writes:

> Hi Andrew,
>
> Just a nit below
>
>> diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
>> index 93ebdf5c061..4d40245228b 100644
>> --- a/gdb/parser-defs.h
>> +++ b/gdb/parser-defs.h
>> @@ -262,6 +262,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
>
> I think the first "various" came from some previous wording and should
> be dropped.

Fixed locally to:

  /* Function called from 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);

Thanks,
Andrew


>
> Best,
> Lancelot.
>
>> +     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.  */
>>  
>> -- 
>> 2.25.4
>> 

[PATCHv2 3/3] gdb: improve error reporting from expression parser

[Andrew Burgess]

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 first case is unchanged, but the second case now
reports like this:

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

Which I think is clearer.  There is a possible issue if the expression
being parsed is very long, GDB will repeat the whole expression.  But
this issue already exists in the standard case; if the error occurs
early in a long expression GDB will repeat everything after the syntax
error.  So I've not worried about this case in my new code either,
which keeps things simpler.

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.  At a minimum it would require some
significant work in order to make all our error handling multi-line
aware.

I've added a couple of extra tests in gdb.base/exprs.exp.
---
 gdb/parse.c                      | 6 +++++-
 gdb/parser-defs.h                | 4 ++++
 gdb/testsuite/gdb.base/exprs.exp | 8 ++++++++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/gdb/parse.c b/gdb/parse.c
index efac0dee1af..e8bb112177f 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -252,7 +252,11 @@ parser_state::parse_error (const char *msg)
   if (this->prev_lexptr)
     this->lexptr = this->prev_lexptr;
 
-  error (_("A %s in expression, near `%s'."), msg, this->lexptr);
+  if (*this->lexptr == '\0')
+    error (_("A %s in expression, near the end of `%s'."),
+	   msg, this->start_of_input);
+  else
+    error (_("A %s in expression, near `%s'."), msg, this->lexptr);
 }
 
 \f
diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
index 4d40245228b..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),
@@ -288,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.base/exprs.exp b/gdb/testsuite/gdb.base/exprs.exp
index 79ae905fccf..8c85b579b9d 100644
--- a/gdb/testsuite/gdb.base/exprs.exp
+++ b/gdb/testsuite/gdb.base/exprs.exp
@@ -275,3 +275,11 @@ 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"
+
+# Test for a syntax error at the end of an expression.
+gdb_test "print v_short + " \
+    "A syntax error in expression, near the end of `v_short \\+'\\."
+
+# Test for a syntax error in the middle of an expression.
+gdb_test "print v_short =}{= 3" \
+    "A syntax error in expression, near `\\}\\{= 3'\\."
-- 
2.25.4

Re: [PATCHv2 0/3] Changes to error reporting from the expression parser

[John Baldwin]

On 1/2/24 6:43 AM, Andrew Burgess wrote:
> In V2:
> 
>    - Have split the "merging of error handling" into its own patch,
>      adding the new error handling is in its own patch.
> 
>    - Reordered patches, the minor bug fix is now first, follow by the
>      refactor, with the new functionality placed in the last patch.
> 
>    - Have dropped the whole <HERE> marker idea, and gone with the
>      simpler 'error at end of ....' style message.  This means that no
>      tests need updating -- I have added a couple of new tests, but
>      everything else should continue to pass as before.
> 
> ---
> 
> While working on another patch I wished that the error reporting from
> the expression parser was better.  This series is my attempt to make
> it better.
> 
> ---
> 
> Andrew Burgess (3):
>    gdb: don't try to style content in error calls
>    gdb: merge error handling from different expression parsers
>    gdb: improve error reporting from expression parser
> 
>   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                      | 15 +++++++++++++++
>   gdb/parser-defs.h                |  9 +++++++++
>   gdb/procfs.c                     |  6 ++----
>   gdb/testsuite/gdb.base/exprs.exp |  8 ++++++++
>   11 files changed, 41 insertions(+), 29 deletions(-)
> 
> 
> base-commit: 90827b4eefb06f6e0ab6cbac9eb94922e2cc8aee

Modulo the one nit I mentioned for the first patch, (and with the
updated comment you noted in your reply to Lancelot SIX), these both
look good to me.

Approved-By: John Baldwin <jhb@FreeBSD.org>

-- 
John Baldwin

Re: [PATCHv2 0/3] Changes to error reporting from the expression parser

[Andrew Burgess]

John Baldwin <jhb@FreeBSD.org> writes:

> On 1/2/24 6:43 AM, Andrew Burgess wrote:
>> In V2:
>> 
>>    - Have split the "merging of error handling" into its own patch,
>>      adding the new error handling is in its own patch.
>> 
>>    - Reordered patches, the minor bug fix is now first, follow by the
>>      refactor, with the new functionality placed in the last patch.
>> 
>>    - Have dropped the whole <HERE> marker idea, and gone with the
>>      simpler 'error at end of ....' style message.  This means that no
>>      tests need updating -- I have added a couple of new tests, but
>>      everything else should continue to pass as before.
>> 
>> ---
>> 
>> While working on another patch I wished that the error reporting from
>> the expression parser was better.  This series is my attempt to make
>> it better.
>> 
>> ---
>> 
>> Andrew Burgess (3):
>>    gdb: don't try to style content in error calls
>>    gdb: merge error handling from different expression parsers
>>    gdb: improve error reporting from expression parser
>> 
>>   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                      | 15 +++++++++++++++
>>   gdb/parser-defs.h                |  9 +++++++++
>>   gdb/procfs.c                     |  6 ++----
>>   gdb/testsuite/gdb.base/exprs.exp |  8 ++++++++
>>   11 files changed, 41 insertions(+), 29 deletions(-)
>> 
>> 
>> base-commit: 90827b4eefb06f6e0ab6cbac9eb94922e2cc8aee
>
> Modulo the one nit I mentioned for the first patch, (and with the
> updated comment you noted in your reply to Lancelot SIX), these both
> look good to me.
>
> Approved-By: John Baldwin <jhb@FreeBSD.org>

I took another look at patch #1, I would have sworn I found two places
originally, but I can't find a second now .... not sure what happened
there.

Anyway, I updated the commit message to talk about a single problem
case, and pushed this series.

Thanks,
Andrew