Re: [PATCH 1/3] record, btrace: add record-btrace target

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Thu, 28 Feb 2013 18:04:00 +0100, Metzger, Markus T wrote:
> The "record source-lines-history" command needs
[...]
> also to suppress the noise of instruction scheduling.

I do not think this is doable in GDB, or at least it is not right to
workaround it in GDB.  It expects a proper support of DW_LNS_negate_stmt in
-O2 -g code from GCC.  This would also fix the current "jumping" during
step-ping in -O2 -g code.


> > > +/* A recorded function segment.  */
> > > +struct btrace_function
> > > +{
> > > +  /* The function symbol.  */
> > > +  struct minimal_symbol *mfun;
> > > +  struct symbol *fun;
> > 
> > When is one of them NULL or may be NULL?
> 
> I think I should always have a minimal symbol. I may not always have a
> full symbol. For example, when there is no debug information.

Without debug information you commonly do not have even the minimal symbol.


> I added a comment saying that one of them may be NULL.

Thanks.


> > > +  btinfo->btrace = target_read_btrace (btinfo->target);
> > 
> > In btrace.c/update_btrace from archer-mmetzger-btrace:
> > 	btp->btrace = target_read_btrace (btp->target);
> > without freeing btp->btrace there beforehand, does it leak there?
> 
> Yes. I have updated the archer branch before sending this
> patch series, though. 

I do not understand it much now - the archer branch
5dd7aa460b03c158b938fa76ff6c83927d597bd8 still contains update_btrace not
using VEC_free and fetch_btrace using VEC_free; so either one is wrong.


> I don't think we should give an error if GDB wants to read the trace twice.
> 
> This extra call to target_btrace_has_changed was meant to save the
> time of repeatedly sending and processing the same trace.

The problem is with large RTT (round-trip-time) gdb<->gdbserver it is even
cheaper to transfer more data than to ask twice (even for less data).


> What we could do is free the trace when the target continues (or stops) and
> set the trace pointers to NULL.  Would that be better?
> If yes, is there already some notifier or hook that I could use?

to_resume is there.

But I would find safer to leave the decision whether something changed or not
still to the target.  One could modify remote_read_btrace:

static VEC (btrace_block_s) *
remote_read_btrace (struct btrace_target_info *tinfo, int if_changed)
  xml = target_read_stralloc (&current_target,
                              TARGET_OBJECT_BTRACE, if_changed ? "if-changed" : NULL);
			                            /* == annex field */

Then I guess remote_btrace_has_changed and even its packet do not have to
exist at all.


> > > +static struct btrace_thread_info *
> > > +require_btrace (void)
> > > +{
> > > +  struct thread_info *tp;
> > > +  struct btrace_thread_info *btinfo;
> > > +
> > > +  DEBUG_FUN ("require trace");
> > > +
> > > +  tp = find_thread_ptid (inferior_ptid);
> > 
> > When possible/feasible we try to no longer use inferior_ptid in GDB, make it
> > a ptid_t parameter instead.
> 
> This function is called from command functions to get the branch trace for
> the current thread.
> 
> At some point, I do need to get the current thread. If I made ptid_t a
> parameter here, I would have to get the current ptid in all callers.

Yes, the direct access to inferior_ptid should be done only at the topmost
callers so that it can be easier changed in the future.  But it is not much
a requirement, you can keep it as is.


> > > +static void
> > > +btrace_insn_history (struct btrace_thread_info *btinfo, struct ui_out *uiout,
> > > +		     unsigned int begin, unsigned int end, int flags)
> > > +{
> > > +  struct gdbarch *gdbarch;
> > > +  unsigned int idx;
> > > +
> > > +  DEBUG_FUN ("itrace (0x%x): [%u; %u[", flags, begin, end);
> > > +
> > > +  gdbarch = target_gdbarch ();
> > > +
> > > +  for (idx = begin; idx < end; ++idx)
> > 
> > btrace_function->{begin,end} are inclusive but these are apparently begin
> > inclusive but end exclusive parameters.
> 
> The [begin; end] range in struct btrace_function refers to source lines.
> The [begin; end[ range here refers to indices into the instruction trace vector.

OK so could you comment it in all such cases?  Sorry I was not explicit
I request such a comment there.


> > > +  /* Initialize file and function name based on the information we have.  */
> > > +  if (fun != NULL)
> > > +    {
> > > +      bfun->filename = symtab_to_filename_for_display (fun->symtab);
> > 
> > Do not store the result of symtab_to_filename_for_display, it is intended only
> > for immediate use - its output may change by "set filename-display".

As any 'struct btrace_function' is alive only during the single execution of
a command there should not be any problems with symtab lifetime, my suggesions
about free_objfile discarding of stale symtab pointers where therefore
irrelevant.


> > Moreover the result is _for_display - so not for any comparisons.

In such case call the field bfun->filename_for_display.

Although TBH I would find easier to store symtab itself as one can use it for
any purpose then and calling symtab_to_filename_for_display each time during
its use is zero-cost.


> > You can store fun->symtab itself.  But in such case one needs to hook
> > a function to free_objfile like there is now hooked breakpoint_free_objfile so
> > that there do not remain stale symtab pointers.
> 
> I can compute the filename when I intend to print a line, so I don't need to store
> the symtab pointer separately.
> 
> I changed it to obtain the filename via a call to symtab_to_fullname.

In such case call the field bfun->fullname.


> I store that
> while I traverse the instruction trace vector so I can compare it to other filenames.
> Would I need to duplicate the string or is it safe to store it as-is for my purpose?

The string is alive as long as symtab is alive.  Both
symtab_to_filename_for_display and symtab_to_fullname return 'const char *' as
the string is stored in the symtab; you get only a readonly reference to it.


> > > +      bfun->function = SYMBOL_PRINT_NAME (fun);
> > 
> > Do not store the output of SYMBOL_PRINT_NAME.  You can store FUN itself (in
> > such case sure you no longer need to store fun->symtab as suggested above).
> > Again you need a hook in free_objfile to prevent stale FUN pointers.
> 
> I changed it to compute the function name whenever I want to print it.
> 
> I'm only storing symbol and minimal symbol pointers for computing the output
> of "record function-call-history". I guess I won't need the hook in that case.

You are right; sorry I originally expected "bfun" has longer lifetime.


> > > +    }
> > > +  else if (mfun != NULL)
> > > +    {
> > > +      bfun->filename = mfun->filename;
> > 
> > mfun->filename is not used in GDB, it is just a basename available only in
> > some cases, for minimal symbols GDB does not know their source filename.
> 
> I use it only for functions for which I do not have a full symbol. The alternative
> would be to not provide a filename in that case. Would that be preferable?

Yes, that would be preferable.


> OK. I would still use symtab_to_filename_for_display when displaying the filename, right?

Yes.


> So I would store the fullname for the above comparison and compute the filename to
> display when printing the line.

I do not see the code - but you need the symtab when printing the filename to
display.  Isn't it easier to use the same symtab also for symtab_to_fullname?
But that is a coding detail, I agree with the principle.


> What would I do if I only have a minimal symbol? Can I still use ->filename for this check?
> See above for a related question on printing filenames.

No, if you have only a minimal symbol you have no filename.  But in such case
there is also no problem with macros from other files etc. as all the
addresses will still resolve to the same minimal symbol.


> > > +  /* Check if we're still in the same file.  */
> > > +  if (compare_filenames_for_search (bfun->filename, filename))
> > > +    return 0;
> > 
> > Use filename_cmp for full pathnames.  Therefore always use symtab_to_fullname.
> > The result of symtab_to_fullname call as 'const char *fullname'.
> 
> Can I store the returned pointer? Would I need to duplicate the string?

It is preferred to store the symtab pointer, not the various kinds of
filenames it provides.

You do not need to duplicate the strings as long as you do not store them
longer than the single command execution (which here you do not).


> > Just do not print anything (no "filename:" prefix) for minimal symbols; also
> > omit the ui_out_field_string call so that in (future) MI the field "file" just
> > does not exist.
> 
> That might affect a lot of library code for which we don't have debug info.

Yes, for libraries without debug info one knows only the function names.
Install the system libraries separate debug info if you want more.


> OK. I assume that a full symbol will always have a symtab. Is this correct?

Correct.


Thanks,
Jan