* Re: [PATCH]tracepoint.c: Add conditionals num to find_matching_tracepoint
2011-08-14 13:41 ` Hui Zhu
@ 2011-08-16 9:28 ` Hui Zhu
2011-08-23 8:53 ` Hui Zhu
2011-08-23 23:24 ` Stan Shebs
2 siblings, 0 replies; 7+ messages in thread
From: Hui Zhu @ 2011-08-16 9:28 UTC (permalink / raw)
To: Stan Shebs, gdb-patches ml
On Sun, Aug 14, 2011 at 21:40, Hui Zhu <teawater@gmail.com> wrote:
> On Tue, Aug 9, 2011 at 07:02, Stan Shebs <stanshebs@earthlink.net> wrote:
>> On 8/7/11 9:28 AM, Hui Zhu wrote:
>>>
>>> Hi,
>>>
>>> I found that when I set some tracepoint to a same address, and use
>>> tsave. And use "target tfile " to open it. It will only one
>>> tracepoint available.
>>> And output some words like:
>>> Created tracepoint 1 for target's tracepoint 1 at 0x40050a.
>>> Assuming tracepoint 1 is same as target's tracepoint 2 at 0x40050a.
>>> Assuming tracepoint 1 is same as target's tracepoint 3 at 0x40050a.
>>>
>>> This is because find_matching_tracepoint didn't check the num.
>>
>>
>> The number is exactly the one property that tracepoint upload must never
>> consider when looking for matching tracepoints, since the numbers vary
>> depending on what the user has been doing during the current GDB session.
>> Addressing the FIXME will help the multiple-tracepoint case, although it's
>> kind of messy.
>>
>> There is another useful heuristic that would be easy to add, which is to
>> exclude matching on a tracepoint that has already been uploaded. Having
>> created tracepoint 1 from the uploaded info, it's never going to be the case
>> that uploaded tracepoints 2 and 3 are the same as 1. It might be as simple
>> as testing number_on_target, but beware that it might be nonzero due to
>> other trace runs or some such.
>>
>> Stan
>> stan@codesourcery.com
>>
>>>
>>> And I think the tracepoint have the same address is really helpful for
>>> user. Because we can set different condition to make tracepoint more
>>> powerful.
>>> So I make following patch.
>>>
>>> Please help me review it.
>>>
>>> Thanks,
>>> Hui
>>>
>>>
>>> 2011-08-08 Hui Zhu<teawater@gmail.com>
>>>
>>> * tracepoint.c (find_matching_tracepoint): Add number check.
>>
>>
Ping.
Thanks,
Hui
>
> Hi Stan,
>
> Thanks for your review.
>
> I make a new patch that check the condition according to your mail.
>
> Best,
> Hui
>
> 2011-08-14 Hui Zhu <teawater@gmail.com>
>
> * tracepoint.c (cond_string_is_same): New function.
> (find_matching_tracepoint): Add condition check
> by cond_string_is_same.
> ---
> tracepoint.c | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> --- a/tracepoint.c
> +++ b/tracepoint.c
> @@ -3091,6 +3091,22 @@ free_uploaded_tsvs (struct uploaded_tsv
> }
> }
>
> +static int
> +cond_string_is_same(char *str1, char *str2)
> +{
> + if (str1 == NULL || str2 == NULL)
> + {
> + if (str1 == str2)
> + return 1;
> + else
> + return 0;
> + }
> + if (strcmp (str1, str2))
> + return 0;
> +
> + return 1;
> +}
> +
> /* Look for an existing tracepoint that seems similar enough to the
> uploaded one. Enablement isn't compared, because the user can
> toggle that freely, and may have done so in anticipation of the
> @@ -3111,7 +3127,8 @@ find_matching_tracepoint (struct uploade
> if (b->type == utp->type
> && t->step_count == utp->step
> && t->pass_count == utp->pass
> - /* FIXME also test conditionals and actions. */
> + && cond_string_is_same (t->base.cond_string, utp->cond_string)
> + /* FIXME also test actions. */
> )
> {
> /* Scan the locations for an address match. */
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH]tracepoint.c: Add conditionals num to find_matching_tracepoint
2011-08-14 13:41 ` Hui Zhu
2011-08-16 9:28 ` Hui Zhu
@ 2011-08-23 8:53 ` Hui Zhu
2011-08-23 23:24 ` Stan Shebs
2 siblings, 0 replies; 7+ messages in thread
From: Hui Zhu @ 2011-08-23 8:53 UTC (permalink / raw)
To: Stan Shebs, gdb-patches ml
Ping.
Thanks,
Hui
On Sun, Aug 14, 2011 at 21:40, Hui Zhu <teawater@gmail.com> wrote:
> On Tue, Aug 9, 2011 at 07:02, Stan Shebs <stanshebs@earthlink.net> wrote:
>> On 8/7/11 9:28 AM, Hui Zhu wrote:
>>>
>>> Hi,
>>>
>>> I found that when I set some tracepoint to a same address, and use
>>> tsave. And use "target tfile " to open it. It will only one
>>> tracepoint available.
>>> And output some words like:
>>> Created tracepoint 1 for target's tracepoint 1 at 0x40050a.
>>> Assuming tracepoint 1 is same as target's tracepoint 2 at 0x40050a.
>>> Assuming tracepoint 1 is same as target's tracepoint 3 at 0x40050a.
>>>
>>> This is because find_matching_tracepoint didn't check the num.
>>
>>
>> The number is exactly the one property that tracepoint upload must never
>> consider when looking for matching tracepoints, since the numbers vary
>> depending on what the user has been doing during the current GDB session.
>> Addressing the FIXME will help the multiple-tracepoint case, although it's
>> kind of messy.
>>
>> There is another useful heuristic that would be easy to add, which is to
>> exclude matching on a tracepoint that has already been uploaded. Having
>> created tracepoint 1 from the uploaded info, it's never going to be the case
>> that uploaded tracepoints 2 and 3 are the same as 1. It might be as simple
>> as testing number_on_target, but beware that it might be nonzero due to
>> other trace runs or some such.
>>
>> Stan
>> stan@codesourcery.com
>>
>>>
>>> And I think the tracepoint have the same address is really helpful for
>>> user. Because we can set different condition to make tracepoint more
>>> powerful.
>>> So I make following patch.
>>>
>>> Please help me review it.
>>>
>>> Thanks,
>>> Hui
>>>
>>>
>>> 2011-08-08 Hui Zhu<teawater@gmail.com>
>>>
>>> * tracepoint.c (find_matching_tracepoint): Add number check.
>>
>>
>
> Hi Stan,
>
> Thanks for your review.
>
> I make a new patch that check the condition according to your mail.
>
> Best,
> Hui
>
> 2011-08-14 Hui Zhu <teawater@gmail.com>
>
> * tracepoint.c (cond_string_is_same): New function.
> (find_matching_tracepoint): Add condition check
> by cond_string_is_same.
> ---
> tracepoint.c | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> --- a/tracepoint.c
> +++ b/tracepoint.c
> @@ -3091,6 +3091,22 @@ free_uploaded_tsvs (struct uploaded_tsv
> }
> }
>
> +static int
> +cond_string_is_same(char *str1, char *str2)
> +{
> + if (str1 == NULL || str2 == NULL)
> + {
> + if (str1 == str2)
> + return 1;
> + else
> + return 0;
> + }
> + if (strcmp (str1, str2))
> + return 0;
> +
> + return 1;
> +}
> +
> /* Look for an existing tracepoint that seems similar enough to the
> uploaded one. Enablement isn't compared, because the user can
> toggle that freely, and may have done so in anticipation of the
> @@ -3111,7 +3127,8 @@ find_matching_tracepoint (struct uploade
> if (b->type == utp->type
> && t->step_count == utp->step
> && t->pass_count == utp->pass
> - /* FIXME also test conditionals and actions. */
> + && cond_string_is_same (t->base.cond_string, utp->cond_string)
> + /* FIXME also test actions. */
> )
> {
> /* Scan the locations for an address match. */
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH]tracepoint.c: Add conditionals num to find_matching_tracepoint
2011-08-14 13:41 ` Hui Zhu
2011-08-16 9:28 ` Hui Zhu
2011-08-23 8:53 ` Hui Zhu
@ 2011-08-23 23:24 ` Stan Shebs
2011-08-24 9:30 ` Hui Zhu
2 siblings, 1 reply; 7+ messages in thread
From: Stan Shebs @ 2011-08-23 23:24 UTC (permalink / raw)
To: Hui Zhu; +Cc: gdb-patches
On 8/14/11 6:40 AM, Hui Zhu wrote:
> [...]
> Hi Stan,
>
> Thanks for your review.
>
> I make a new patch that check the condition according to your mail.
>
> Best,
> Hui
>
> 2011-08-14 Hui Zhu<teawater@gmail.com>
>
> * tracepoint.c (cond_string_is_same): New function.
> (find_matching_tracepoint): Add condition check
> by cond_string_is_same.
> ---
> tracepoint.c | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> --- a/tracepoint.c
> +++ b/tracepoint.c
> @@ -3091,6 +3091,22 @@ free_uploaded_tsvs (struct uploaded_tsv
> }
> }
>
> +static int
> +cond_string_is_same(char *str1, char *str2)
Don't forget the all-important space! :-)
> +{
> + if (str1 == NULL || str2 == NULL)
> + {
> + if (str1 == str2)
> + return 1;
> + else
> + return 0;
This bit is a little simpler as just "return (str1 == str2);"
> + }
> + if (strcmp (str1, str2))
> + return 0;
> +
> + return 1;
Typically, I would write this as "return (strcmp (str1, str2) == 0);"
It would also be good to add a function header comment that this
function is heuristic and will miss the cases where the conditional is
semantically identical but differs in whitespace, such as "x == 0" vs
"x==0" - but that's generally OK, because it just results in an extra
tracepoint that is easily deleted.
OK to install with these changes, and thanks!
Stan
stan@codesourcery.com
> +}
> +
> /* Look for an existing tracepoint that seems similar enough to the
> uploaded one. Enablement isn't compared, because the user can
> toggle that freely, and may have done so in anticipation of the
> @@ -3111,7 +3127,8 @@ find_matching_tracepoint (struct uploade
> if (b->type == utp->type
> && t->step_count == utp->step
> && t->pass_count == utp->pass
> - /* FIXME also test conditionals and actions. */
> + && cond_string_is_same (t->base.cond_string, utp->cond_string)
> + /* FIXME also test actions. */
> )
> {
> /* Scan the locations for an address match. */
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH]tracepoint.c: Add conditionals num to find_matching_tracepoint
2011-08-23 23:24 ` Stan Shebs
@ 2011-08-24 9:30 ` Hui Zhu
0 siblings, 0 replies; 7+ messages in thread
From: Hui Zhu @ 2011-08-24 9:30 UTC (permalink / raw)
To: Stan Shebs; +Cc: gdb-patches
On Wed, Aug 24, 2011 at 07:24, Stan Shebs <stanshebs@earthlink.net> wrote:
> On 8/14/11 6:40 AM, Hui Zhu wrote:
>>
>> [...]
>> Hi Stan,
>>
>> Thanks for your review.
>>
>> I make a new patch that check the condition according to your mail.
>>
>> Best,
>> Hui
>>
>> 2011-08-14 Hui Zhu<teawater@gmail.com>
>>
>> * tracepoint.c (cond_string_is_same): New function.
>> (find_matching_tracepoint): Add condition check
>> by cond_string_is_same.
>> ---
>> tracepoint.c | 19 ++++++++++++++++++-
>> 1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> --- a/tracepoint.c
>> +++ b/tracepoint.c
>> @@ -3091,6 +3091,22 @@ free_uploaded_tsvs (struct uploaded_tsv
>> }
>> }
>>
>> +static int
>> +cond_string_is_same(char *str1, char *str2)
>
> Don't forget the all-important space! :-)
Sorry for forgot it.
>
>> +{
>> + if (str1 == NULL || str2 == NULL)
>> + {
>> + if (str1 == str2)
>> + return 1;
>> + else
>> + return 0;
>
> This bit is a little simpler as just "return (str1 == str2);"
My code is so ugly. Thanks for help me make it more better. :)
>
>> + }
>> + if (strcmp (str1, str2))
>> + return 0;
>> +
>> + return 1;
>
> Typically, I would write this as "return (strcmp (str1, str2) == 0);"
>
> It would also be good to add a function header comment that this function is
> heuristic and will miss the cases where the conditional is semantically
> identical but differs in whitespace, such as "x == 0" vs "x==0" - but that's
> generally OK, because it just results in an extra tracepoint that is easily
> deleted.
That is really a issue for us. I add a comment for it. Wish I can
get some time to make it better.
>
> OK to install with these changes, and thanks!
I made a new patch according to your patch and checked it in.
Thanks for your help.
Best,
Hui
>
> Stan
> stan@codesourcery.com
>
>> +}
>> +
>> /* Look for an existing tracepoint that seems similar enough to the
>> uploaded one. Enablement isn't compared, because the user can
>> toggle that freely, and may have done so in anticipation of the
>> @@ -3111,7 +3127,8 @@ find_matching_tracepoint (struct uploade
>> if (b->type == utp->type
>> && t->step_count == utp->step
>> && t->pass_count == utp->pass
>> - /* FIXME also test conditionals and actions. */
>> + && cond_string_is_same (t->base.cond_string, utp->cond_string)
>> + /* FIXME also test actions. */
>> )
>> {
>> /* Scan the locations for an address match. */
>>
>
>
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/tracepoint.c.diff?cvsroot=src&r1=1.230&r2=1.231
2011-08-24 Hui Zhu <teawater@gmail.com>
* tracepoint.c (cond_string_is_same): New function.
(find_matching_tracepoint): Add condition check
by cond_string_is_same.
===================================================================
RCS file: /cvs/src/src/gdb/tracepoint.c,v
retrieving revision 1.230
retrieving revision 1.231
diff -u -r1.230 -r1.231
--- src/gdb/tracepoint.c 2011/08/04 19:10:13 1.230
+++ src/gdb/tracepoint.c 2011/08/24 09:24:10 1.231
@@ -3091,6 +3091,19 @@
}
}
+/* FIXME this function is heuristic and will miss the cases where the
+ conditional is semantically identical but differs in whitespace,
+ such as "x == 0" vs "x==0". */
+
+static int
+cond_string_is_same (char *str1, char *str2)
+{
+ if (str1 == NULL || str2 == NULL)
+ return (str1 == str2);
+
+ return (strcmp (str1, str2) == 0);
+}
+
/* Look for an existing tracepoint that seems similar enough to the
uploaded one. Enablement isn't compared, because the user can
toggle that freely, and may have done so in anticipation of the
@@ -3111,7 +3124,8 @@
if (b->type == utp->type
&& t->step_count == utp->step
&& t->pass_count == utp->pass
- /* FIXME also test conditionals and actions. */
+ && cond_string_is_same (t->base.cond_string, utp->cond_string)
+ /* FIXME also test actions. */
)
{
/* Scan the locations for an address match. */
^ permalink raw reply [flat|nested] 7+ messages in thread