Re: [PATCH 4/4 v5] gdb/debuginfod: Add .debug_line downloading

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Aaron Merey <amerey@redhat.com> writes:

> -/* Read directory or file name entry format, starting with byte of
> -   format count entries, ULEB128 pairs of entry formats, ULEB128 of
> -   entries count and the entries themselves in the described entry
> -   format.  */
> +
> +/* Like read_formatted_entries but the .debug_line and .debug_line_str

This function is also called read_formatted_entries, so the comment is a
bit confusing to me. Perhaps start with "Like the other
read_formatted_entries, but ..."?

> +   are stored in LINE_BUFP and LINE_STR_DATA.  This is used for cases
> +   where these sections are read from separate files without necessarily
> +   having access to the entire debuginfo file they originate from.  */
>  
>  static void
> -read_formatted_entries (dwarf2_per_objfile *per_objfile, bfd *abfd,
> -			const gdb_byte **bufp, struct line_header *lh,
> -			unsigned int offset_size,
> -			void (*callback) (struct line_header *lh,
> -					  const char *name,
> -					  dir_index d_index,
> -					  unsigned int mod_time,
> -					  unsigned int length))
> +read_formatted_entries
> +  (bfd *parent_bfd, const gdb_byte **line_bufp,
> +   const gdb::array_view<const gdb_byte> line_str_data,
> +   struct line_header *lh,
> +   unsigned int offset_size,
> +   void (*callback) (struct line_header *lh,
> +		     const char *name,
> +		     dir_index d_index,
> +		     unsigned int mod_time,
> +		     unsigned int length))
>  {

<snip>

> @@ -166,36 +173,48 @@ read_formatted_entries (dwarf2_per_objfile *per_objfile, bfd *abfd,
>  	  switch (form)
>  	    {
>  	    case DW_FORM_string:
> -	      string.emplace (read_direct_string (abfd, buf, &bytes_read));
> +	      string.emplace (read_direct_string (parent_bfd, buf, &bytes_read));
>  	      buf += bytes_read;
>  	      break;
>  
>  	    case DW_FORM_line_strp:
>  	      {
> -		const char *str
> -		  = per_objfile->read_line_string (buf, offset_size);
> +		if (line_str_data.empty ())
> +		  error (_("Dwarf Error: DW_FORM_line_strp used without " \
> +			   "required section"));

Is error the correct response here? Or should the bad form just be
complained about and ignored?

> +		if (line_str_data.size () <= offset_size)

I don't understand this check. Shouldn't it be the following?

  if (line_str_data.size () <= str_offset)

> +		  error (_("Dwarf Error: DW_FORM_line_strp pointing outside " \
> +			   "of section .debug_line"));

IIUC, the message should say ".debug_line_str" instead of ".debug_line".
Also, same question about error vs complaint.

> +
> +		ULONGEST str_offset = read_offset (parent_bfd, buf, offset_size);
> +
> +		const char *str;
> +		if (str_buf[str_offset] == '\0')
> +		  str = nullptr;
> +		else
> +		  str = (const char *) (str_buf + str_offset);
>  		string.emplace (str);
>  		buf += offset_size;
> +		break;
>  	      }
> -	      break;

<snip>

> +gdb::array_view<const gdb_byte>
> +mapped_debug_line::read_debug_line_separate
> +  (char *filename, std::unique_ptr<index_cache_resource> *resource)

filename can be const char *.

> +{
> +  if (filename == nullptr)
> +    return {};
> +
> +  try
> +  {
> +    line_resource_mmap *mmap_resource
> +      = new line_resource_mmap (filename);
> +
> +    resource->reset (mmap_resource);
> +
> +    return gdb::array_view<const gdb_byte>
> +      ((const gdb_byte *) mmap_resource->mapping.get (),
> +       mmap_resource->mapping.size ());
> +  }
> +  catch (const gdb_exception &except)
> +  {
> +    exception_print (gdb_stderr, except);
> +  }
> +
> +  return {};
> +}
> +
> +/* See read.h.  */
> +
> +bool
> +mapped_debug_line::read_debug_line_from_debuginfod (objfile *objfile)
> +{
> +  const bfd_build_id *build_id = build_id_bfd_get (objfile->obfd.get ());
> +  if (build_id == nullptr)
> +    return false;
> +
> +  gdb::unique_xmalloc_ptr<char> line_path;
> +  scoped_fd line_fd = debuginfod_section_query (build_id->data,
> +						build_id->size,
> +						bfd_get_filename
> +						  (objfile->obfd.get ()),
> +						".debug_line",
> +						&line_path);
> +
> +  if (line_fd.get () < 0)
> +    return false;
> +
> +  gdb::unique_xmalloc_ptr<char> line_str_path;
> +  scoped_fd line_str_fd = debuginfod_section_query (build_id->data,
> +						    build_id->size,
> +						    bfd_get_filename
> +						      (objfile->obfd.get ()),
> +						    ".debug_line_str",
> +						    &line_str_path);
> +
> +  line_data = read_debug_line_separate (line_path.get (), &line_resource);
> +  line_str_data = read_debug_line_separate (line_str_path.get (),
> +					    &line_str_resource);
> +
> +  const gdb_byte *line_ptr = line_data.data ();
> +
> +  while (line_ptr < line_data.data () + line_data.size ())
> +    {
> +      line_header_up lh
> +	= dwarf_decode_line_header (objfile->obfd.get (),
> +				    line_data, line_str_data,
> +				    &line_ptr, false,
> +				    nullptr, nullptr);
> +      line_headers.emplace_back (lh.release ());

This line is destroying a line_header_up just to construct a new one to
put in the vector. I believe you can use std::move here instead to avoid
the extra work.

> +    }
> +
> +  return true;
> +}
> +#endif /* !HAVE_SYS_MMAN_H */

-- 
Thiago