From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10164 invoked by alias); 10 Oct 2011 13:48:21 -0000 Received: (qmail 9906 invoked by uid 22791); 10 Oct 2011 13:48:19 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,MSGID_FROM_MTA_HEADER,RP_MATCHES_RCVD,TW_BJ X-Spam-Check-By: sourceware.org Received: from mtagate3.uk.ibm.com (HELO mtagate3.uk.ibm.com) (194.196.100.163) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 10 Oct 2011 13:48:01 +0000 Received: from d06nrmr1307.portsmouth.uk.ibm.com (d06nrmr1307.portsmouth.uk.ibm.com [9.149.38.129]) by mtagate3.uk.ibm.com (8.13.1/8.13.1) with ESMTP id p9ADm013016424 for ; Mon, 10 Oct 2011 13:48:00 GMT Received: from d06av02.portsmouth.uk.ibm.com (d06av02.portsmouth.uk.ibm.com [9.149.37.228]) by d06nrmr1307.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p9ADlxJ42453670 for ; Mon, 10 Oct 2011 14:47:59 +0100 Received: from d06av02.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p9ADlw5I015220 for ; Mon, 10 Oct 2011 07:47:58 -0600 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with SMTP id p9ADlvOP015144; Mon, 10 Oct 2011 07:47:57 -0600 Message-Id: <201110101347.p9ADlvOP015144@d06av02.portsmouth.uk.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Mon, 10 Oct 2011 15:47:57 +0200 Subject: Re: [patch] Verify byte-by-byte if both files are the same on "remote:" [Re: [rfc] False separate debuginfo warning with "remote:" To: jan.kratochvil@redhat.com (Jan Kratochvil) Date: Mon, 10 Oct 2011 13:48:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org In-Reply-To: <20111010083738.GA3272@host1.jankratochvil.net> from "Jan Kratochvil" at Oct 10, 2011 10:37:38 AM MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-10/txt/msg00261.txt.bz2 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 > > 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