From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 128414 invoked by alias); 27 Feb 2015 19:53:34 -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 128358 invoked by uid 89); 27 Feb 2015 19:53:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,MSGID_FROM_MTA_HEADER,SPF_SOFTFAIL,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: e34.co.us.ibm.com Received: from e34.co.us.ibm.com (HELO e34.co.us.ibm.com) (32.97.110.152) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 27 Feb 2015 19:53:31 +0000 Received: from /spool/local by e34.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 27 Feb 2015 12:53:29 -0700 Received: from d03dlp03.boulder.ibm.com (9.17.202.179) by e34.co.us.ibm.com (192.168.1.134) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Fri, 27 Feb 2015 12:53:27 -0700 Received: from b03cxnp08026.gho.boulder.ibm.com (b03cxnp08026.gho.boulder.ibm.com [9.17.130.18]) by d03dlp03.boulder.ibm.com (Postfix) with ESMTP id 80BD919D8040 for ; Fri, 27 Feb 2015 12:44:35 -0700 (MST) Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by b03cxnp08026.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t1RJrYcp46072028 for ; Fri, 27 Feb 2015 12:53:42 -0700 Received: from d03av02.boulder.ibm.com (localhost [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t1RJqsxT019813 for ; Fri, 27 Feb 2015 12:52:54 -0700 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d03av02.boulder.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with SMTP id t1RJqq7X018591; Fri, 27 Feb 2015 12:52:52 -0700 Message-Id: <201502271952.t1RJqq7X018591@d03av02.boulder.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Fri, 27 Feb 2015 20:52:27 +0100 Subject: Re: [PATCH 1/2] Fast tracepoint for powerpc64le To: cole945@gmail.com (Wei-cheng Wang), palves@redhat.com Date: Fri, 27 Feb 2015 19:53:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org In-Reply-To: <54E77725.2070707@gmail.com> from "Wei-cheng Wang" at Feb 21, 2015 02:04:21 AM MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15022719-0017-0000-0000-00000918DC71 X-SW-Source: 2015-02/txt/msg00838.txt.bz2 Wei-cheng Wang wrote: > These patches implement fast tracepoint for PowerPC64. > > The first part includes required porting for PowerPC64 (and 32-bit) target. > Including > * Install fast tracepoint jump pad > * Agent expression bytecode compilation for powerpc64 only. > For 32-bit, bytecode interpreter is used instead. > * IPA (libinproctrace.so) > * Implement required gdbarch hooks. > * Enable tracepoint testing for powerpc. Excellent! Thanks for working on this. I'm still looking at the actual patches, but let me reply right now to the extra questions you raise. Pedro, I'd also appreciate your comments on some of the gdbserver tracepoint.c issues ... > * collection.exp fails are DWARF issues. x86 failed too. > (https://sourceware.org/bugzilla/show_bug.cgi?id=15081) > * ftrace.exp: x86 has the same issue (KFAIL in x86) > (https://sourceware.org/bugzilla/show_bug.cgi?id=13808) > * no-attach-trace.exp: x86 has the same issue. > Tracepoint is not supported when target is `exec'. > I think this should be XFAIL? > * unavailable.exp: x86 has the same issue. For the time being, I think it's fine to fail on Power if we fail on x86 too. Maybe should mark the test KFAIL in that case, however ... > * tspeed.exp: This case is used to test whether fast tracepoints > are *faster* than regular tracepoints. The case itself uses > sys+user time to find a proper iteration count for measurement. > (quote: "Total test time should be between 2 and 5 seconds.") > However, in my environment, 2 seconds of sys+user time means > 2 minutes wall clock, so this case failed due to timeout. The tspeed.exp file already has: # Typically we need a little extra time for this test. set timeout 180 Is that still not enough? > * entry-values fails: The casea try to backtrace at > an inline-asm-inserted symbol without debug information. > The prologue analyzer is confused. OK. Maybe this can be fixed by enhancing the analyzer (but that can be done in a separate patch). > * tfind.exp: One of the tracepoint is inserted at > `*gdb_recursion_test'. It's not hit because local-entry is called > instead. The 18 FAILs are off-by-one error. That's really a testcase issue, we had similar problems with setting breakpoints on "*func" on powerpc64le. This patch contains examples how I handled it for breakpoints: https://sourceware.org/ml/gdb-patches/2014-01/msg01102.html This test case seem a bit more complicated, we may need to split it in two parts; one that uses a normal "trace gdb_recursion_test" without the "*", and possibly a second one that specifically tests that "trace *func" works, using a source file that makes sure to call func via a function pointers (as in step-bt.c). > The main reason why PowerPC64 big-endian doesn't work is > calling convention (function descriptors) issue. > When installing a tracepoint in inferior memory, gdbserver > asks the address of "gdb_collect" (and etc.) using qSymbol packet, > and it generate a sequence of instructions to calling that address. > However, gdb-client "return the start of code instead of > any data function descriptor." > See commenting in remote_check_symbols/remote.c, > https://sourceware.org/ml/gdb-patches/2007-06/msg00389.html > and gen_call() in this patch. > In order for powerpc64be to work, qSymbol packet should be > extend for function descriptors. This is annoying. This was done to support libthread_db in gdbserver. Unfortunately, there are a number of components involved here: - To support debugging multi-threaded inferiors, gdbserver links against libthread_db (provided by glibc). - At startup, gdbserver's thread_db_enable_reporting routine calls into libthread_db's td_ta_event_addr. - td_ta_event_addr calls back into gdbserver's ps_pglobal_lookup to retrieve the address of __nptl_create_event in the inferior - In order to implement ps_pglobal_lookup, gdbserver issues a qSymbol packet back to GDB. - GDB looks the symbol up in the symbol table, and sends a result packet to gdbserver. - ps_pglobal_lookup returns that address to td_ta_event_addr. - td_ta_event_addr returns that address to thread_db_enable_reporting. - thread_db_enable_reporting uses set_breakpoint_at to install a breakpoint at that address (i.e. the __nptl_create_event routine in the inferior). Now, the equivalent action happens in GDB itself when debugging natively. Here, the equivalent enable_thread_event_reporting routine calls td_ta_event_addr, which calls ps_pglobal_lookup, which does a regular symbol lookup, and returns the address back. Now it is enable_thread_event_reporting itself that translates this address into the function code address required to set a breakpoint at, in helper routine enable_thread_event: /* Set up the breakpoint. */ gdb_assert (exec_bfd); (*bp) = (gdbarch_convert_from_func_ptr_addr (target_gdbarch (), /* Do proper sign extension for the target. */ (bfd_get_sign_extend_vma (exec_bfd) > 0 ? (CORE_ADDR) (intptr_t) notify.u.bptaddr : (CORE_ADDR) (uintptr_t) notify.u.bptaddr), ¤t_target)); create_thread_event_breakpoint (target_gdbarch (), *bp); With gdbserver, however, remote.c already replies with the code address to the qSymbol command, and gdbserver passes the code address through td_ta_event_addr back to itself. It's really not correct to have ps_pglobal_lookup return different addresses (code vs. descriptor), depending on whether GDB or gdbserver calls it. It happens to work because libthread_db doesn't really look at the address except for passing it through, but it still all seems quite weird. So I guess there's two ways to fix this. One would be to change gdbserver to work more like GDB here. This would involve removing the descriptor->code address conversion in remote.c, and instead performing the conversion in gdbserver's thread_db_enable_reporting. Now, there is no gdbarch_convert_from_func_ptr_addr in gdbserver, so a similar mechanism would have to be invented there. (I guess this would mean a new target hook.) Fortunately, the only platform that uses function descriptors *and* supports libthread_db debugging in gdbserver is ppc64-linux, so we'd only have to add that new mechanim on this platform. This has the advantage that qSymbol could now be used to lookup function symbols and get the descriptor address as expected. On the other hand, this would mean an incompatible change in the remote protocol: if you used a new GDB together with an old gdbserver (or vice versa), thread debugging would stop working. However, I guess that could be fixed by having gdbserver request the new behavior from GDB by specifying a feature code. With old GDBs gdbserver would have to skip the descriptor->code conversion. The second alternative would be to extend qSymbol to support returning two different types of addresses for function symbols: the symbol value (i.e. function pointer value, i.e. descriptor on PPC64), and a code address suitable to set a breakpoint on function entry. This could be either by having gdbserver request one or the other via an additional flag on the qSymbol request, or else by GDB simply always returning both values in two fields. Again, this would be an incompatible protocol change that would need to be guarded by a qFeature check. In this case, gdbserver would use the "normal" symbol values for most purposes (e.g. tracepoint routine lookup), but would use the "code address" values to return from ps_pglobal_lookup. With old GDBs, gdbserver would disable fast tracepoint support on powerpc64. If we do that, then for consistency it might also be useful to pass code addresses to ps_pglobal_lookup in GDB itself. In addition, the "code address" could be changed to skip the local entry point prolog on powerpc64le to ensure that the breakpoint is set correctly. (This does not matter in practice since __nptl_create_event has no local entry point, but it would seem more fully correct in general.) Overall, the second alternative seems a bit better to me, but I'd certainly appreciate feedback on this ... > For powerpc32 to work, some data structure/function in tracepoint.c > need to be fixed. For example, > > * write_inferior_data_ptr should be fixed for big-endian. > If sizeof (CORE_ADDR) is larger than sizeof (void*), zeros are written. > BTW, I thnink write_inferior_data_pointer provides the same functionality > without this issue. I'm not sure why write_inferior_data_ptr is needed? This is odd, I don't see the point of this either. Of course, as the comment says, much of this stuff will break anyway if gdbserver is compiled differently than the inferior (e.g. a 64-bit gdbserver debugging a 32-bit inferior), because it assumes the structure layout is identical. However, if we do have a 32-bit gdbserver, then I don't see why it shouldn't be possible to debug a 32-bit inferior, just because CORE_ADDR is a 64-bit type ... > * Data structure layout between gdbserver and IPA is not consistent. > > There are two versions of tracepoint_action one for gdbserver, > and antoher for inferior (IPA side). > > - struct tracepoint_action > | { > | #ifndef IN_PROCESS_AGENT > | const struct tracepoint_action_ops *ops; > | - #endif > | | char type; > - - }; > > It is the base object for action objects. > > struct collect_memory_action > { > struct tracepoint_action base; <-- > { > const struct tracepoint_action_ops *ops; > - char type; > | } > | > | ULONGEST addr; > | ULONGEST len; > - int32_t basereg; > }; > > When gdbserver downloading the action object to inferior, > it copies the object from offsetof(type) to the end. > (See m_tracepoint_action_download/tracepoint.c for example) > Howevery, the object layouts may not be consistent between > the two versions (with or without ops fields.) > It depends the the alignment requirement of addr (first data member > after base object), and the padding of tracepoint_action. > > In this case, the distance from "type" to "addr" changes > > Wihtout ops with ops > 0 1 2 3 0 1 2 3 > 0 type| PADDING... 0 ops-------------| > 4 ................ 4 type|PADDING....| > 8 addr------------ 8 addr------------- > c ---------------| c ----------------| > 10 len------------- 10 len-------------- > 14 ---------------| 14 ----------------| > 18 basereg--------| 18 basereg---------| Ugh. That's a strange construct, and extremely dependent on alignment rules (as you noticed). I'm not really sure what the best way to fix this would be. My preference right now would be get rid of "ops" on the gdbserver side too, and just switch on "type" in the two places where the ops->send and ops->download routines are called right now. This makes the data structures the same on gdbserver and IPA, which simplifies downloading quite a bit. (Also, it keeps the data structure identical in IPA, which should avoid compatibility issues between versions.) Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com