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

Ulrich Weigand
2011-10-10T13:48:21Z

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