On 11/10/2011 11:51 PM, Pedro Alves wrote:
> On Tuesday 08 November 2011 06:30:17, Yao Qi wrote:
> 
>> @@ -10660,6 +10713,8 @@ update_global_location_list (int should_insert)
>>           || (gdbarch_has_global_breakpoints (target_gdbarch))))
>>      insert_breakpoint_locations ();
>>  
>> +  download_tracepoint_locations ();
> 
> Should be guarded by `should_insert'.
> 

Right.  Fixed.

>> -      /* Skip LOCP entries which will definitely never be needed.
>> +      /* skip LOCP entries which will definitely never be needed.
> 
> Drop this spurious change please.

Removed.
> 
>> @@ -10491,7 +10539,9 @@ update_global_location_list (int should_insert)
>>                              if it should be inserted in case it will be
>>                              unduplicated.  */
>>                           if (loc2 != old_loc
>> -                             && unduplicated_should_be_inserted (loc2))
>> +                             && unduplicated_should_be_inserted (loc2)
>> +                             && (is_tracepoint (old_loc->owner)
>> +                                 == is_tracepoint (loc2->owner)))
> 
> What if both are tracepoints, but of different kind?  I think this
> might be better handled within breakpoint_locations_match (called just above).
> 

I move this part into a new function tracepoint_locations_match, and
call it in breakpoint_locations_match.

> 
>>    /* Nonzero if this is not the first breakpoint in the list
>> -     for the given address.  */
>> +     for the given address.  In current implementation, location
>> +     of tracepoint never be duplicated with other locations of
>> +     tracepoints and other kinds of breakpoints, because two locations
>> +     at the same address may have different actions, so both of
>> +     these locations should be downloaded. 
> 
> and so that "tfind N" always works.
> 
> Please drop the "In current implementation" bit, and the "it is
> possible" comment.

OK, comment is changed as you suggested.

> 
>> +  /* Locations of tracepoints never be duplicated.  */
> 
> A verb is missing.  "can never be", perhaps?
> 

Oops.  Fixed.

>> +  if (is_tracepoint (left->owner))
>> +    gdb_assert (left->duplicate == 0);
> 
> `duplicate' is a boolean.  Make that `gdb_assert (!left->duplicate)'.
> 
>> +  if (is_tracepoint (right->owner))
>> +    gdb_assert (right->duplicate == 0);
> 
>> +         gdb_assert (loc->inserted == 0);
> 
> Boolean ==> `gdb_assert (!loc->inserted)'
> 

Fixed.

-- 
Yao (齐尧)