From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15102 invoked by alias); 14 Mar 2012 16:13:28 -0000 Received: (qmail 15091 invoked by uid 22791); 14 Mar 2012 16:13:26 -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; Wed, 14 Mar 2012 16:13:13 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 568A11C6B6E; Wed, 14 Mar 2012 12:13:12 -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 L3N2hr95WgA3; Wed, 14 Mar 2012 12:13:12 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 18E071C6B69; Wed, 14 Mar 2012 12:13:11 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id C3B7F145615; Wed, 14 Mar 2012 09:13:03 -0700 (PDT) Date: Wed, 14 Mar 2012 16:13:00 -0000 From: Joel Brobecker To: Roland Schwingel Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Add dll trampoline code handling for windows 64bit Message-ID: <20120314161303.GD2853@adacore.com> References: <4F609EB7.2020801@onevision.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4F609EB7.2020801@onevision.com> 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: 2012-03/txt/msg00486.txt.bz2 Hi Roland, Thanks for the patch. In addition to Tom's answer, I have a few minor comments. The real review will have to come from our Windows Maintainer for your patch to be approved... > 2012-03-14 Roland Schwingel > > * amd64-windows-tdep.c: #include "frame.h" > (amd64_windows_skip_trampoline_code): New function. > (amd64_windows_init_abi): Add trampoline registration. (missing period at the end of the first line). > +/* Stuff for WIN64 PE style DLL's but is pretty generic really. */ It's great that you thought about adding a description comment to document the new function, thank you! I just wish the description was a little more precise. In fact, what this function does should already be described in gdbarch.h. For gdbarch callback/"methods", we usually simply say: /* Implement the "skip_trampoline_code" gdbarch method. */ No need to repeat the hook documentation at each instance. > + struct gdbarch *gdbarch = get_frame_arch (frame); > + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); Formatting nit: We do not align parameters in GDB code. Also, the indentation is wrong. We use 2 spaces, not 4, so the whole function needs to be re-indented. > + /* check for jmp *(%rip) */ Also, the GNU Coding Style which GDB follows requires us to start sentences with a capital letter, and to end them with a period. (and 2 spaces after periods). I would format the comment above as: /* Check for "jmp *(%rip)". */ > + if (pc && read_memory_unsigned_integer (pc, 2, byte_order) == 0x25ff) > + { > + unsigned long indirect = > + read_memory_unsigned_integer (pc + 2, 4, byte_order); Can use use ULONGEST here, instead of "unsigned long"? > + CORE_ADDR destination; > + gdb_byte *pos, addr[8]; Same as above, please, no extra spacing to align the parameter names. > + read_memory (pc + indirect, addr, 8); > + pos = (gdb_byte *) &destination; > + pos[0] = addr[6]; > + pos[1] = addr[7]; > + pos[2] = addr[0]; > + pos[3] = addr[1]; > + pos[4] = addr[2]; > + pos[5] = addr[3]; > + pos[6] = addr[4]; > + pos[7] = addr[5]; > + > + return destination; Yeah, Tom's suggestion is a better suggestion. I think you are going to have endianness issues this way. You could use read_memory_typed_address as well, but it's a little more involved, and I don't think it's necessary here. But otherwise, to me, the latter is the function to be used for reading addresses from inferior memory. I am sorry if it feels like it's a lot of little rules. It is. But it should be easy to learn them and it allows us to have a consistent style for our code. Thanks, -- Joel