[1/2] gdb: improve error reporting from expression parser
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Testing passed
|
Commit Message
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(-)
Comments
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.
@@ -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
@@ -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);
}
@@ -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);
}
@@ -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);
}
@@ -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);
}
@@ -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);
}
@@ -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);
}
@@ -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);
+}
+
const char *
@@ -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;
@@ -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"
@@ -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'\\."
@@ -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.
@@ -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
@@ -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"
@@ -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
@@ -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