[v2] Make source.c:search_command_helper use source cache

  The current implementation of search_command_helper accesses the line
offsets of the current program spaces's source code through
the source cache but then accesses its contents through the disk. This
PR updates the implementation so that the access of the contents is also
through the source cache.

Tested on x86_64-redhat-linux.

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
---
    v1 -> v2:
        Remove leftover debugging code.

    Rationale:
        Supporting access through the source cache during searching will make it
        easier to implement the embedded source functionality of DW_LNCT_source.

 gdb/source.c | 79 +++++++++++++++++++++++++---------------------------
 1 file changed, 38 insertions(+), 41 deletions(-)
  

I am sorry for the PING. I know that I am supposed to wait for 2 weeks
before pinging (according to the Contributing documentation) but I am
nervous that I have not followed the proper procedures for sending the
patch.

Granting the premise that the patch is something that is useful, I am
sure that you will have feedback. I am more than willing to make any
changes to get it into shape.

As I said before, thank you for the work that you all do maintaining
such an incredible piece of software.

Will


On Mon, Mar 25, 2024 at 10:01 AM Will Hawkins <hawkinsw@obs.cr> wrote:
>
> The current implementation of search_command_helper accesses the line
> offsets of the current program spaces's source code through
> the source cache but then accesses its contents through the disk. This
> PR updates the implementation so that the access of the contents is also
> through the source cache.
>
> Tested on x86_64-redhat-linux.
>
> Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
> ---
>     v1 -> v2:
>         Remove leftover debugging code.
>
>     Rationale:
>         Supporting access through the source cache during searching will make it
>         easier to implement the embedded source functionality of DW_LNCT_source.
>
>  gdb/source.c | 79 +++++++++++++++++++++++++---------------------------
>  1 file changed, 38 insertions(+), 41 deletions(-)
>
> diff --git a/gdb/source.c b/gdb/source.c
> index bbeb4154258..ef042b0e96e 100644
> --- a/gdb/source.c
> +++ b/gdb/source.c
> @@ -1617,46 +1617,45 @@ search_command_helper (const char *regex, int from_tty, bool forward)
>    if (!source_open)
>      error (_("source code access disabled"));
>
> -  scoped_fd desc (open_source_file (loc->symtab ()));
> -  if (desc.get () < 0)
> -    perror_with_name (symtab_to_filename_for_display (loc->symtab ()),
> -                     -desc.get ());
> -
> -  int line = (forward
> -             ? last_line_listed + 1
> -             : last_line_listed - 1);
> -
>    const std::vector<off_t> *offsets;
> -  if (line < 1
> -      || !g_source_cache.get_line_charpos (loc->symtab (), &offsets)
> -      || line > offsets->size ())
> +  if (!g_source_cache.get_line_charpos (loc->symtab (), &offsets))
>      error (_("Expression not found"));
>
> -  if (lseek (desc.get (), (*offsets)[line - 1], 0) < 0)
> -    perror_with_name (symtab_to_filename_for_display (loc->symtab ()));
> +  std::string lines;
> +  if (!g_source_cache.get_source_lines (loc->symtab (), 1, offsets->size (),
> +                                       &lines))
> +    perror_with_name (symtab_to_filename_for_display (loc->symtab ()), -ENOENT);
> +
> +  int line_no = (forward
> +                ? last_line_listed + 1
> +                : last_line_listed - 1);
>
> -  gdb_file_up stream = desc.to_file (FDOPEN_MODE);
> -  clearerr (stream.get ());
> +  if (line_no < 1
> +      || line_no > offsets->size ())
> +    error (_("Expression not found"));
> +
> +  size_t current_line_start = (*offsets)[line_no - 1];
>
> -  gdb::def_vector<char> buf;
> -  buf.reserve (256);
> +  std::string buf;
>
>    while (1)
>      {
> -      buf.resize (0);
> -
> -      int c = fgetc (stream.get ());
> -      if (c == EOF)
> +      /* Invariant: current_line_start is either the index
> +        of the start of the current line in LINES *or* the
> +        length of the source code (LINES, when there is nothing
> +        else to do).  */
> +      if (current_line_start == lines.length ())
>         break;
> -      do
> -       {
> -         buf.push_back (c);
> -       }
> -      while (c != '\n' && (c = fgetc (stream.get ())) >= 0);
> +
> +      size_t current_line_end = ((line_no + 1) > offsets->size ()
> +                                ? lines.size () - 1
> +                                : (*offsets)[line_no] - 1);
> +
> +      size_t sz = current_line_end - current_line_start;
> +      buf = lines.substr (current_line_start, sz);
>
>        /* Remove the \r, if any, at the end of the line, otherwise
>          regular expressions that end with $ or \n won't work.  */
> -      size_t sz = buf.size ();
>        if (sz >= 2 && buf[sz - 2] == '\r')
>         {
>           buf[sz - 2] = '\n';
> @@ -1664,29 +1663,27 @@ search_command_helper (const char *regex, int from_tty, bool forward)
>         }
>
>        /* We now have a source line in buf, null terminate and match.  */
> -      buf.push_back ('\0');
> +      buf += '\0';
>        if (re_exec (buf.data ()) > 0)
>         {
>           /* Match!  */
> -         print_source_lines (loc->symtab (), line, line + 1, 0);
> -         set_internalvar_integer (lookup_internalvar ("_"), line);
> -         loc->set (loc->symtab (), std::max (line - lines_to_list / 2, 1));
> +         print_source_lines (loc->symtab (), line_no, line_no + 1, 0);
> +         set_internalvar_integer (lookup_internalvar ("_"), line_no);
> +         loc->set (loc->symtab (), std::max (line_no - lines_to_list / 2, 1));
>           return;
>         }
>
>        if (forward)
> -       line++;
> +       {
> +         line_no++;
> +         current_line_start = current_line_end + 1;
> +       }
>        else
>         {
> -         line--;
> -         if (line < 1)
> +         line_no--;
> +         if (line_no < 1)
>             break;
> -         if (fseek (stream.get (), (*offsets)[line - 1], 0) < 0)
> -           {
> -             const char *filename
> -               = symtab_to_filename_for_display (loc->symtab ());
> -             perror_with_name (filename);
> -           }
> +         current_line_start = (*offsets)[line_no - 1];
>         }
>      }
>
> --
> 2.44.0
>
  

Hello!

I hope that everyone is having a great Monday! If there is something
that I can do to make this patch better, please let me know. I am
sincerely enjoying the opportunity to contribute to GDB and I hope
that you find these patches helpful!

Sincerely,
Will

On Fri, Mar 29, 2024 at 10:46 AM Will Hawkins <hawkinsw@obs.cr> wrote:
>
> I am sorry for the PING. I know that I am supposed to wait for 2 weeks
> before pinging (according to the Contributing documentation) but I am
> nervous that I have not followed the proper procedures for sending the
> patch.
>
> Granting the premise that the patch is something that is useful, I am
> sure that you will have feedback. I am more than willing to make any
> changes to get it into shape.
>
> As I said before, thank you for the work that you all do maintaining
> such an incredible piece of software.
>
> Will
>
>
> On Mon, Mar 25, 2024 at 10:01 AM Will Hawkins <hawkinsw@obs.cr> wrote:
> >
> > The current implementation of search_command_helper accesses the line
> > offsets of the current program spaces's source code through
> > the source cache but then accesses its contents through the disk. This
> > PR updates the implementation so that the access of the contents is also
> > through the source cache.
> >
> > Tested on x86_64-redhat-linux.
> >
> > Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
> > ---
> >     v1 -> v2:
> >         Remove leftover debugging code.
> >
> >     Rationale:
> >         Supporting access through the source cache during searching will make it
> >         easier to implement the embedded source functionality of DW_LNCT_source.
> >
> >  gdb/source.c | 79 +++++++++++++++++++++++++---------------------------
> >  1 file changed, 38 insertions(+), 41 deletions(-)
> >
> > diff --git a/gdb/source.c b/gdb/source.c
> > index bbeb4154258..ef042b0e96e 100644
> > --- a/gdb/source.c
> > +++ b/gdb/source.c
> > @@ -1617,46 +1617,45 @@ search_command_helper (const char *regex, int from_tty, bool forward)
> >    if (!source_open)
> >      error (_("source code access disabled"));
> >
> > -  scoped_fd desc (open_source_file (loc->symtab ()));
> > -  if (desc.get () < 0)
> > -    perror_with_name (symtab_to_filename_for_display (loc->symtab ()),
> > -                     -desc.get ());
> > -
> > -  int line = (forward
> > -             ? last_line_listed + 1
> > -             : last_line_listed - 1);
> > -
> >    const std::vector<off_t> *offsets;
> > -  if (line < 1
> > -      || !g_source_cache.get_line_charpos (loc->symtab (), &offsets)
> > -      || line > offsets->size ())
> > +  if (!g_source_cache.get_line_charpos (loc->symtab (), &offsets))
> >      error (_("Expression not found"));
> >
> > -  if (lseek (desc.get (), (*offsets)[line - 1], 0) < 0)
> > -    perror_with_name (symtab_to_filename_for_display (loc->symtab ()));
> > +  std::string lines;
> > +  if (!g_source_cache.get_source_lines (loc->symtab (), 1, offsets->size (),
> > +                                       &lines))
> > +    perror_with_name (symtab_to_filename_for_display (loc->symtab ()), -ENOENT);
> > +
> > +  int line_no = (forward
> > +                ? last_line_listed + 1
> > +                : last_line_listed - 1);
> >
> > -  gdb_file_up stream = desc.to_file (FDOPEN_MODE);
> > -  clearerr (stream.get ());
> > +  if (line_no < 1
> > +      || line_no > offsets->size ())
> > +    error (_("Expression not found"));
> > +
> > +  size_t current_line_start = (*offsets)[line_no - 1];
> >
> > -  gdb::def_vector<char> buf;
> > -  buf.reserve (256);
> > +  std::string buf;
> >
> >    while (1)
> >      {
> > -      buf.resize (0);
> > -
> > -      int c = fgetc (stream.get ());
> > -      if (c == EOF)
> > +      /* Invariant: current_line_start is either the index
> > +        of the start of the current line in LINES *or* the
> > +        length of the source code (LINES, when there is nothing
> > +        else to do).  */
> > +      if (current_line_start == lines.length ())
> >         break;
> > -      do
> > -       {
> > -         buf.push_back (c);
> > -       }
> > -      while (c != '\n' && (c = fgetc (stream.get ())) >= 0);
> > +
> > +      size_t current_line_end = ((line_no + 1) > offsets->size ()
> > +                                ? lines.size () - 1
> > +                                : (*offsets)[line_no] - 1);
> > +
> > +      size_t sz = current_line_end - current_line_start;
> > +      buf = lines.substr (current_line_start, sz);
> >
> >        /* Remove the \r, if any, at the end of the line, otherwise
> >          regular expressions that end with $ or \n won't work.  */
> > -      size_t sz = buf.size ();
> >        if (sz >= 2 && buf[sz - 2] == '\r')
> >         {
> >           buf[sz - 2] = '\n';
> > @@ -1664,29 +1663,27 @@ search_command_helper (const char *regex, int from_tty, bool forward)
> >         }
> >
> >        /* We now have a source line in buf, null terminate and match.  */
> > -      buf.push_back ('\0');
> > +      buf += '\0';
> >        if (re_exec (buf.data ()) > 0)
> >         {
> >           /* Match!  */
> > -         print_source_lines (loc->symtab (), line, line + 1, 0);
> > -         set_internalvar_integer (lookup_internalvar ("_"), line);
> > -         loc->set (loc->symtab (), std::max (line - lines_to_list / 2, 1));
> > +         print_source_lines (loc->symtab (), line_no, line_no + 1, 0);
> > +         set_internalvar_integer (lookup_internalvar ("_"), line_no);
> > +         loc->set (loc->symtab (), std::max (line_no - lines_to_list / 2, 1));
> >           return;
> >         }
> >
> >        if (forward)
> > -       line++;
> > +       {
> > +         line_no++;
> > +         current_line_start = current_line_end + 1;
> > +       }
> >        else
> >         {
> > -         line--;
> > -         if (line < 1)
> > +         line_no--;
> > +         if (line_no < 1)
> >             break;
> > -         if (fseek (stream.get (), (*offsets)[line - 1], 0) < 0)
> > -           {
> > -             const char *filename
> > -               = symtab_to_filename_for_display (loc->symtab ());
> > -             perror_with_name (filename);
> > -           }
> > +         current_line_start = (*offsets)[line_no - 1];
> >         }
> >      }
> >
> > --
> > 2.44.0
> >
  

Hello!

As ever, thank you for the work that you all put into gdb. If the
patch below is something that you would consider accepting and there
are things that need to be improved, please just let me know!

Thank you, again!
Will


On Mon, Apr 8, 2024 at 2:12 PM Will Hawkins <hawkinsw@obs.cr> wrote:
>
> Hello!
>
> I hope that everyone is having a great Monday! If there is something
> that I can do to make this patch better, please let me know. I am
> sincerely enjoying the opportunity to contribute to GDB and I hope
> that you find these patches helpful!
>
> Sincerely,
> Will
>
> On Fri, Mar 29, 2024 at 10:46 AM Will Hawkins <hawkinsw@obs.cr> wrote:
> >
> > I am sorry for the PING. I know that I am supposed to wait for 2 weeks
> > before pinging (according to the Contributing documentation) but I am
> > nervous that I have not followed the proper procedures for sending the
> > patch.
> >
> > Granting the premise that the patch is something that is useful, I am
> > sure that you will have feedback. I am more than willing to make any
> > changes to get it into shape.
> >
> > As I said before, thank you for the work that you all do maintaining
> > such an incredible piece of software.
> >
> > Will
> >
> >
> > On Mon, Mar 25, 2024 at 10:01 AM Will Hawkins <hawkinsw@obs.cr> wrote:
> > >
> > > The current implementation of search_command_helper accesses the line
> > > offsets of the current program spaces's source code through
> > > the source cache but then accesses its contents through the disk. This
> > > PR updates the implementation so that the access of the contents is also
> > > through the source cache.
> > >
> > > Tested on x86_64-redhat-linux.
> > >
> > > Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
> > > ---
> > >     v1 -> v2:
> > >         Remove leftover debugging code.
> > >
> > >     Rationale:
> > >         Supporting access through the source cache during searching will make it
> > >         easier to implement the embedded source functionality of DW_LNCT_source.
> > >
> > >  gdb/source.c | 79 +++++++++++++++++++++++++---------------------------
> > >  1 file changed, 38 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/gdb/source.c b/gdb/source.c
> > > index bbeb4154258..ef042b0e96e 100644
> > > --- a/gdb/source.c
> > > +++ b/gdb/source.c
> > > @@ -1617,46 +1617,45 @@ search_command_helper (const char *regex, int from_tty, bool forward)
> > >    if (!source_open)
> > >      error (_("source code access disabled"));
> > >
> > > -  scoped_fd desc (open_source_file (loc->symtab ()));
> > > -  if (desc.get () < 0)
> > > -    perror_with_name (symtab_to_filename_for_display (loc->symtab ()),
> > > -                     -desc.get ());
> > > -
> > > -  int line = (forward
> > > -             ? last_line_listed + 1
> > > -             : last_line_listed - 1);
> > > -
> > >    const std::vector<off_t> *offsets;
> > > -  if (line < 1
> > > -      || !g_source_cache.get_line_charpos (loc->symtab (), &offsets)
> > > -      || line > offsets->size ())
> > > +  if (!g_source_cache.get_line_charpos (loc->symtab (), &offsets))
> > >      error (_("Expression not found"));
> > >
> > > -  if (lseek (desc.get (), (*offsets)[line - 1], 0) < 0)
> > > -    perror_with_name (symtab_to_filename_for_display (loc->symtab ()));
> > > +  std::string lines;
> > > +  if (!g_source_cache.get_source_lines (loc->symtab (), 1, offsets->size (),
> > > +                                       &lines))
> > > +    perror_with_name (symtab_to_filename_for_display (loc->symtab ()), -ENOENT);
> > > +
> > > +  int line_no = (forward
> > > +                ? last_line_listed + 1
> > > +                : last_line_listed - 1);
> > >
> > > -  gdb_file_up stream = desc.to_file (FDOPEN_MODE);
> > > -  clearerr (stream.get ());
> > > +  if (line_no < 1
> > > +      || line_no > offsets->size ())
> > > +    error (_("Expression not found"));
> > > +
> > > +  size_t current_line_start = (*offsets)[line_no - 1];
> > >
> > > -  gdb::def_vector<char> buf;
> > > -  buf.reserve (256);
> > > +  std::string buf;
> > >
> > >    while (1)
> > >      {
> > > -      buf.resize (0);
> > > -
> > > -      int c = fgetc (stream.get ());
> > > -      if (c == EOF)
> > > +      /* Invariant: current_line_start is either the index
> > > +        of the start of the current line in LINES *or* the
> > > +        length of the source code (LINES, when there is nothing
> > > +        else to do).  */
> > > +      if (current_line_start == lines.length ())
> > >         break;
> > > -      do
> > > -       {
> > > -         buf.push_back (c);
> > > -       }
> > > -      while (c != '\n' && (c = fgetc (stream.get ())) >= 0);
> > > +
> > > +      size_t current_line_end = ((line_no + 1) > offsets->size ()
> > > +                                ? lines.size () - 1
> > > +                                : (*offsets)[line_no] - 1);
> > > +
> > > +      size_t sz = current_line_end - current_line_start;
> > > +      buf = lines.substr (current_line_start, sz);
> > >
> > >        /* Remove the \r, if any, at the end of the line, otherwise
> > >          regular expressions that end with $ or \n won't work.  */
> > > -      size_t sz = buf.size ();
> > >        if (sz >= 2 && buf[sz - 2] == '\r')
> > >         {
> > >           buf[sz - 2] = '\n';
> > > @@ -1664,29 +1663,27 @@ search_command_helper (const char *regex, int from_tty, bool forward)
> > >         }
> > >
> > >        /* We now have a source line in buf, null terminate and match.  */
> > > -      buf.push_back ('\0');
> > > +      buf += '\0';
> > >        if (re_exec (buf.data ()) > 0)
> > >         {
> > >           /* Match!  */
> > > -         print_source_lines (loc->symtab (), line, line + 1, 0);
> > > -         set_internalvar_integer (lookup_internalvar ("_"), line);
> > > -         loc->set (loc->symtab (), std::max (line - lines_to_list / 2, 1));
> > > +         print_source_lines (loc->symtab (), line_no, line_no + 1, 0);
> > > +         set_internalvar_integer (lookup_internalvar ("_"), line_no);
> > > +         loc->set (loc->symtab (), std::max (line_no - lines_to_list / 2, 1));
> > >           return;
> > >         }
> > >
> > >        if (forward)
> > > -       line++;
> > > +       {
> > > +         line_no++;
> > > +         current_line_start = current_line_end + 1;
> > > +       }
> > >        else
> > >         {
> > > -         line--;
> > > -         if (line < 1)
> > > +         line_no--;
> > > +         if (line_no < 1)
> > >             break;
> > > -         if (fseek (stream.get (), (*offsets)[line - 1], 0) < 0)
> > > -           {
> > > -             const char *filename
> > > -               = symtab_to_filename_for_display (loc->symtab ());
> > > -             perror_with_name (filename);
> > > -           }
> > > +         current_line_start = (*offsets)[line_no - 1];
> > >         }
> > >      }
> > >
> > > --
> > > 2.44.0
> > >
  

>>>>> "Will" == Will Hawkins <hawkinsw@obs.cr> writes:

Will> The current implementation of search_command_helper accesses the line
Will> offsets of the current program spaces's source code through
Will> the source cache but then accesses its contents through the disk. This
Will> PR updates the implementation so that the access of the contents is also
Will> through the source cache.

It seems to me that this will result in some changes, because the source
cache may hold text that has had styling applied, and so may have
embedded ANSI escape sequences -- which the search command isn't
expecting.

If this is needed for your source-embedding work then perhaps the source
cache should store an unmodified copy of the source as well, at least in
the case that styles have been applied.

Tom
  

>>>>> "Will" == Will Hawkins <hawkinsw@obs.cr> writes:

Will> I am sorry for the PING. I know that I am supposed to wait for 2 weeks
Will> before pinging (according to the Contributing documentation) but I am
Will> nervous that I have not followed the proper procedures for sending the
Will> patch.

It's totally fine, don't worry about it at all.
Pinging is also fine, everybody is just pretty busy so reviews don't
always happen in a timely way unfortunately.  Pinging at least makes
sure it doesn't drop off the to-do list.

Tom
  

On Fri, Apr 19, 2024 at 3:41 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Will" == Will Hawkins <hawkinsw@obs.cr> writes:
>
> Will> I am sorry for the PING. I know that I am supposed to wait for 2 weeks
> Will> before pinging (according to the Contributing documentation) but I am
> Will> nervous that I have not followed the proper procedures for sending the
> Will> patch.
>
> It's totally fine, don't worry about it at all.

Thank you! I just didn't want to be a burden, that's all! It's been
fun working with you and Tom (so. many. Toms.) and I hope that I can
continue.

> Pinging is also fine, everybody is just pretty busy so reviews don't
> always happen in a timely way unfortunately.  Pinging at least makes
> sure it doesn't drop off the to-do list.
>
> Tom
  

On Fri, Apr 19, 2024 at 3:40 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Will" == Will Hawkins <hawkinsw@obs.cr> writes:
>
> Will> The current implementation of search_command_helper accesses the line
> Will> offsets of the current program spaces's source code through
> Will> the source cache but then accesses its contents through the disk. This
> Will> PR updates the implementation so that the access of the contents is also
> Will> through the source cache.
>
> It seems to me that this will result in some changes, because the source
> cache may hold text that has had styling applied, and so may have
> embedded ANSI escape sequences -- which the search command isn't
> expecting.

Interesting, I didn't notice that.

>
> If this is needed for your source-embedding work then perhaps the source
> cache should store an unmodified copy of the source as well, at least in
> the case that styles have been applied.

It is part of that work, yes. I hoped that by consolidating access to
the source code through the cache it would also streamline other parts
of the code. I will take your advise on storing a second copy and spin
another revision of the code.

Thank you for the feedback!
Will



>
> Tom