From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26177 invoked by alias); 5 Nov 2011 13:14:40 -0000 Received: (qmail 26155 invoked by uid 22791); 5 Nov 2011 13:14:38 -0000 X-SWARE-Spam-Status: No, hits=-1.6 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; Sat, 05 Nov 2011 13:14:23 +0000 Received: from nat-jpt.mentorg.com ([192.94.33.2] helo=PR1-MAIL.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1RMg4k-0007NJ-P5 from Yao_Qi@mentor.com for gdb-patches@sourceware.org; Sat, 05 Nov 2011 06:14:22 -0700 Received: from [127.0.0.1] ([172.16.63.104]) by PR1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.1830); Sat, 5 Nov 2011 22:14:20 +0900 Message-ID: <4EB536C8.3090406@codesourcery.com> Date: Sat, 05 Nov 2011 13:14:00 -0000 From: Yao Qi User-Agent: Mozilla/5.0 (X11; Linux i686; rv:7.0.1) Gecko/20110930 Thunderbird/7.0.1 MIME-Version: 1.0 To: Pedro Alves CC: gdb-patches@sourceware.org Subject: Re: [patch/gdbserver] Fix a bug when setting two fast tracepoints at the same place References: <4EB41054.3070706@codesourcery.com> <201111041703.03822.pedro@codesourcery.com> In-Reply-To: <201111041703.03822.pedro@codesourcery.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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/msg00128.txt.bz2 On 11/05/2011 01:03 AM, Pedro Alves wrote: > 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) > OK. >> > { >> > - 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.) > Yes, this list should be taken care of when adding tracepoint while trace is running. Since there shouldn't be many tracepoints setting at the same address, so it is fine to me to iterate all tracepoints of the same address. >> > - /* 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. > OK, I misunderstood ctx.regspace. Fixed as you suggested. > Okay with these changes. Patch below is what I committed. -- Yao (齐尧) gdb/gdbserver: 2011-11-05 Yao Qi * tracepoint.c (gdb_collect): Loop over tracepoints of same address as TPOINT's. gdb/testsuite: 2011-11-05 Yao Qi * gdb.trace/trace-break.exp: Add test on setting two fast tracepoints at the same address. Index: gdb/gdbserver/tracepoint.c =================================================================== RCS file: /cvs/src/src/gdb/gdbserver/tracepoint.c,v retrieving revision 1.29 diff -u -r1.29 tracepoint.c --- gdb/gdbserver/tracepoint.c 2 Nov 2011 23:44:21 -0000 1.29 +++ gdb/gdbserver/tracepoint.c 5 Nov 2011 13:06:38 -0000 @@ -5480,14 +5480,9 @@ 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 ()); @@ -5497,30 +5492,49 @@ return; } - /* Test the condition if present, and collect if true. */ - if (tpoint->cond == NULL - || condition_true_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx, - tpoint)) + for (ctx.tpoint = tpoint; + ctx.tpoint != NULL && ctx.tpoint->address == tpoint->address; + ctx.tpoint = ctx.tpoint->next) { - collect_data_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx, - tpoint->address, tpoint); + if (!ctx.tpoint->enabled) + 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 (); + /* Multiple tracepoints of different types, such as fast tracepoint and + static tracepoint, can be set at the same address. */ + if (ctx.tpoint->type != tpoint->type) + continue; + + /* Test the condition if present, and collect if true. */ + if (ctx.tpoint->cond == NULL + || condition_true_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx, + ctx.tpoint)) + { + collect_data_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx, + ctx.tpoint->address, ctx.tpoint); + + /* 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 (); + break; + } + } + 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 (); + break; + } + } } } Index: gdb/testsuite/gdb.trace/trace-break.exp =================================================================== RCS file: /cvs/src/src/gdb/testsuite/gdb.trace/trace-break.exp,v retrieving revision 1.1 diff -u -r1.1 trace-break.exp --- gdb/testsuite/gdb.trace/trace-break.exp 31 Oct 2011 12:55:26 -0000 1.1 +++ gdb/testsuite/gdb.trace/trace-break.exp 5 Nov 2011 13:06:38 -0000 @@ -234,6 +234,7 @@ break_trace_same_addr_1 "ftrace" ${break_always_inserted} break_trace_same_addr_2 "trace" "ftrace" ${break_always_inserted} break_trace_same_addr_2 "ftrace" "trace" ${break_always_inserted} + break_trace_same_addr_2 "ftrace" "ftrace" ${break_always_inserted} break_trace_same_addr_3 "ftrace" ${break_always_inserted} break_trace_same_addr_4 "ftrace" ${break_always_inserted} }