From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30823 invoked by alias); 23 Jul 2009 19:46:10 -0000 Received: (qmail 30802 invoked by uid 22791); 23 Jul 2009 19:46:09 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mx2.redhat.com (HELO mx2.redhat.com) (66.187.237.31) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 23 Jul 2009 19:46:01 +0000 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n6NJjvYV023753; Thu, 23 Jul 2009 15:45:57 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx2.corp.redhat.com (8.13.1/8.13.1) with ESMTP id n6NJjuXZ022358; Thu, 23 Jul 2009 15:45:56 -0400 Received: from opsy.redhat.com (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 n6NJjtfs032560; Thu, 23 Jul 2009 15:45:55 -0400 Received: by opsy.redhat.com (Postfix, from userid 500) id 8F2B23782F5; Thu, 23 Jul 2009 11:42:22 -0600 (MDT) To: Reid Kleckner Cc: gdb-patches@sourceware.org, unladen-swallow@googlegroups.com Subject: Re: [RFA] Add interface for registering JITed code References: <9a9942200907221615o570e749fh5cb186c1600f159c@mail.gmail.com> From: Tom Tromey Reply-To: tromey@redhat.com Date: Thu, 23 Jul 2009 23:21:00 -0000 In-Reply-To: <9a9942200907221615o570e749fh5cb186c1600f159c@mail.gmail.com> (Reid Kleckner's message of "Wed\, 22 Jul 2009 16\:15\:38 -0700") Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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: 2009-07/txt/msg00589.txt.bz2 >>>>> "Reid" == Reid Kleckner writes: Reid> So that means we need LLVM to generate dwarf debug info, and we Reid> need to register it with GDB. Nice. Your overall approach seems good to me. [...] Reid> If LLVM frees machine code, then it sets the action enum in the Reid> descriptor to JIT_UNREGISTER, points the descriptor at the relevant Reid> code entry, and calls __jit_debug_register_code again. Given that this will be a supported way for GDB to connect to JITs, I think that the official interface (symbol names, types, enum values, etc) should be documented in the GDB manual somewhere. I wonder what happens if the ELF data in the inferior is corrupted. Perhaps there should be a way to disable the feature. What do you think? This patch also deserves an entry in NEWS. I have a few comments on the details of the patch itself. Reid> +struct breakpoint * Reid> +create_jit_event_breakpoint (CORE_ADDR address) Reid> +{ Reid> + struct breakpoint *b; Reid> + Reid> + b = create_internal_breakpoint (address, bp_jit_event); Reid> + update_global_location_list_nothrow (1); Reid> + return b; Reid> +} I am not sure but I suspect this should be called from breakpoint_re_set -- that is, follow the longjmp model a bit more. Does the current code work ok if LLVM is a .so linked into the application? I suspect, but do not know for sure, that in this situation gdb will not see the JIT symbols when the inferior-created hook is run.q There is also the somewhat more pathological case of a program with multiple JITs linked in. I'm on the fence about whether this is really needed. Reid> +/* This is a linked list mapping code entry addresses to objfiles. */ Reid> +static struct jit_obj_link *jit_objfiles = NULL; You can store arbitrary module-specific data in an objfile using the objfile_data API. This would be better because it is an already supported way of dealing with the lifetime of the associated data. This would mean getting rid of the linked list here and just looping over objfiles when needed. Reid> + error (_("Unable to read JIT descriptor from remote memory!\n")); Don't use \n at the end of error text. (There is code in gdb that does this but I think the rule is not to.) Reid> + descriptor->version = extract_unsigned_integer (&desc_buf[0], 4); You'll need to update to head ... a few of these functions have an extra argument now. I am not sure if using target_gdbarch is ok -- maybe passing the arch to jit_read_descriptor as an argument would be better. Maybe Ulrich could weigh in here. Reid> + descriptor->first_entry = extract_typed_address Reid> + (&desc_buf[8 + ptr_size], ptr_type); A formatting nit ... I think it is more typical to split before the '=' than before the '('. Reid> +static void Reid> +jit_register_code (CORE_ADDR entry_addr, struct jit_code_entry *code_entry) [...] Reid> + free (buffer); xfree, but... Reid> + objfile = symbol_file_add_from_local_memory (templ, buffer, size); Reid> + if (objfile == NULL) Reid> + free (buffer); symbol_file_add_from_local_memory can call error, so you need a cleanup here rather than explicit calls to free. Reid> + if (reg_addr == (CORE_ADDR)NULL) Reid> + return; Just write `0' instead of `(CORE_ADDR)NULL'. There are 2 instances of this. Reid> + if (err) return; Newline before the return. There are a few of these. Reid> + /* Check that the version number agrees with that we support. */ Reid> + if (descriptor.version != 1) Reid> + { Reid> + error (_("Unsupported JIT protocol version in descriptor!\n")); Reid> + return; error calls longjmp, so the return is redundant. There are a couple of these in the patch. Reid> +/* When the JIT breakpoint fires, the inferior wants us to take one of these Reid> + actions. */ Reid> + Reid> +typedef enum Reid> +{ Reid> + JIT_NOACTION = 0, Reid> + JIT_REGISTER, Reid> + JIT_UNREGISTER Reid> +} jit_actions_t; Please update the comment here to explain that these values are exported and used by the inferior, so they cannot be changed. Reid> + /* Hack to work around the fact that BFD does not take ownership of the Reid> + memory for files allocated in memory. */ Is it possible to fix this directly in BFD? Since... Reid> + bim = (struct bfd_in_memory *) objfile->obfd->iostream; ... this is definitely fishy :-) Reid> +struct objfile * Reid> +symbol_file_add_from_local_memory (struct bfd *templ, bfd_byte *buffer, Reid> + bfd_size_type size) Reid> +{ [...] Reid> + filename = xstrdup (""); Because this function can call error, you need a cleanup here. Tom