From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28830 invoked by alias); 11 Jan 2013 15:06:05 -0000 Received: (qmail 28797 invoked by uid 22791); 11 Jan 2013 15:06:02 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_NO,TW_BJ X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 11 Jan 2013 15:05:54 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id ADEB42E645; Fri, 11 Jan 2013 10:05:53 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id kQq4UHDtVl4r; Fri, 11 Jan 2013 10:05:53 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 3BBBB2E1B4; Fri, 11 Jan 2013 10:05:53 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 550DCC14D7; Fri, 11 Jan 2013 19:05:37 +0400 (RET) Date: Fri, 11 Jan 2013 15:06:00 -0000 From: Joel Brobecker To: Yao Qi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Don't check PST is NULL in read_symtab Message-ID: <20130111150537.GM6143@adacore.com> References: <1357869440-23451-1-git-send-email-yao@codesourcery.com> <20130111045233.GK6143@adacore.com> <50F025B5.2060203@codesourcery.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="KDt/GgjP6HVcx58l" Content-Disposition: inline In-Reply-To: <50F025B5.2060203@codesourcery.com> User-Agent: Mutt/1.5.21 (2010-09-15) 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: 2013-01/txt/msg00225.txt.bz2 --KDt/GgjP6HVcx58l Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-length: 1183 > IMO, we don't need an assertion to check PST, because the function is > used in this way, > > (*pst->read_symtab) (objfile, pst); I am fine without the assertion as well. But if we followed your argument, we would never need an assertion. For me, assertions achieve two goals: 1. Clearly document an expectation; 2. Cause a semi-friendly abortion, rather than a mysterious behavior or crash. As of today, the way this function is called indeed guarantees that PST is never NULL. But someone adding a call at a later date might introduce a bug and cause it to be called with PST == NULL... > 2013-01-11 Yao Qi > > * dwarf2read.c (dwarf2_psymtab_to_symtab): Code indent. Let's take an example to show you what I meant in my first suggestion. Attached is a bogus change I made a function in dwarf2read. I added an "else" around a large block of code. The first patch shows the actual change, with code reindentation. That's the real patch which would be checked in eventually - but it's barely readable. So, to better show the real changes, I also attach the result of "git diff/show -b", where whitespace-only changes are ignored. -- Joel --KDt/GgjP6HVcx58l Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="dwarf2read-actual.diff" Content-length: 2678 diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index e2088f1..d590248 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -1918,39 +1918,41 @@ dwarf2_read_section (struct objfile *objfile, struct dwarf2_section_info *info) if (dwarf2_section_empty_p (info)) return; + else + { + abfd = sectp->owner; - abfd = sectp->owner; + /* If the section has relocations, we must read it ourselves. + Otherwise we attach it to the BFD. */ + if ((sectp->flags & SEC_RELOC) == 0) + { + const gdb_byte *bytes = gdb_bfd_map_section (sectp, &info->size); - /* If the section has relocations, we must read it ourselves. - Otherwise we attach it to the BFD. */ - if ((sectp->flags & SEC_RELOC) == 0) - { - const gdb_byte *bytes = gdb_bfd_map_section (sectp, &info->size); + /* We have to cast away const here for historical reasons. + Fixing dwarf2read to be const-correct would be quite nice. */ + info->buffer = (gdb_byte *) bytes; + return; + } - /* We have to cast away const here for historical reasons. - Fixing dwarf2read to be const-correct would be quite nice. */ - info->buffer = (gdb_byte *) bytes; - return; - } + buf = obstack_alloc (&objfile->objfile_obstack, info->size); + info->buffer = buf; - buf = obstack_alloc (&objfile->objfile_obstack, info->size); - info->buffer = buf; + /* When debugging .o files, we may need to apply relocations; see + http://sourceware.org/ml/gdb-patches/2002-04/msg00136.html . + We never compress sections in .o files, so we only need to + try this when the section is not compressed. */ + retbuf = symfile_relocate_debug_section (objfile, sectp, buf); + if (retbuf != NULL) + { + info->buffer = retbuf; + return; + } - /* When debugging .o files, we may need to apply relocations; see - http://sourceware.org/ml/gdb-patches/2002-04/msg00136.html . - We never compress sections in .o files, so we only need to - try this when the section is not compressed. */ - retbuf = symfile_relocate_debug_section (objfile, sectp, buf); - if (retbuf != NULL) - { - info->buffer = retbuf; - return; + if (bfd_seek (abfd, sectp->filepos, SEEK_SET) != 0 + || bfd_bread (buf, info->size, abfd) != info->size) + error (_("Dwarf Error: Can't read DWARF data from '%s'"), + bfd_get_filename (abfd)); } - - if (bfd_seek (abfd, sectp->filepos, SEEK_SET) != 0 - || bfd_bread (buf, info->size, abfd) != info->size) - error (_("Dwarf Error: Can't read DWARF data from '%s'"), - bfd_get_filename (abfd)); } /* A helper function that returns the size of a section in a safe way. --KDt/GgjP6HVcx58l Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="dwarf2read-spaces_ignored.diff" Content-length: 733 diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index e2088f1..d590248 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -1918,7 +1918,8 @@ dwarf2_read_section (struct objfile *objfile, struct dwarf2_section_info *info) if (dwarf2_section_empty_p (info)) return; - + else + { abfd = sectp->owner; /* If the section has relocations, we must read it ourselves. @@ -1951,6 +1952,7 @@ dwarf2_read_section (struct objfile *objfile, struct dwarf2_section_info *info) || bfd_bread (buf, info->size, abfd) != info->size) error (_("Dwarf Error: Can't read DWARF data from '%s'"), bfd_get_filename (abfd)); + } } /* A helper function that returns the size of a section in a safe way. --KDt/GgjP6HVcx58l--