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

[Will Hawkins <hawkinsw@obs.cr>]

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
> > >