This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Add CTF support to GDB [1/4] Add "-ctf" to tsave command


On Fri, Dec 14, 2012 at 7:36 PM, Hui Zhu <teawater@gmail.com> wrote:
> On Fri, Nov 30, 2012 at 4:06 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>>> "Hui" == Hui Zhu <hui_zhu@mentor.com> writes:
>>
>> Hui> This patch is for the CTF write.
>> Hui> It add "-ctf" to tsave command.  With this option, tsave can save
>> Hui> current trace frame to CTF file format.
>>
>> Hui> +struct ctf_save_collect_s
>> Hui> +{
>> Hui> +  struct ctf_save_collect_s *next;
>> Hui> +  char *str;
>> Hui> +  char *ctf_str;
>> Hui> +  int align_size;
>> Hui> +  struct expression *expr;
>> Hui> +  struct type *type;
>> Hui> +  int is_ret;
>> Hui> +};
>>
>> Like Hafiz said -- comments would be nice.
>
> I added some comments in the new patches.
>
>>
>> Hui> +static void
>> Hui> +ctf_save_fwrite (FILE *fd, const gdb_byte *buf, size_t size)
>> Hui> +{
>> Hui> +  if (fwrite (buf, size, 1, fd) != 1)
>> Hui> +    error (_("Unable to write file for saving trace data (%s)"),
>> Hui> +     safe_strerror (errno));
>>
>> Why not use the existing ui_file code?
>>
>> Then you could remove this function plus several others.
>>
>> Maybe it is because you need fseek, but that seems like a simple
>> addition to ui_file.
>
> I still not update this part because fseek patch is still not OK.
> And after discussion with Pedro, I was really worry about change to
> ui_file will make CTF write doesn't have error check.  Could you help
> me with it?
>
>>
>> Hui> +    case TYPE_CODE_ARRAY:
>> Hui> +      for (; TYPE_CODE (type) == TYPE_CODE_ARRAY;
>> Hui> +     type = TYPE_TARGET_TYPE (type))
>> Hui> +  ;
>>
>> You probably want some check_typedef calls in there.
>
> Because typedef will be handle as a type in this part, so this part
> doesn't need check_typedef.
>
>>
>> Hui> +  ctf_save_type_name_write (tcsp, TYPE_FIELD_TYPE (type, biggest_id));
>>
>> Here too.
>
> I think this part is same with array.
>
>>
>> Hui> +      ctf_save_fwrite_format (tcsp->metadata_fd, "%s", TYPE_NAME (type));
>>
>> What if TYPE_NAME is NULL?
>
> Add code handle it  like TYPE_CODE_STRUCT.
>
>>
>> Hui> +static void
>> Hui> +ctf_save_type_size_write (struct ctf_save_s *tcsp, struct type *type)
>> Hui> +{
>> Hui> +  if (TYPE_CODE (type) == TYPE_CODE_ARRAY)
>> Hui> +    {
>> Hui> +      for (; TYPE_CODE (type) == TYPE_CODE_ARRAY;
>> Hui> +     type = TYPE_TARGET_TYPE (type))
>>
>> check_typedef
>
> This is function will call itself to post all the define of type to file.
> So It don't need check_typedef.
>
>>
>> Hui> +  if (TYPE_NAME (type) && (strcmp (TYPE_NAME (type), "uint32_t") == 0
>> Hui> +                     || strcmp (TYPE_NAME (type), "uint64_t") == 0))
>> Hui> +    return;
>>
>> check_typedef.
>>
>> Also it seems like this clause should go in the TYPE_CODE_INT case.
>>
>> Hui> +
>> Hui> +  switch (TYPE_CODE (type))
>> Hui> +    {
>> Hui> +      case TYPE_CODE_TYPEDEF:
>> Hui> +  ctf_save_fwrite_format (tcsp->metadata_fd, "typedef ");
>> Hui> +  ctf_save_var_define_write (tcsp, TYPE_TARGET_TYPE (type),
>>
>> check_typedef; though if your intent is to peel just a single layer,
>> then it is a bit trickier -- I think the best you can do is always call
>> it, then use TYPE_TARGET_TYPE if it is non-NULL or the result of
>> check_typedef otherwise.
>
> If use check_typedef, this part will generate the define that
> different with the type descriptor of the code.
>
> For example:
> Following is the define in the code:
> typedef char test_t1;
> typedef test_t1 test_t2;
> typedef test_t2 test_t3;
>
> If use TYPE_TARGET_TYPE, it will generate following metadata:
> typedef char test_t1;
> typedef test_t1 test_t2;
> typedef test_t2 test_t3;
>
> If use check_typedef, it will generate following metadata:
> typedef char test_t1;
> typedef char test_t2;
> typedef char test_t3;
>
>>
>> Hui> +    tcsp->tab = tab;
>> [...]
>> Hui> +    tcsp->tab = old_tab;
>>
>> No idea if it matters, but if an exception is thrown during the '...'
>> code, then the 'tab' field will be left set improperly.
>
> Please don't worry about this part.
> This tab always be set to local value in stack.  So even if it is
> drop, it will not affect anything.
>
> For example:
> char tab[256];
> const char *old_tab;
>
> old_tab = tcsp->tab;
> snprintf (tab, 256, "%s\t", old_tab);
> tcsp->tab = tab;
> [...]
> tcsp->tab = old_tab;
>
>>
>> Hui> +      case TYPE_CODE_PTR:
>> Hui> +  align_size = TYPE_LENGTH (type);
>> Hui> +  break;
>>
>> Surely the alignment rules are ABI dependent.
>> I would guess that what you have will work in many cases, but definitely
>> not all of them.
>
> All the type will be handle and record in function
> ctf_save_type_check_and_write.
> The size align will be handle in this function too.
>
>>
>> Hui> +    frame = get_current_frame ();
>> Hui> +    if (!frame)
>> Hui> +      error (_("get current frame fail"));
>> Hui> +    frame = get_prev_frame (frame);
>> Hui> +    if (!frame)
>> Hui> +      error (_("get prev frame fail"));
>>
>> These messages could be improved.
>>
>> Hui> +      warning (_("\
>> Hui> +Not save \"%s\" of tracepoint %d to ctf file because get its value fail: %s"),
>> Hui> +         str, tps->tp->base.number, e.message);
>>
>> Likewise.
>
> Could you help me with this part?  :)
>
>>
>> Hui> +      /* XXX: no sure about variable_length
>> Hui> +   and need set is_variable_length if need.  */
>> Hui> +      collect->align_size = align_size;
>> Hui> +      if (collect->align_size > tps->align_size)
>> Hui> +        tps->align_size = collect->align_size;
>>
>> No new FIXME comments.
>> Can you find the answer to this question and either fix the code or drop
>> the comment?
>
> Fixed.
>
>>
>> Hui> +  char name[strlen (print_name) + 1];
>>
>> I think you need an explicit alloca here.
>> Or xmalloc + xfree, which is probably better.
>
> Fixed.
>
>>
>> Although, this approach just seems weird, since it seems like you
>> already have the symbol and you want its value; constructing and parsing
>> an expression to get this is very roundabout.
>>
>> I'm not sure I really understand the goal here; but the parsing approach
>> is particularly fragile if you have shadowing.
>
> Function ctf_save_collect_get will parse the collect string and add
> them to struct.
> Each tracepoint will call this function just once.
>
> The code that try to get the value is in function ctf_save:
>               back_chain = make_cleanup (null_cleanup, NULL);
>               TRY_CATCH (e, RETURN_MASK_ERROR)
>                 {
>                   val = evaluate_expression (collect->expr);
>                   content = value_contents (val);
>                 }
> Could you tell me some better way?
>
>>
>> Hui> +        iterate_over_block_local_vars (block,
>> Hui> +                                       tsv_save_do_loc_arg_collect,
>> Hui> +                                       &cb_data);
>>
>> Why just iterate over this block and not the enclosing ones as well?
>>
>> Hmm, a lot of this code looks like code from tracepoint.c.
>> I think it would be better to share the code if that is possible.
>
> I tried to share code with function add_local_symbols.  But it is not
> a big function and use different way to get block.
>
>>
>> Hui> +static char *
>> Hui> +ctf_save_metadata_change_char (struct ctf_save_s *tcsp, char *ctf_str,
>> Hui> +                         int i, const char *str)
>> Hui> +{
>> Hui> +  char *new;
>> Hui> +
>> Hui> +  new = xmalloc (strlen (ctf_str) + (i ? 1 : 0) + strlen (str) + 1 - 1);
>> Hui> +  ctf_str[i] = '\0';
>> Hui> +  sprintf (new, "%s%s%s_%s", ctf_str, (i ? "_" : ""), str, ctf_str + i + 1);
>>
>> Just use xstrprintf.
>
> Fixed.
>
>>
>> Hui> +static void
>> Hui> +ctf_save_do_nothing (void *p)
>> Hui> +{
>> Hui> +}
>>
>> Use null_cleanup instead.
>
> Fixed.
>
>>
>> Hui> +    if (collect->expr)
>> Hui> +      free_current_contents (&collect->expr);
>>
>> Why free_current_contents here?
>> That seems weird.
>
> If this collect is $_ret, it will not have collect->expr.
> Or maybe this collect will be free because when setup this collect get error.
> So check it before free it.
>
>>
>> Hui> +  char file_name[strlen (dirname) + 1 + strlen (CTF_DATASTREAM_NAME) + 1];
>>
>> alloca or malloc.
>
> Fixed.
>
>>
>> Hui> +  sprintf (file_name, "%s/%s", dirname, CTF_METADATA_NAME);
>> Hui> +  tcs.metadata_fd = fopen (file_name, "w");
>> Hui> +  if (!tcs.metadata_fd)
>> Hui> +    error (_("Unable to open file '%s' for saving trace data (%s)"),
>> Hui> +     file_name, safe_strerror (errno));
>> Hui> +  ctf_save_metadata_header (&tcs);
>> Hui> +
>> Hui> +  sprintf (file_name, "%s/%s", dirname, CTF_DATASTREAM_NAME);
>> Hui> +  tcs.datastream_fd = fopen (file_name, "w");
>> Hui> +  if (!tcs.datastream_fd)
>> Hui> +    error (_("Unable to open file '%s' for saving trace data (%s)"),
>> Hui> +     file_name, safe_strerror (errno));
>>
>> On error these files will be left partially written.
>> Is that intentional?
>
> What my thought about this part is even if GDB get error when CTF
> save, the data before this error is OK.  So in clean up function
> ctf_save_cleanup, it will not remove this file.  And it will write the
> data that don't have error, function ctf_save_metadata (tcsp).
>
>>
>> Hui> +extern void ctf_save (char *dirname);
>>
>> Why not const?
>> This applies in a few spots in the patch.
>
> Fixed.
>
>>
>> Hui> @@ -2465,6 +2466,7 @@ void
>> Hui>  mi_cmd_trace_save (char *command, char **argv, int argc)
>> [...]
>> Hui> +      if (strcmp (argv[0], "-ctf") == 0)
>> Hui> +  generate_ctf = 1;
>>
>> The 'usage' line needs an update.
>
> Fixed.
>
>>
>> Hui> +  if (generate_ctf)
>> Hui> +    ctf_save (filename);
>> Hui> +  else
>> Hui> +    trace_save (filename, target_saves);
>>
>> I don't understand why CTF isn't just an option to trace_save -- share
>> the trace-dependent work, separate out the formatting.
>>
>
> I separate read and write CTF support function because:
> CTF format is a complex format.  I tried to support all of it but I failed.
> Then I changed to use libbabeltrace, it works very good with read CTF.
>  But it doesn't supply API for CTF write.  And I discussion the
> developer of libbabeltrace, they said they don't have plan for that.
> So I add CTF write function inside GDB with my hand.  And because CTF
> is design for tracepoint support.  So it is really fit with tsave
> command.  So I add it as an option.
>
>> Hui>  trace_save_command (char *args, int from_tty)
>> Hui>  {
>> Tom
>
> Thanks for your comments.  I post a new version.
>
> Best,
> Hui

Hi Tom,

I post a  patch that updated according to the update of trunk.

Thanks,
Hui


2012-12-18  Hui Zhu  <hui_zhu@mentor.com>

	* Makefile.in (REMOTE_OBS): Add ctf.o.
	(SFILES): Add ctf.c.
	(HFILES_NO_SRCDIR): Add ctf.h.
	* ctf.c, ctf.h: New files.
	* mi/mi-main.c (ctf.h): New include.
	(mi_cmd_trace_save): Add "-ctf".
	* tracepoint.c (ctf.h): New include.
	(collect_pseudocommand): Remove static.
	(trace_save_command): Add "-ctf".
	(_initialize_tracepoint): Ditto.
	* tracepoint.h (collect_pseudocommand): Add extern.

Attachment: tsave-ctf.txt
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]