[PATCH v2 1/1] gdb, linespec: also error when explicit line BP is outside source file range

[Keith Seitz]

Hi,

Thank you for the patch. I quite like the push for better
user experience!

On 11/12/24 4:34 AM, Klaus Gerlicher wrote:
> From: "Gerlicher, Klaus" <klaus.gerlicher@intel.com>
> 
> Setting a breakpoint on an explicit source line outside
> of the source files line range informs the user that
> there is no compiled code at this line.

I apologize if this is nitpicky, but please don't use the
word "explicit." This word is used by linespec.c when referring
to explicit locations (as opposed to linespec locations).

So I read the title of this and assumed you were discussing
something like "break -line 12345". We're really dealing
with locations at a given or specified line number -- regardless
of them being specified with linespec or explicit locations. Please
adjust comments accordingly.

> This patch will then yield the following output:
> 
> (gdb) b 1
> No compiled code for line 1 in the current file.
> (gdb) b 534435345
> No source for line 534435345 in the current file.  File has 92 lines.
> (gdb) b file.cpp:32432
> No source for line 32432 in file "file.cpp".  File has 92 lines.

Very nice! That really does yield more clarity. [And thank you
for including before/after output in your commit message. It
really makes it much easier to understand what changes are being
proposed.]

> Note that depending on the GDB setting for breakpoint pending
> (set breakpoint pending on|off|auto), set to auto or off
> this will not create a pending breakpoint.  For breakpoint
> pending set to on a pending breakpoint will be created
> regardless of the line being in source file line range.

This seems consistent with how we've always done things, so
there are no surprises here.

> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index 528abc46b89..f1d0190a800 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -45,6 +45,7 @@
>   #include "gdbsupport/def-vector.h"
>   #include <algorithm>
>   #include "inferior.h"
> +#include "source-cache.h"
>   
>   /* An enumeration of the various things a user might attempt to
>      complete for a linespec location.  */
> @@ -2163,16 +2164,60 @@ create_sals_line_offset (struct linespec_state *self,
>   
>     if (values.empty ())
>       {
> +      std::string lines;
> +
> +      /* Try to find source code associated with the explicitly

Note: "explicitly" not needed/desired here.

> +	 specified line.  If the line is out of source file range
> +	 error out the same way as for no compiled code but with
> +	 a different message for clarity.  */
> +      bool line_found = false;
> +      int max_line = -1;
> +      for (const auto &symtab : ls->file_symtabs)
> +	{
> +	  if (!g_source_cache.get_source_lines (symtab, val.line,
> +						val.line, &lines))
> +	    {

I know this isn't particularly time-sensitive code, but if we can
prevent get_source_lines from doing unnecessary work, I would
prefer that we do that. That is, can we pass "nullptr" instead of
"&lines"? Seems like it should not be all that difficult (at quick
glance, at least).

> +	      const std::vector<off_t> *offsets = nullptr;
> +	      g_source_cache.get_line_charpos (symtab, &offsets);
> +
> +	      if (offsets == nullptr)
> +		continue;
> +
> +	      max_line = std::max (max_line, (int) offsets->size ());
> +	    }
> +	  else
> +	    {
> +	      line_found = true;
> +	      break;
> +	    }
> +	}
> +
>         if (ls->explicit_loc.source_filename)
> -	throw_error (NOT_FOUND_ERROR,
> -		     _("No compiled code for line %d in file \"%s\"."),
> -		     val.line, ls->explicit_loc.source_filename.get ());
> +	{
> +	  if (line_found)
> +	    throw_error (NOT_FOUND_ERROR,
> +			 _("No compiled code for line %d in file \"%s\"."),
> +			 val.line, ls->explicit_loc.source_filename.get ());
> +	  else
> +	    throw_error (NOT_FOUND_ERROR,
> +			 _("No source for line %d in file \"%s\".  "
> +			   "File has %d lines."),
> +			 val.line,
> +			 ls->explicit_loc.source_filename.get (),
> +			 max_line);
> +	}
>         else
> -	throw_error (NOT_FOUND_ERROR,
> -		     _("No compiled code for line %d in the current file."),
> -		     val.line);
> +	{
> +	  if (line_found)
> +	    throw_error (NOT_FOUND_ERROR,
> +			 _("No compiled code for line %d in the "
> +			   "current file."), val.line);
> +	  else
> +	    throw_error (NOT_FOUND_ERROR,
> +			 _("No source for line %d in the current file.  "
> +			   "File has %d lines."), val.line, max_line);
> +	}
>       }
> -
>     return values;
>   }
>   
> diff --git a/gdb/testsuite/gdb.base/break.exp b/gdb/testsuite/gdb.base/break.exp
> index 34ac21982ea..fdc28c8af92 100644
> --- a/gdb/testsuite/gdb.base/break.exp
> +++ b/gdb/testsuite/gdb.base/break.exp
> @@ -487,7 +487,7 @@ proc_with_prefix test_break_nonexistent_line {} {
>       # breakpoint on a nonexistent source line.
>       gdb_test_no_output "set breakpoint pending off"
>       gdb_test "break 999" \
> -	"^No compiled code for line 999 in the current file\\." \
> +	"^No source for line 999 in the current file.  File has \[0-9\]+ lines\\." \
>   	"break on non-existent source line"
>   }

BTW, I like the way you've dealt with this new output. This had the
potential to spiral out of control like source line numbers changing
in test source files (hence our use of gdb_get_line_number), but you
caught that gotcha before it even became an issue.

> diff --git a/gdb/testsuite/gdb.trace/tracecmd.exp b/gdb/testsuite/gdb.trace/tracecmd.exp
> index 688980c78f7..2d89cd35b38 100644
> --- a/gdb/testsuite/gdb.trace/tracecmd.exp
> +++ b/gdb/testsuite/gdb.trace/tracecmd.exp
> @@ -65,7 +65,7 @@ gdb_test "info trace" "in gdb_recursion_test.*$srcfile:$testline2.
>   gdb_delete_tracepoints
>   gdb_test_no_output "set breakpoint pending off"
>   gdb_test "trace $srcfile:99999" \
> -	"No compiled code for line 99999 in file \".*$srcfile\"\\." \
> +	"^No source for line 99999 in file \".*$srcfile\"\\.  File has 157 lines\\." \
>   	"1.2a: trace invalid line in sourcefile"
>   gdb_test "info trace" "No tracepoints.*" \
>   	"1.2b: reject invalid line in srcfile"

This one breaks from the above pattern, though. Is there a specific
reason for checking for "157 lines" or can this just use "\[0-9\]+"
as other tests do?

Keith