Re: [patch] Verify byte-by-byte if both files are the same on "remote:" [Re: [rfc] False separate debuginfo warning with "remote:"

["Ulrich Weigand" <uweigand@de.ibm.com>]

Jan Kratochvil wrote:
> On Fri, 07 Oct 2011 15:53:09 +0200, Ulrich Weigand wrote:
> > Normally, the "stat" check would still catch this situation. However, with
> > "remote:" access, "stat" is not implemented.
> > 
> > To fix this, either of the following two approaches could be employed:
> > - Implement "stat" for the "remote:" file access protocol (but this would
> >   imply extending the remote protocol, and wouldn't help with old gdbservers
> >   on the other side)
> > - Omit the potentially spurious warning if the remote protocol is used to
> >   access the file (but this would also omit the warning if we get a real
> >   debuginfo mismatch due to out-of-date debuginfo)
> > 
> > Any thoughs?  Am I missing another option here?
> 
> Is acceptable the way below?  It is only backward compatibility fallback for
> systems still not using both .build-id and the .debug suffix, any of those two
> fixes would suffice.
> 
> It is backward compatible with existing gdbserver so it can be used as
> a fallback.  If the performance hit is bad even for this fallback case all the
> other methods failed I would be for gdbserver protocol extension for
> these-two-files-are-the-same, used if both files are "remote:".

Right, that's a good solution.  If there are indeed performance issues,
I think we should then implement "stat" in the remote protocol, because:
- this already fits into the existing BFD callback structure, and
- it would provide correct file lengths, which some debug info readers
  (notably stabs) use as sanity checks to ensure references fall into
  the file

> 2011-10-10  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Fix separate debuginfo warning with "remote:" access.
> 	* symfile.c (get_file_crc): New function with the code moved from ...
> 	(separate_debug_file_exists): ... this function, specifically variables
> 	buffer and count.  New variable verified_as_different, set it.  Remove
> 	file_crc initialization.  Verify also if both files are not the same
> 	manually, if needed.

Just a couple comments on the patch itself:

> +/* Return 32-bit CRC for ABFD.  ABFD must have its file position at start.  */

From an API simplicity point of view, I'd somewhat prefer for the function to
rewind the file position itself instead of requiring the caller to do it ...

> @@ -1345,25 +1359,35 @@ separate_debug_file_exists (const char *name, unsigned long crc,
>       negatives.  */
>  
>    if (bfd_stat (abfd, &abfd_stat) == 0
> -      && bfd_stat (parent_objfile->obfd, &parent_stat) == 0
> -      && abfd_stat.st_dev == parent_stat.st_dev
> -      && abfd_stat.st_ino == parent_stat.st_ino
> -      && abfd_stat.st_ino != 0)
> +      && abfd_stat.st_ino != 0
> +      && bfd_stat (parent_objfile->obfd, &parent_stat) == 0)
>      {
> -      bfd_close (abfd);
> -      return 0;
> +      if (abfd_stat.st_dev == parent_stat.st_dev
> +	  && abfd_stat.st_ino == parent_stat.st_ino)
> +	{
> +	  bfd_close (abfd);
> +	  return 0;
> +	}
> +      verified_as_different = 1;
>      }
> +  else
> +    verified_as_different = 0;

So this no longer handles the st_ino == 0 case.  I think we still need to do
that, to cope with filesystems (e.g. on Windows?) where stat works, but does
not provide inode numbers ...  Two files with zero st_ino should not be
considered equal.

>    if (crc != file_crc)
>      {
> -      warning (_("the debug information found in \"%s\""
> -		 " does not match \"%s\" (CRC mismatch).\n"),
> -	       name, parent_objfile->name);
> +      /* If one (or both) the files are accessed for example the via "remote:"
> +	 gdbserver way it does not support the bfd_stat operation.  Verify
> +	 whether those two files are not the same manually.  */
> +      if (verified_as_different
> +	  || bfd_seek (parent_objfile->obfd, 0, SEEK_SET) != 0
> +	  || get_file_crc (parent_objfile->obfd) != crc)
> +	warning (_("the debug information found in \"%s\""
> +		   " does not match \"%s\" (CRC mismatch).\n"),
> +		 name, parent_objfile->name);

As another performance optimization, maybe it would make sense to cache
the parent's CRC (e.g. in the objfile) to avoid redundant computation?

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com