Re: [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles

[Simon Marchi <simon.marchi@polymtl.ca>]

I did not do a full review, since Lancelot's comments were already a
good start.  I just added some complements to his comments.

>>  /* See arch-utils.h.  */
>>  void
>> -default_read_core_file_mappings (struct gdbarch *gdbarch,
>> -				 struct bfd *cbfd,
>> -				 gdb::function_view<void (ULONGEST count)>
>> -				   pre_loop_cb,
>> -				 gdb::function_view<void (int num,
>> -							  ULONGEST start,
>> -							  ULONGEST end,
>> -							  ULONGEST file_ofs,
>> -							  const char *filename)>
>> -				   loop_cb)
>> +default_read_core_file_mappings
>> + (struct gdbarch *gdbarch,
>> +  struct bfd *cbfd,
>> +  gdb::function_view<void (ULONGEST count)> pre_loop_cb,
>> +  gdb::function_view<void (int num,
>> +			   ULONGEST start,
>> +			   ULONGEST end,
>> +			   ULONGEST file_ofs,
>> +			   const char *filename,
>> +			   const bfd_build_id *build_id)>
>> +  loop_cb)
> 
> It looks like 'loop_cb' could go on the previous line.
> 
> If the type of the function callbacks are too big, I guess it could be
> possible to give them a name before declaring the function.  Something
> like
> 
>     using loop_cb_ftype = gdb::function_view<void (...)>;

Agreed, and giving a name to the function type helps readability too.

>> @@ -282,6 +282,16 @@ core_target::build_file_mappings ()
>>  
>>  	/* Set target_section fields.  */
>>  	m_core_file_mappings.emplace_back (start, end, sec);
>> +
>> +	/* If this is a bfd of a shared library, record its soname
>> +	   and build id.  */
>> +	if (build_id != nullptr)
>> +	  {
>> +	    gdb::optional<std::string> soname = gdb_bfd_read_elf_soname (bfd);
>> +	    if (soname)
>> +	      current_program_space->set_cbfd_soname_build_id (soname->data (),
>> +							       build_id);
> 
> Here, since set_cbfd_soname_build_id's first argument is a std::string,
> you could just use '*soname' instead of 'soname->data ()'.

Since the caller (here) doesn't need to keep the string, I would suggest
`std::move (*soname)` (see below).

>> @@ -358,6 +359,41 @@ print_program_space (struct ui_out *uiout, int requested)
>>      }
>>  }
>>  
>> +/* See progspace.h.  */
>> +
>> +void
>> +program_space::set_cbfd_soname_build_id (std::string soname,
> 
> This parameter could be 'std::string const &' or...
> 
>> +					 const bfd_build_id *build_id)
>> +{
>> +  std::string build_id_hex = build_id_to_string (build_id);
>> +  cbfd_soname_to_build_id[soname] = build_id_hex;
> 
> ... use 'std::move (soname)' here.
> 
> I guess the more 'usual' approach would be to have the argument as a
> const reference (but to be honest, the implication of calling one more
> ctor and copying the soname is negligible, to say the least).

Since set_cbfd_soname_build_id needs to keep its own copy of the object,
I think it's better to take it by value like this, and have the caller
std::move its copy if it doesn't need to keep it around.  And have
set_cbfd_soname_build_id std::move it into the map:

  cbfd_soname_to_build_id[std::move (soname)] = build_id_hex;

This way, soname is never copied if it doesn't need to.  Taking by
const-reference means that set_cbfd_soname_build_id will have no choice
but to copy the string into the map.

And in fact, I would suggest doing:

  cbfd_soname_to_build_id[std::move (soname)]
    = build_id_to_string (build_id);

... to avoid copying the build_id_hex string.  Alternatively, you could
do:

  std::string build_id_hex = build_id_to_string (build_id);
  cbfd_soname_to_build_id[std::move (soname)] = std::move (build_id_hex);

... but I think the first version is fine.


>> +
>> +  return;
> 
> I am not sure if the GNU coding standard says something about this, but
> 'return;' as the last statement of a void function is redundant.

Indeed.

>> +}
>> +
>> +/* See progspace.h.  */
>> +
>> +const char *
>> +program_space::get_cbfd_soname_build_id (const char *soname)
> 
> With set_cbfd_soname_build_id using a std::string, I would find it more
> consistent to use std::string here also.  Any reason not to use it I
> missed?
> 
> You could use 'basename (soname.c_str ())' bellow.
> 
> The return type could also be 'const std::string *' (the map stores
> std::string internally), but keeping a const char * is pretty similar.

We wouldn't want to return an std::string, that would make an
unnecessary copy.  If returning a `const char *` doesn't make life
difficult for the caller, I think it's perfectly fine.  Previously, I
would have suggested returning string_view, because that abstracts how
the string is stored.  But that isn't right, since the string_view
interface doesn't require the view to be null-terminated (which we
usually need).  We would need something like cstring_view instead:

  http://open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1402r0.pdf

But in the mean time, I think `const char *` is fine.

>> +gdb_bfd_read_elf_soname (struct bfd *bfd)
>> +{
>> +  gdb_assert (bfd != nullptr);
>> +
>> +  gdb_bfd_ref_ptr abfd = gdb_bfd_open (bfd->filename, gnutarget);
>> +
>> +  if (abfd == nullptr)
>> +    return gdb::optional<std::string> ();
>> +
>> +  /* Check that bfd is an ET_DYN ELF file.  */
>> +  bfd_check_format (abfd.get (), bfd_object);

I asked this in my previous review, still applies here:

  What's the point of calling bfd_check_format without checking the
  result?  It looks like a function without side-effects.

>> +  if (!(bfd_get_file_flags (abfd.get ()) & DYNAMIC))
>> +    return gdb::optional<std::string> ();
>> +
>> +  /* Determine soname of shared library.  If found map soname to build-id.  */
>> +  CORE_ADDR idx;
>> +  if (!gdb_bfd_scan_elf_dyntag (DT_SONAME, abfd.get (), &idx, nullptr))
>> +    return gdb::optional<std::string> ();
>> +
>> +  struct bfd_section *dynstr = bfd_get_section_by_name (abfd.get (), ".dynstr");
>> +  if (dynstr == nullptr)
>> +    return gdb::optional<std::string> ();
>> +
>> +  /* Read the soname from the string table.  */
>> +  gdb::byte_vector dynstr_buf;
>> +  if (!gdb_bfd_get_full_section_contents (abfd.get (), dynstr, &dynstr_buf))
>> +    return gdb::optional<std::string> ();
>> +
>> +  return gdb::optional<std::string> ((char *)dynstr_buf.data () + idx);

Do we need bounds checking to protect against bad ELF files?  What if
DT_SONAME points outside .dynstr's size?  And ideally that a
null-terminator is found somewhere between idx and the end of the
section.  Otherwise, I could craft a .dynstr section that does not
contain a null terminator, that would make GDB read out of bounds.

> This will not change much, but you could cast to 'const char *' (this
> is the type the std::string constructor expects).

And to shorten things up a bit, you can drop "gdb::optional":

  return std::string ((const char *) dynstr_buf.data () + idx);

While at it, instead of returning "gdb::optional<std::string> ()" on
failure, you can simply "return {}".

Simon