* [RFA] mdebugread.c (psymtab_to_symtab_1): assert that 'fh' is not null
@ 2011-03-04 22:15 Michael Snyder
2011-03-06 16:17 ` Jan Kratochvil
0 siblings, 1 reply; 6+ messages in thread
From: Michael Snyder @ 2011-03-04 22:15 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 86 bytes --]
This occurs along a path where fh *could* be null, and then we
dereference it.
OK?
[-- Attachment #2: null3.txt --]
[-- Type: text/plain, Size: 741 bytes --]
2011-03-04 Michael Snyder <msnyder@msnyder-server.eng.vmware.com>
* mdebugread.c (psymtab_to_symtab_1): Assert fh is not null.
Index: mdebugread.c
===================================================================
RCS file: /cvs/src/src/gdb/mdebugread.c,v
retrieving revision 1.120
diff -u -p -r1.120 mdebugread.c
--- mdebugread.c 2 Mar 2011 23:54:16 -0000 1.120
+++ mdebugread.c 4 Mar 2011 22:10:46 -0000
@@ -4323,6 +4323,7 @@ psymtab_to_symtab_1 (struct partial_symt
top_stack->blocktype = stFile;
ext_ptr = PST_PRIVATE (pst)->extern_tab;
+ gdb_assert (fh);
for (i = PST_PRIVATE (pst)->extern_count; --i >= 0; ext_ptr++)
parse_external (ext_ptr, fh->fBigendian,
pst->section_offsets, pst->objfile);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFA] mdebugread.c (psymtab_to_symtab_1): assert that 'fh' is not null
2011-03-04 22:15 [RFA] mdebugread.c (psymtab_to_symtab_1): assert that 'fh' is not null Michael Snyder
@ 2011-03-06 16:17 ` Jan Kratochvil
2011-03-06 18:54 ` Michael Snyder
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kratochvil @ 2011-03-06 16:17 UTC (permalink / raw)
To: Michael Snyder; +Cc: gdb-patches
On Fri, 04 Mar 2011 23:15:39 +0100, Michael Snyder wrote:
> This occurs along a path where fh *could* be null, and then we
> dereference it.
I agree there is a bug.
> + gdb_assert (fh);
[...]
> parse_external (ext_ptr, fh->fBigendian,
> pst->section_offsets, pst->objfile);
But I find as even a worse bug to introduce such an assertion without any
comment there.
I at least myself perceive any assertions that the programmer thinks such
invariant is valid there. That invariant says that if its negation happens
then the programmer was wrong.
If there is a known bug you have found and you just do not intend to fix it
now I find introducing such an assertion as misleading. The reader then
assumes such invariant and can do additional wrong conclusions from it.
In such case there should be primarily a "FIXME" comment, best even to
reference a filed a Bug. There can be also an assertion there but the "FIXME"
I find essential there.
Thanks,
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFA] mdebugread.c (psymtab_to_symtab_1): assert that 'fh' is not null
2011-03-06 16:17 ` Jan Kratochvil
@ 2011-03-06 18:54 ` Michael Snyder
2011-03-06 19:05 ` Jan Kratochvil
0 siblings, 1 reply; 6+ messages in thread
From: Michael Snyder @ 2011-03-06 18:54 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: gdb-patches
Jan Kratochvil wrote:
> On Fri, 04 Mar 2011 23:15:39 +0100, Michael Snyder wrote:
>> This occurs along a path where fh *could* be null, and then we
>> dereference it.
>
> I agree there is a bug.
>
>
>> + gdb_assert (fh);
> [...]
>> parse_external (ext_ptr, fh->fBigendian,
>> pst->section_offsets, pst->objfile);
>
> But I find as even a worse bug to introduce such an assertion without any
> comment there.
>
> I at least myself perceive any assertions that the programmer thinks such
> invariant is valid there. That invariant says that if its negation happens
> then the programmer was wrong.
>
> If there is a known bug you have found and you just do not intend to fix it
> now I find introducing such an assertion as misleading. The reader then
> assumes such invariant and can do additional wrong conclusions from it.
>
> In such case there should be primarily a "FIXME" comment, best even to
> reference a filed a Bug. There can be also an assertion there but the "FIXME"
> I find essential there.
All these recent submissions of mine are coming from running Coverity
on gdb. In this case, Coverity reports that we checked the variable fh
for null (suggesting that we think null is possible), but then
dereferenced it on an unchecked path.
I figured that triggering an assert was more gracefull than just
crashing on a null pointer dereference, and gives the user the chance
to recover.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFA] mdebugread.c (psymtab_to_symtab_1): assert that 'fh' is not null
2011-03-06 18:54 ` Michael Snyder
@ 2011-03-06 19:05 ` Jan Kratochvil
2011-03-07 16:53 ` Tom Tromey
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kratochvil @ 2011-03-06 19:05 UTC (permalink / raw)
To: Michael Snyder; +Cc: gdb-patches
On Sun, 06 Mar 2011 19:46:23 +0100, Michael Snyder wrote:
> All these recent submissions of mine are coming from running Coverity
> on gdb. In this case, Coverity reports that we checked the variable fh
> for null (suggesting that we think null is possible), but then
> dereferenced it on an unchecked path.
>
> I figured that triggering an assert was more gracefull than just
> crashing on a null pointer dereference, and gives the user the chance
> to recover.
I agree from the user point of view it may be (slightly) better (*).
But from the GDB developers point of view this change without any specific
code comment is not acceptable (as explained in my mail) as it confuses the
code readers.
I would find perfect:
/* FIXME: FH may be NULL here but the code below cannot handle it. */
gdb_assert (fh);
Thanks,
Jan
(*) In fact I like the crash from the Fedora point of view better as ABRT
(Automatic Bug Reporting Tool) will automatically report it. In the case
of a failed gdb_assert the user may or may not allow the core dump and
therefore the crash bugreport by ABRT may be suppressed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Create a core file of GDB? (y or n) n
This is just FYI and it should not be related to FSF GDB maintenance.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFA] mdebugread.c (psymtab_to_symtab_1): assert that 'fh' is not null
2011-03-06 19:05 ` Jan Kratochvil
@ 2011-03-07 16:53 ` Tom Tromey
2011-03-07 17:21 ` Jan Kratochvil
0 siblings, 1 reply; 6+ messages in thread
From: Tom Tromey @ 2011-03-07 16:53 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: Michael Snyder, gdb-patches
>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
Jan> (*) In fact I like the crash from the Fedora point of view better as ABRT
Jan> (Automatic Bug Reporting Tool) will automatically report it. In the case
Jan> of a failed gdb_assert the user may or may not allow the core dump and
Jan> therefore the crash bugreport by ABRT may be suppressed.
Jan> A problem internal to GDB has been detected,
Jan> further debugging may prove unreliable.
Jan> Create a core file of GDB? (y or n) n
Jan> This is just FYI and it should not be related to FSF GDB maintenance.
Then perhaps we should disable this in Fedora, or at least mention ABRT
in the note.
Tom
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFA] mdebugread.c (psymtab_to_symtab_1): assert that 'fh' is not null
2011-03-07 16:53 ` Tom Tromey
@ 2011-03-07 17:21 ` Jan Kratochvil
0 siblings, 0 replies; 6+ messages in thread
From: Jan Kratochvil @ 2011-03-07 17:21 UTC (permalink / raw)
To: Tom Tromey; +Cc: Michael Snyder, gdb-patches
On Mon, 07 Mar 2011 17:43:05 +0100, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
>
> Jan> (*) In fact I like the crash from the Fedora point of view better as ABRT
> Jan> (Automatic Bug Reporting Tool) will automatically report it. In the case
> Jan> of a failed gdb_assert the user may or may not allow the core dump and
> Jan> therefore the crash bugreport by ABRT may be suppressed.
> Jan> A problem internal to GDB has been detected,
> Jan> further debugging may prove unreliable.
> Jan> Create a core file of GDB? (y or n) n
> Jan> This is just FYI and it should not be related to FSF GDB maintenance.
>
> Then perhaps we should disable this in Fedora, or at least mention ABRT
> in the note.
It is more complicated, it should:
without ABRT:
create core file: n => nothing happens
create core file: y => ulimit -c unlimited + crash
with ABRT:
create core file: n => keep ulimit -c 0 + crash
create core file: y => ulimit -c unlimited + crash
As ABRT always catches any crash even with ulimit -c 0 but it leaves the
regular core file at currentdir as if no ABRT was running.
Thanks,
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-03-07 16:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-03-04 22:15 [RFA] mdebugread.c (psymtab_to_symtab_1): assert that 'fh' is not null Michael Snyder
2011-03-06 16:17 ` Jan Kratochvil
2011-03-06 18:54 ` Michael Snyder
2011-03-06 19:05 ` Jan Kratochvil
2011-03-07 16:53 ` Tom Tromey
2011-03-07 17:21 ` Jan Kratochvil
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox