* [PATCH 0/2] Changes to error reporting from the expression parser @ 2023-12-23 19:56 Andrew Burgess 2023-12-23 19:56 ` [PATCH 1/2] gdb: improve error reporting from " Andrew Burgess ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Andrew Burgess @ 2023-12-23 19:56 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] gdb: improve error reporting from expression parser 2023-12-23 19:56 [PATCH 0/2] Changes to error reporting from the expression parser Andrew Burgess @ 2023-12-23 19:56 ` Andrew Burgess 2023-12-28 19:12 ` John Baldwin 2023-12-23 19:56 ` [PATCH 2/2] gdb: don't try to style content in error calls Andrew Burgess 2024-01-02 14:43 ` [PATCHv2 0/3] Changes to error reporting from the expression parser Andrew Burgess 2 siblings, 1 reply; 13+ messages in thread From: Andrew Burgess @ 2023-12-23 19:56 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] gdb: improve error reporting from expression parser 2023-12-23 19:56 ` [PATCH 1/2] gdb: improve error reporting from " Andrew Burgess @ 2023-12-28 19:12 ` John Baldwin 0 siblings, 0 replies; 13+ messages in thread From: John Baldwin @ 2023-12-28 19:12 UTC (permalink / raw) To: Andrew Burgess, gdb-patches 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] gdb: don't try to style content in error calls 2023-12-23 19:56 [PATCH 0/2] Changes to error reporting from the expression parser Andrew Burgess 2023-12-23 19:56 ` [PATCH 1/2] gdb: improve error reporting from " Andrew Burgess @ 2023-12-23 19:56 ` Andrew Burgess 2024-01-02 14:43 ` [PATCHv2 0/3] Changes to error reporting from the expression parser Andrew Burgess 2 siblings, 0 replies; 13+ messages in thread From: Andrew Burgess @ 2023-12-23 19:56 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCHv2 0/3] Changes to error reporting from the expression parser 2023-12-23 19:56 [PATCH 0/2] Changes to error reporting from the expression parser Andrew Burgess 2023-12-23 19:56 ` [PATCH 1/2] gdb: improve error reporting from " Andrew Burgess 2023-12-23 19:56 ` [PATCH 2/2] gdb: don't try to style content in error calls Andrew Burgess @ 2024-01-02 14:43 ` Andrew Burgess 2024-01-02 14:43 ` [PATCHv2 1/3] gdb: don't try to style content in error calls Andrew Burgess ` (3 more replies) 2 siblings, 4 replies; 13+ messages in thread From: Andrew Burgess @ 2024-01-02 14:43 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCHv2 1/3] gdb: don't try to style content in error calls 2024-01-02 14:43 ` [PATCHv2 0/3] Changes to error reporting from the expression parser Andrew Burgess @ 2024-01-02 14:43 ` Andrew Burgess 2024-01-03 19:50 ` John Baldwin 2024-01-02 14:43 ` [PATCHv2 2/3] gdb: merge error handling from different expression parsers Andrew Burgess ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: Andrew Burgess @ 2024-01-02 14:43 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv2 1/3] gdb: don't try to style content in error calls 2024-01-02 14:43 ` [PATCHv2 1/3] gdb: don't try to style content in error calls Andrew Burgess @ 2024-01-03 19:50 ` John Baldwin 0 siblings, 0 replies; 13+ messages in thread From: John Baldwin @ 2024-01-03 19:50 UTC (permalink / raw) To: Andrew Burgess, gdb-patches 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCHv2 2/3] gdb: merge error handling from different expression parsers 2024-01-02 14:43 ` [PATCHv2 0/3] Changes to error reporting from the expression parser Andrew Burgess 2024-01-02 14:43 ` [PATCHv2 1/3] gdb: don't try to style content in error calls Andrew Burgess @ 2024-01-02 14:43 ` Andrew Burgess 2024-01-02 15:01 ` Lancelot SIX 2024-01-02 14:43 ` [PATCHv2 3/3] gdb: improve error reporting from expression parser Andrew Burgess 2024-01-03 19:52 ` [PATCHv2 0/3] Changes to error reporting from the " John Baldwin 3 siblings, 1 reply; 13+ messages in thread From: Andrew Burgess @ 2024-01-02 14:43 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv2 2/3] gdb: merge error handling from different expression parsers 2024-01-02 14:43 ` [PATCHv2 2/3] gdb: merge error handling from different expression parsers Andrew Burgess @ 2024-01-02 15:01 ` Lancelot SIX 2024-01-03 9:39 ` Andrew Burgess 0 siblings, 1 reply; 13+ messages in thread From: Lancelot SIX @ 2024-01-02 15:01 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv2 2/3] gdb: merge error handling from different expression parsers 2024-01-02 15:01 ` Lancelot SIX @ 2024-01-03 9:39 ` Andrew Burgess 0 siblings, 0 replies; 13+ messages in thread From: Andrew Burgess @ 2024-01-03 9:39 UTC (permalink / raw) To: Lancelot SIX; +Cc: gdb-patches 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 >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCHv2 3/3] gdb: improve error reporting from expression parser 2024-01-02 14:43 ` [PATCHv2 0/3] Changes to error reporting from the expression parser Andrew Burgess 2024-01-02 14:43 ` [PATCHv2 1/3] gdb: don't try to style content in error calls Andrew Burgess 2024-01-02 14:43 ` [PATCHv2 2/3] gdb: merge error handling from different expression parsers Andrew Burgess @ 2024-01-02 14:43 ` Andrew Burgess 2024-01-03 19:52 ` [PATCHv2 0/3] Changes to error reporting from the " John Baldwin 3 siblings, 0 replies; 13+ messages in thread From: Andrew Burgess @ 2024-01-02 14:43 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv2 0/3] Changes to error reporting from the expression parser 2024-01-02 14:43 ` [PATCHv2 0/3] Changes to error reporting from the expression parser Andrew Burgess ` (2 preceding siblings ...) 2024-01-02 14:43 ` [PATCHv2 3/3] gdb: improve error reporting from expression parser Andrew Burgess @ 2024-01-03 19:52 ` John Baldwin 2024-01-04 9:43 ` Andrew Burgess 3 siblings, 1 reply; 13+ messages in thread From: John Baldwin @ 2024-01-03 19:52 UTC (permalink / raw) To: Andrew Burgess, gdb-patches 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv2 0/3] Changes to error reporting from the expression parser 2024-01-03 19:52 ` [PATCHv2 0/3] Changes to error reporting from the " John Baldwin @ 2024-01-04 9:43 ` Andrew Burgess 0 siblings, 0 replies; 13+ messages in thread From: Andrew Burgess @ 2024-01-04 9:43 UTC (permalink / raw) To: John Baldwin, gdb-patches 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-01-04 9:43 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-12-23 19:56 [PATCH 0/2] Changes to error reporting from the expression parser Andrew Burgess 2023-12-23 19:56 ` [PATCH 1/2] gdb: improve error reporting from " Andrew Burgess 2023-12-28 19:12 ` John Baldwin 2023-12-23 19:56 ` [PATCH 2/2] gdb: don't try to style content in error calls Andrew Burgess 2024-01-02 14:43 ` [PATCHv2 0/3] Changes to error reporting from the expression parser Andrew Burgess 2024-01-02 14:43 ` [PATCHv2 1/3] gdb: don't try to style content in error calls Andrew Burgess 2024-01-03 19:50 ` John Baldwin 2024-01-02 14:43 ` [PATCHv2 2/3] gdb: merge error handling from different expression parsers Andrew Burgess 2024-01-02 15:01 ` Lancelot SIX 2024-01-03 9:39 ` Andrew Burgess 2024-01-02 14:43 ` [PATCHv2 3/3] gdb: improve error reporting from expression parser Andrew Burgess 2024-01-03 19:52 ` [PATCHv2 0/3] Changes to error reporting from the " John Baldwin 2024-01-04 9:43 ` Andrew Burgess
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).