Re: [PATCH v4 2/2] gdb: CTF support

[Wei-min Pan <weimin.pan@oracle.com>]

On 10/9/2019 10:41 AM, Tom Tromey wrote:
 >>>>>> Weimin Pan <weimin.pan@oracle.com> writes:
 >
 >> This patch adds the CTF (Compact Ansi-C Type Format) support in gdb.
 >
 > This went in by mistake, but since it was on the road to going in
 > anyway, I guess we'll just live with that.  We all make mistakes, but at
 > the same time, please do try to be careful.

I'm sure that the lesson learned from all this will be remembered and
the mistake won't be repeated.

 >
 >> diff --git a/Makefile.def b/Makefile.def
 >> index 6f5e881..311feb9 100644
 > [...]
 >
 > I didn't look, did the top-level changes go in to gcc?
 > The top-level configury code is canonically maintained there.

Given the following rules in Makefile.def:

dependencies = { module=configure-gcc; on=all-binutils; };
dependencies = { module=all-binutils; on=all-libctf; };
dependencies = { module=all-gdb; on=all-libctf; };

binutils gets built before gcc does. It's not clear why making changes
into gcc is needed?

 >
 >> +static const struct objfile_data *ctf_file_key;
 >
 > It's preferable to use the type-safe registry approach in new code.

This register key was not intended to manage the object with new/delete
but to be used to close file descriptors that are associated with the
ctf file/archive.

 >>> +typedef struct ctf_context
 >> +{
 >> +  ctf_file_t *fp;
 >> +  struct objfile *of;
 >> +  struct buildsym_compunit *builder;
 >> +} ctf_context_t;
 >
 > gdb doesn't generally use typedefs like this, especially now that it's
 > in C++.

It seems there are several places, e.g. aarch64-tdep.c, event-loop.c,
linespec.c, procfs.c, that do. We can drop "typedef" if you prefer.

 >
 >> +/* Get text segment base for OBJFILE, TSIZE contains the segment 
size.  */
 >> +
 >> +static CORE_ADDR
 >> +get_objfile_text_range (struct objfile *of, int *tsize)
 >> +{
 >> +  CORE_ADDR text_base;
 >> +  bfd *abfd = of->obfd;
 >> +  const asection *codes;
 >> +
 >> +  codes = bfd_get_section_by_name (abfd, ".text");
 >
 > Instead of this function, it's more usual in gdb to use:
 >
 >       CORE_ADDR baseaddr
 >       = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
 >
 > Maybe this function is even wrong in some situation, I'm not sure.

You suspect that bfd_get_section_by_name might return a wrong *asection?
Yes, we can use ANOFFSET, as you suggested, for the text base but still
need to get the size of the text section.