From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25985 invoked by alias); 4 Jul 2011 18:41:32 -0000 Received: (qmail 25975 invoked by uid 22791); 4 Jul 2011 18:41:31 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00 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; Mon, 04 Jul 2011 18:41:16 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 096FC2BB209; Mon, 4 Jul 2011 14:41:16 -0400 (EDT) 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 bmUUsx80Srcv; Mon, 4 Jul 2011 14:41:15 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id C2C032BB0C4; Mon, 4 Jul 2011 14:41:15 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id B3959145615; Mon, 4 Jul 2011 11:41:11 -0700 (PDT) Date: Mon, 04 Jul 2011 18:52:00 -0000 From: Joel Brobecker To: Fawzi Mohamed Cc: gdb-patches@sourceware.org, ext Tristan Gingold , Andr? P?nitz Subject: Re: [PATCH,gdb]: ensures that cie ptr of an fda is a cie Message-ID: <20110704184111.GP2407@adacore.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.20 (2009-06-14) 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-07/txt/msg00107.txt.bz2 > This increases the robustness of gdb, so I think it is worthwhile to add. Indeed. > 2011-07-04 Fawzi Mohamed > > * dwarf2-frame.c (decode_frame_entry, decode_frame_entry_1): ensure > that cie pointer really points to a cie I'm not a specialist of this area of the code, so I can't approve. However, I'm noticing some style issues (style is project-specific). Also, please make sure to use the '-p' switch when creating the diff, so that we can get a little more context for each hunk (see man diff). > +enum eh_frame_type { > + eh_cie_type_id = 1, > + eh_fde_type_id = 2, > + eh_cie_or_fde_type_id = 3 > +}; The open curly brace should be on the next line, and our indentation level is 2 characters, thus: enum eh_frame_type { eh_cie_type_id = 1, eh_fde_type_id = 2, eh_cie_or_fde_type_id = 3 }; Also, we try to document every type and every function that we add in GDB. And for types like this, it is often advantageous to also document each element (not sure in this case). You can have a look at struct frame_id in frame.h for an example of how we document our types. > static gdb_byte *decode_frame_entry (struct comp_unit *unit, gdb_byte *start, > int eh_frame_p, > struct dwarf2_cie_table *cie_table, > - struct dwarf2_fde_table *fde_table); > + struct dwarf2_fde_table *fde_table, > + enum eh_frame_type entry_type); The documentation of these function need to be augmented in order to describe what this extra parameter does. Same for all the other functions that you have modified. > + gdb_assert ((entry_type & eh_cie_type_id)!=0); Style: spaces around the != operator. Also, please provide a comment explaining why entry_type & eh_cie_type_id can never be zero. It might be obvious if already specified in the function documention, but might not be so obvious. In fact, what is the purpose of this assertion? It looks like you're validating the value of entry_type to make sure that it is either a eh_cie_type_id or a eh_cie_or_fde_type_id. This leads me to believe that the values inside eh_frame_type are meant to be bit flags. This would be much clearer with documentation and by using the following syntax: enum eh_frame_type { eh_cie_type_id = 1 << 0, eh_fde_type_id = 1 << 1, eh_cie_or_fde_type_id = eh_cie_type_id + eh_fde_type_id }; (I might also add an `unknown' value that's equal to zero, just to name that value). > + gdb_assert ((entry_type & eh_fde_type_id)!=0); > + Same comment. -- Joel