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

[Zeck S <zeck654321@gmail.com>]

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

Not sure what etiquette is correct, but just so this thread isn't left
hanging without an update, after nearly a month:
I have created a QEMU user mode emulation board configuration, so that I
can test this patch.
https://sourceware.org/pipermail/gdb-patches/2023-December/204760.html

After that is resolved (or I am told of a better way),
I should be able to get some tests running by wrapping the compiler in a
script as suggested
at the bottom of this page
https://sourceware.org/gdb/wiki/Running%20the%20test%20suite%20with%20a%20non-GCC%20compiler

On Fri, Nov 10, 2023 at 9:07 PM Zeck S <zeck654321@gmail.com> wrote:

> I'm still working on the testing. Been setting up QEMU stuff so I can run
> linux mips code. Seems like the best option.
> I think I should be able to get the test suite to call the IDO compiler
> and use the linux mips linker.
>
> Also, sent the signed paperwork to the FSF for copyright assignment, and
> am waiting to hear back.
>
> Been a few weeks, so, figured I'd post an update. Fixed one more small bug
> I found as well.
>
> On Mon, Oct 23, 2023 at 7:25 PM Zeck S <zeck654321@gmail.com> wrote:
>
>> 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 *
>>>
>>>