Re: [RFC][PATCH?] fixed some segfaults and bugs in mdebug support

[Zeck S <zeck654321@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6803 bytes --]

Oh, I messed up the diff. Spoiled by github, never done it this way.
The testing makes sense now. Might be the weekend before I can make much
progress on that.
Thanks for the fast reply!

On Mon, Oct 23, 2023 at 4:40 AM Andrew Burgess <aburgess@redhat.com> wrote:

> Zeck S <zeck654321@gmail.com> writes:
>
> > First off, I apologize if I'm doing this process wrong. I have sent an
> > email to assign@gnu.org trying to get the paperwork required for
> copyright
> > assignment. I think that's the correct thing to do?
> >
> > While I wait on that, I'm not sure exactly what is required for these
> > changes.
> >
> > Here's what I fixed in mdebug support.
> >
> > info sym funcName would segfault
> > The first problem was that no compunit_symtab was set for the
> global_block
> > on blockvectors in  new_symtab. This caused a crash in block.c.
> > initialize_block_iterator called get_block_compunit_symtab and the
> > assertion gdb_assert (gb->compunit_symtab != NULL); would fail.
> >
> > info types would segfault
> > The second problem was memory corruption. struct global_block is a larger
> > and different type from plain block and blockvector is expected to have
> > index 0 be a global_block struct. This can be seen done correctly in
> jit.c
> > near /* Now add the special blocks */ under if (i == GLOBAL_BLOCK).
> Failing
> > to allocate this correctly leads to crashes for me (usually) in
> > set_compunit_symtab where the assertion  gdb_assert (gb->compunit_symtab
> ==
> > NULL); would randomly fail. This fix is also in new_symtab.
> >
> > info line file:line did not work
> > The third problem was finding lines never worked because add_line never
> set
> > .is_stmt to true, so in symtab.c find_line_common never saw item->is_stmt
> > as true, do it always went down the /* Ignore non-statements. */ path in
> > its main loop.
>
> I was confused by this description as the only change I see is you
> removing this line 'lt->item[lt->nitems].is_stmt = 1;' , but I suspect
> you generated your diff the wrong way round.
>
> You should consider creating your diff as a git commit, then use 'git
> send-email' to send out patches, I found this site
> https://git-send-email.io/ a pretty useful guide for setting up git &
> email sending.
>
> >
> > I looked in the gdb/testsuite directory, and I don't see a directory for
> > mips or mdebug? Unsure how to set up a test for this. To make files with
> > mdebug symbols, I used the old IRIX IDO compiler running under a kind of
> > qemu setup used by N64 game reverse engineering projects. (N64 dev is why
> > I'm interested in this symbol format. I can connect vscode to gdb and gdb
> > to an n64 emulator with a gdb stub to debug with symbols)
>
> You might not need to add any new tests at all, IF you can identify some
> existing tests that are fixed by your changes.
>
> Most tests are not separated based on which compiler or environment is
> used, though clearly there are exceptions, e.g. gdb.arch/*.exp does
> contain some architecture specific tests.  Instead most tests are
> written based on the GDB feature being tested.  For example,
> gdb.base/infoline.exp tests the 'info line' command.
>
> The expectation is that if someone has a more niche compiler or
> environment then they will perform their own regression testing using
> their setup.
>
> So, hopefully, if you can get the GDB tests running using your
> toolchain, then without your patch you'll see some failures in (maybe)
> gdb.base/infoline.exp, and after your patch some of the failures would
> be resolved, you'd then mention some (or all) of these improvements in
> your commit message.
>
> Of course, if your particular situation isn't covered by an existing
> test then you might need to extend an existing test -- or create a new
> test -- whatever seems most appropriate.
>
> >
> > diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
> > index 4b0a1eb255f..9cb30ce0acd 100644
> > --- a/gdb/mdebugread.c
> > +++ b/gdb/mdebugread.c
> > @@ -239,9 +239,6 @@ enum block_type { FUNCTION_BLOCK, NON_FUNCTION_BLOCK
> };
> >  static struct block *new_block (struct objfile *objfile,
> >   enum block_type, enum language);
> >
> > -static struct block *new_global_block (struct objfile *objfile,
> > - enum block_type, enum language);
> > -
> >  static struct compunit_symtab *new_symtab (const char *, int, struct
> > objfile *);
> >
> >  static struct linetable *new_linetable (int);
> > @@ -4545,7 +4542,6 @@ add_line (struct linetable *lt, int lineno,
> CORE_ADDR
> > adr, int last)
> >      return lineno;
> >
> >    lt->item[lt->nitems].line = lineno;
> > -  lt->item[lt->nitems].is_stmt = 1;
> >    lt->item[lt->nitems++].set_unrelocated_pc (unrelocated_addr (adr <<
> 2));
> >    return lineno;
> >  }
> > @@ -4638,10 +4634,9 @@ new_symtab (const char *name, int maxlines, struct
> > objfile *objfile)
> >
> >    /* All symtabs must have at least two blocks.  */
> >    bv = new_bvect (2);
> > -  bv->set_block (GLOBAL_BLOCK, new_global_block (objfile,
> > NON_FUNCTION_BLOCK, lang));
> > +  bv->set_block (GLOBAL_BLOCK, new_block (objfile, NON_FUNCTION_BLOCK,
> > lang));
> >    bv->set_block (STATIC_BLOCK, new_block (objfile, NON_FUNCTION_BLOCK,
> > lang));
> >    bv->static_block ()->set_superblock (bv->global_block ());
> > -  bv->global_block ()->set_compunit_symtab(cust);
> >    cust->set_blockvector (bv);
> >
> >    cust->set_debugformat ("ECOFF");
> > @@ -4740,21 +4735,6 @@ new_block (struct objfile *objfile, enum
> block_type
> > type,
> >    return retval;
> >  }
> >
> > -static struct block *
> > -new_global_block (struct objfile *objfile, enum block_type type,
> > -   enum language language)
>
> Static functions should have a comment before them.  In this case
> something as simple as:
>
>   /* Like new_block, but create a global_block.  */
>
> Though I wonder if we could/should just give new_block an extra
> parameter so its declaration becomes:
>
>   static struct block *new_block (struct objfile *objfile,
>                                   enum block_type, enum language,
>                                   bool global_block = false);
>
> Hopefully it's obvious how the new parameter would be used :)
>
> Thanks,
> Andrew
>
>
> > -{
> > -  struct block *retval = new (&objfile->objfile_obstack) global_block;
> > -
> > -  if (type == FUNCTION_BLOCK)
> > -    retval->set_multidict (mdict_create_linear_expandable (language));
> > -  else
> > -    retval->set_multidict (mdict_create_hashed_expandable (language));
> > -
> > -  return retval;
> > -}
> > -
> > -
> >  /* Create a new symbol with printname NAME.  */
> >
> >  static struct symbol *
>
>