[RFC,PATCH?] fixed some segfaults and bugs in mdebug support
Checks
Commit Message
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 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)
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)
-{
- 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 *
Comments
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 *
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 *
>
>
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 *
>>
>>
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 *
>>>
>>>
@@ -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 *);