From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6585 invoked by alias); 4 Nov 2011 17:03:25 -0000 Received: (qmail 6575 invoked by uid 22791); 4 Nov 2011 17:03:23 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 04 Nov 2011 17:03:09 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=EU1-MAIL.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1RMNAa-0006w0-HQ from pedro_alves@mentor.com for gdb-patches@sourceware.org; Fri, 04 Nov 2011 10:03:08 -0700 Received: from scottsdale.localnet ([172.16.63.104]) by EU1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.1830); Fri, 4 Nov 2011 17:03:05 +0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [patch/gdbserver] Fix a bug when setting two fast tracepoints at the same place Date: Fri, 04 Nov 2011 17:03:00 -0000 User-Agent: KMail/1.13.6 (Linux/2.6.38-12-generic; KDE/4.7.2; x86_64; ; ) Cc: Yao Qi References: <4EB41054.3070706@codesourcery.com> In-Reply-To: <4EB41054.3070706@codesourcery.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201111041703.03822.pedro@codesourcery.com> X-IsSubscribed: yes 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-11/txt/msg00119.txt.bz2 On Friday 04 November 2011 16:18:28, Yao Qi wrote: > The problem is that when setting two fast tracepoints at the same place, > only one works. When setting fast tracepoint at an address, a jump pad > is created to connect a jmp insn written at tracepoint place and > collection routine. Even multiple fast tracepoints are set at the same > address, there is only jump pad associated with this address. Inferior > will jump to gdb_collect from jump pad with the first tracepoint > information, rather than all tracepoints information at that address. > This is cause of this problem. Yeah, a known problem that we never got to fix. We talked about it in the context of the original private stub where later the gdbserver port was based on, and we worried about the extra instructions slowing down fast collection. Every instruction counts in this path, and speed was more important. But in the gdbserver case, I think we're fine. > @@ -5417,47 +5417,62 @@ gdb_collect (struct tracepoint *tpoint, unsigned char *regs) > if (!tracing) > return; > > - if (!tpoint->enabled) > - return; > - > ctx.base.type = fast_tracepoint; > ctx.regs = regs; > ctx.regcache_initted = 0; > - ctx.tpoint = tpoint; > > - /* Wrap the regblock in a register cache (in the stack, we don't > - want to malloc here). */ > - ctx.regspace = alloca (register_cache_size ()); > - if (ctx.regspace == NULL) > + ctx.tpoint = tpoint; > + for (; ctx.tpoint && ctx.tpoint->address == tpoint->address; > + ctx.tpoint = ctx.tpoint->next) Put the initialization where it belongs. Use explicit NULL as the surrounding code: + for (ctx.tpoint = tpoint; + ctx.tpoint != NULL && ctx.tpoint->address == tpoint->address; + ctx.tpoint = ctx.tpoint->next) > { > - trace_debug ("Trace buffer block allocation failed, skipping"); > - return; > - } > + if (!ctx.tpoint->enabled) > + continue; > > - /* Test the condition if present, and collect if true. */ > - if (tpoint->cond == NULL > - || condition_true_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx, > - tpoint)) > - { > - collect_data_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx, > - tpoint->address, tpoint); > + /* Multiple tracepoints of different types, such as fast tracepoint and > + static tracepoint, can be set at the same address. */ I was going to suggest sorting by type as well as address, but that won't play well with adding new tracepoints while the trace run is ongoing. ;-) (Splitting the single list into separate sorted lists one per type would work though, but that's a bigger change.) > + if (ctx.tpoint->type != tpoint->type) > + continue; > > - /* Note that this will cause original insns to be written back > - to where we jumped from, but that's OK because we're jumping > - back to the next whole instruction. This will go badly if > - instruction restoration is not atomic though. */ > - if (stopping_tracepoint > - || trace_buffer_is_full > - || expr_eval_result != expr_eval_no_error) > - stop_tracing (); > - } > - else > - { > - /* If there was a condition and it evaluated to false, the only > - way we would stop tracing is if there was an error during > - condition expression evaluation. */ > - if (expr_eval_result != expr_eval_no_error) > - stop_tracing (); > + /* Wrap the regblock in a register cache (in the stack, we don't > + want to malloc here). */ > + ctx.regspace = alloca (register_cache_size ()); > + if (ctx.regspace == NULL) > + { > + trace_debug ("Trace buffer block allocation failed, skipping"); > + continue; > + } This grows the stack at every iteration. We should do this only once. Please move it out of the loop. Okay with these changes. -- Pedro Alves