From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 35401 invoked by alias); 16 Mar 2016 17:55:18 -0000 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 Received: (qmail 35390 invoked by uid 89); 16 Mar 2016 17:55:17 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=H*r:192.168.43, Hx-spam-relays-external:sk:public-, H*r:sk:public-, H*RU:sk:public- X-HELO: xyzzy.0x04.net Received: from xyzzy.0x04.net (HELO xyzzy.0x04.net) (109.74.193.254) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 16 Mar 2016 17:55:13 +0000 Received: from hogfather.0x04.net (89-65-66-135.dynamic.chello.pl [89.65.66.135]) by xyzzy.0x04.net (Postfix) with ESMTPS id 069DE3FE63; Wed, 16 Mar 2016 18:56:11 +0100 (CET) Received: from [192.168.43.80] (public-gprs362901.centertel.pl [37.47.48.214]) by hogfather.0x04.net (Postfix) with ESMTPSA id C73C95800FD; Wed, 16 Mar 2016 18:55:09 +0100 (CET) Subject: Re: [PATCH 3/4 v2] gdbserver: Add powerpc fast tracepoint support. To: Ulrich Weigand References: <20160316165841.59F891537@oc7340732750.ibm.com> Cc: gdb-patches@sourceware.org From: =?UTF-8?Q?Marcin_Ko=c5=9bcielnicki?= Message-ID: <56E99DFA.7080206@0x04.net> Date: Wed, 16 Mar 2016 17:55:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <20160316165841.59F891537@oc7340732750.ibm.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2016-03/txt/msg00262.txt.bz2 On 16/03/16 17:58, Ulrich Weigand wrote: > Marcin Kościelnicki wrote: > >> I'm not sure how to properly determine whether ELFv1 or ELFv2 is in use >> by the target, so I assume it's the same ABI as in gdbserver itself. > > That's a generally sensible assumption. In principle, you can check > which ABI the target is using by checking the e_flags field in the > ELF header. The problem for gdbserver is that it usually doesn't > access the ELF file. However, the ELF header is mapped into target > memory, so could be checked there. Yet again, the *address* of the > ELF header is not very easily detectable without making assumptions > on how memory is layed out. > > The best bet would probably be to determine the AT_PHDR auxillary vector, > which gives you the address of the *program headers* for the main > executable in the target, and then round that address down to the page > size boundary. This should give you the address of the ELF header, > when making the assumption that the linker places program headers > in the fist page, following the ELF header. This is not necessarily > guaranteed by the ELF format, but is what all linkers happen to do. I'll try to do that. Seems like a bit of work, but it's only called once per installed pad / emitted expression, so should be OK. > >> Allocating the jump pad buffer needs a mention. On powerpc64, IPA simply >> scans from 0x10000000 (the executable load address) downwards until it >> find a free address. However, on 32-bit, ld.so loads dynamic libraries >> there, and they can easily cause us to exceed the 32MiB branch range - >> so, instead I aim right after the executable, at sbrk(0). This will >> of course cause further memory allocation via brk to fail, but glibc >> transparently falls back to mmap in this case, so that's not a problem. >> Of course, given a program with large data/bss section, this might >> exceed the branch range as well, so perhaps some better heuristic is >> in order here. > > These heuristics seem reasonable to me. > >> This fixes get_raw_reg for 32-bit (ULONGEST was used to read the >> registers dumped by jump pad instead of unsigned long). Also slightly >> optimizes gen_limm and fixes a few comments. > >> +alloc_jump_pad_buffer (size_t size) >> +{ >> +#ifdef __powerpc64__ >> + uintptr_t addr; >> + int pagesize; >> + void *res; >> + >> + pagesize = sysconf (_SC_PAGE_SIZE); >> + if (pagesize == -1) >> + perror_with_name ("sysconf"); >> + >> + addr = 0x10000000 - size; > > It also would be preferable to dynamically determine the actual > load address, since 0x10000000 is just the default, which can > be changed e.g. via linker script or due to PIE address randomization. > > You might again use the AT_PHDR auxillary vector (which is what > Wei-cheng's patches did). Alright, I'll do that (and throw it in for aarch64 as well, code should be near-identical). > >> +/* Get the thread area address. This is used to recognize which >> + thread is which when tracing with the in-process agent library. We >> + don't read anything from the address, and treat it as opaque; it's >> + the address itself that we assume is unique per-thread. */ >> + >> +static int >> +ppc_get_thread_area (int lwpid, CORE_ADDR *addr) >> +{ >> +#ifdef __powerpc64__ >> + struct lwp_info *lwp = find_lwp_pid (pid_to_ptid (lwpid)); >> + struct thread_info *thr = get_lwp_thread (lwp); >> + struct regcache *regcache = get_thread_regcache (thr, 1); >> + int is_64 = register_size (regcache->tdesc, 0) == 8; >> +#endif >> + long res; >> + >> +#ifdef __powerpc64__ >> + if (is_64) >> + res = ptrace (PTRACE_PEEKUSER, lwpid, (long) PT_R13 * sizeof (long), >> + (long) 0); >> + else >> +#endif >> + res = ptrace (PTRACE_PEEKUSER, lwpid, (long) PT_R2 * sizeof (long), >> + (long) 0); > > Was there a particular reason why you changed to using direct ptrace > calls instead of collecting the value from the regcache as in the > original patch? Not really, I just happened to had this bit written before I learned of Wei-cheng's patches. What are your preferences here? > >> +/* GEN_LOAD and GEN_STORE generate 64- or 32-bit load/store for ppc64 or ppc32 >> + respectively. They are primary used for save/restore GPRs in jump-pad, >> + not used for bytecode compiling. */ >> + >> +#if defined __powerpc64__ >> +#define GEN_LOAD(buf, rt, ra, si) (is_64 ? GEN_LD (buf, rt, ra, si) : \ >> + GEN_LWZ (buf, rt, ra, si)) >> +#define GEN_STORE(buf, rt, ra, si) (is_64 ? GEN_STD (buf, rt, ra, si) : \ >> + GEN_STW (buf, rt, ra, si)) > > Huh, using the is_64 variable from the surrounding routine without > passing it in as macro argument is probably not the best idea :-) I'll change that. > >> +#if _CALL_ELF == 2 >> + /* XXX is there a way to get this dynamically from the inferior? */ >> + int is_opd = 0; > > See above. > > > Otherwise, this looks good to me. > > Thanks, > Ulrich >