Re: [PATCH] target attributes [1/5] core and load from target function

["Abid, Hafiz" <hafiz_abid@mentor.com>]

On 18/02/13 10:01:10, Hui Zhu wrote:
> On Fri, Nov 30, 2012 at 2:03 PM, Abid, Hafiz <Hafiz_Abid@mentor.com>  
> wrote:
> > Hi,
> >
> >>+  ta = xzalloc (sizeof (struct target_attribute));
> >>+  ta->name = xstrdup (name);
> > This name is not freed and will leak.
> 
> Added xfree in clear_target_attributes.
> 
> >
> >>+  if (*tap)
> >>+    (*tap)->next = ta;
> >>+  else
> >>+    target_attributes_list = ta;
> >>+  *tap = ta;
> >>+
> >>+  user_data = &ta;
> >
> > So you are assigning ta to (*tap) and (*tap)->next. Then user_data  
> is also being assigned. It did not look right to me. I wanted to  
> bring it in your attention in case it is a typo.
> 
> user_data will always point to the last one of target_attributes_list.
>   Could you tell me which part is wrong?
This code may be right. It just looked a bit confusing. Also tap and  
user_data point to samething. One of the following 2 assignment seems  
redundant.
*tap = ta;
user_data = &ta;

> 
> >
> >>+    unsigned int agent_access;
> >>+    unsigned int gdb_access;
> > I can understand the access mode for GDB. But what agent_access  
> means and how it will effect a user?
> 
> There is the introduce of agent
> http://sourceware.org/gdb/current/onlinedocs/gdb/Agent-Expressions.html
> 
> Post a new version according to your review.
> 
> Thanks,
> Hui
> 
> 2013-02-18  Hui Zhu  <hui_zhu@mentor.com>
> 
> 	* Makefile.in (SFILES): Add target-attributes.c.
> 	(HFILES_NO_SRCDIR): Add target-attributes.h.
> 	(COMMON_OBS): Add target-attributes.o.
> 	* breakpoint.h (breakpoint): Add target_only_cond_check.
> 	* remote.c (target-attributes.h): New include.
> 	(PACKET_qXfer_target_attributes_read): New enum.
> 	(remote_start_remote): Add handler for target attributes.
> 	(remote_protocol_features): Add "qXfer:target-attributes:read".
> 	(remote_xfer_partial): Add handler for TARGET_OBJECT_ATTRIBUTES.
> 	(_initialize_remote): Add command
> 	"set remote target-attributes-packet".
> 	(target-attributes.c, target-attributes.h): New files.
> 	* tracepoint.c (target-attributes.h): New include.
> 	(find_trace_state_variable_number,
> 	trace_variable_number_check_1,
> 	trace_variable_number_check): New functions.
> 	(trace_variable_command): Call trace_variable_number_check_1.
> 	(merge_uploaded_trace_state_variables): Call
> 	trace_variable_number_check.
> 	tracepoint.h (trace_variable_number_check): New extern.
> 
> >
> >> -----Original Message-----
> >> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> >> owner@sourceware.org] On Behalf Of Hui Zhu
> >> Sent: Wednesday, November 21, 2012 8:55 AM
> >> To: Qi, Yao
> >> Cc: Zhu, Hui; gdb-patches ml
> >> Subject: Re: [PATCH] target attributes [1/5] core and load from  
> target
> >> function
> >>
> >> On Sun, Sep 2, 2012 at 6:37 PM, Hui Zhu <teawater@gmail.com> wrote:
> >> > On Wed, Aug 29, 2012 at 10:43 PM, Yao Qi <yao@codesourcery.com>
> >> wrote:
> >> >> On 08/29/2012 04:11 PM, Hui Zhu wrote:
> >> >>> +static void
> >> >>> +target_attribute_address_handler (struct gdb_xml_parser  
> *parser,
> >> >>> +                               const struct gdb_xml_element
> >> *element,
> >> >>> +                               void *user_data,
> >> >>> +                               VEC(gdb_xml_value_s)  
> *attributes) {
> >> >>> +  int i, len;
> >> >>> +  struct gdb_xml_value *attrs = VEC_address (gdb_xml_value_s,
> >> >>> +attributes);
> >> >>> +  struct target_attribute *ta = *(struct target_attribute
> >> >>> +**)user_data;
> >> >>> +  CORE_ADDR start, end;
> >> >>> +  struct target_attribute_address *ta_addr;
> >> >>
> >> >> I happen to see some compilation warnings on my new Fedora 16  
> box,
> >> >> while these warnings don't appear on my Ubuntu box.
> >> >>
> >> >> ... -Werror -c -o target-memory.o -MT target-memory.o -MMD -MP  
> -MF
> >> >> .deps/target-memory.Tpo ../../../git/gdb/target-memory.c
> >> >> ../../../git/gdb/target-attributes.c: In function
> >> 'target_attribute_address_handler':
> >> >> ../../../git/gdb/target-attributes.c:487:16: error: 'end' may be
> >> used
> >> >> uninitialized in this function [-Werror=uninitialized]
> >> >> ../../../git/gdb/target-attributes.c:486:18: error: 'start' may  
> be
> >> >> used uninitialized in this function [-Werror=uninitialized]
> >> >> ../../../git/gdb/target-attributes.c: In function
> >> 'target_attribute_attr_handler':
> >> >> ../../../git/gdb/target-attributes.c:376:12: error: 'type' may  
> be
> >> >> used uninitialized in this function [-Werror=uninitialized]
> >> >> ../../../git/gdb/target-attributes.c: In function
> >> 'target_attribute_type':
> >> >> ../../../git/gdb/target-attributes.c:632:3: error: 'ret' may be  
> used
> >> >> uninitialized in this function [-Werror=uninitialized]
> >> >> cc1: all warnings being treated as errors
> >> >> make: *** [target-attributes.o] Error 1
> >> >>
> >> >> The gcc I am using is 4.6.3
> >> >> $ gcc --version
> >> >> gcc (GCC) 4.6.3 20120306 (Red Hat 4.6.3-2)
> >> >>
> >> >> We have to get these warnings fixed.
> >> >>
> >> >>> +
> >> >>> +  len = VEC_length (gdb_xml_value_s, attributes);  for (i =  
> 0; i <
> >> >>> + len; i++)
> >> >>> +    {
> >> >>> +      if (strcmp (attrs[i].name, "start") == 0)
> >> >>> +        start = * (ULONGEST *) attrs[i].value;
> >> >>> +      else if (strcmp (attrs[i].name, "end") == 0)
> >> >>> +        end = * (ULONGEST *) attrs[i].value;
> >> >>> +      else
> >> >>> +        gdb_xml_error (parser, _("Unknown attribute name  
> '%s'."),
> >> >>> +                    attrs[i].name);
> >> >>> +    }
> >> >>> +
> >> >>> +  ta_addr = xmalloc (sizeof (struct  
> target_attribute_address));
> >> >>> + ta_addr->start = start;  ta_addr->end = end;
> >> >>> +
> >> >>> +  ta_addr->prev = ta->addresses;
> >> >>> +  ta->addresses = ta_addr;
> >> >>> +}
> >> >>
> >> >> --
> >> >> Yao
> >> >
> >> > Thanks.  I post a new version.
> >> >
> >> > Best,
> >> > Hui
> >>
> >> Got some error when built with trunk.  Post a new version for this
> >> error.
> >>
> >> Thanks,
> >> Hui
>