From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16565 invoked by alias); 28 Jul 2011 16:36:25 -0000 Received: (qmail 16405 invoked by uid 22791); 28 Jul 2011 16:36:23 -0000 X-SWARE-Spam-Status: No, hits=-7.5 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 28 Jul 2011 16:36:05 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p6SGa48H015374 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 28 Jul 2011 12:36:04 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p6SGa4Qo005319; Thu, 28 Jul 2011 12:36:04 -0400 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id p6SGa2X5009167; Thu, 28 Jul 2011 12:36:03 -0400 From: Tom Tromey To: Sanjoy Das Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 2/4] Populates jit-reader.h.in References: <1311523427-20501-1-git-send-email-sanjoy@playingwithpointers.com> <1311523427-20501-3-git-send-email-sanjoy@playingwithpointers.com> Date: Thu, 28 Jul 2011 16:40:00 -0000 In-Reply-To: <1311523427-20501-3-git-send-email-sanjoy@playingwithpointers.com> (Sanjoy Das's message of "Sun, 24 Jul 2011 21:33:45 +0530") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain 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/msg00791.txt.bz2 >>>>> "Sanjoy" == Sanjoy Das writes: Sanjoy> Adds the interface to be implemented by the JIT debug-info Sanjoy> readers and the API they can use to jit-reader.h.in. This looks pretty good. I like the comments quite a bit, thanks for making the extra effort here. Sanjoy> +/* Return status codes. */ Sanjoy> +enum { Sanjoy> + GDB_FAIL = 0, Sanjoy> + GDB_SUCCESS = 1 Sanjoy> +}; Give this enum a tag and then use it as the return type in those functions currently documented as returning GDB_FAIL or GDB_SUCCESS. Sanjoy> +#define GDB_MAX_REGISTER_SIZE 64 /* Mirrors the internal GDB definition. */ A concern I have is that we may bump this value in future versions of GDB. (IIUC, internally it is really just a convenience so we can allocate temporary arrays of the right size on the stack.) I think it would be good to try to address this problem before it bites. One way to do that would be to put the register size into one of the structures, rather than having a constant. E.g., there could be another field in gdb_reg_value; or maybe some other way. Sanjoy> + /* For internal use. */ Sanjoy> + void *private; It isn't clear whose internal use these fields are for -- gdb or the jit plugin? I suggest changing each one to read "For use by GDB" or "For use by the plugin" as appropriate. I didn't see anything in the patch series to actually wire up the JIT code, so it is hard to fully review this patch. (I vaguely recall seeing this in the earlier series -- but a resubmit should ideally be complete.) Sanjoy> +extern struct gdb_reader_funcs *gdb_init_reader (void); It seems to me that we could put the API version into the returned gdb_reader_funcs, to be checked by GDB. Then we don't need a special #define as in patch #1, just an ordinary version number. Tom