[OB] pointer ref, m2-typeprint.c

[OB] pointer ref, m2-typeprint.c

[msnyder]

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

Move a pointer reference to after the check for NULL.


[-- Attachment #2: null.txt --]
[-- Type: text/plain, Size: 816 bytes --]

2007-06-28  Michael Snyder  <msnyder@access-company.com>

	* m2-typeprint.c (m2_print_type): Move pointer ref after null test
	(Coverity).

Index: m2-typeprint.c
===================================================================
RCS file: /cvs/src/src/gdb/m2-typeprint.c,v
retrieving revision 1.11
diff -p -r1.11 m2-typeprint.c
*** m2-typeprint.c	9 Jan 2007 17:58:51 -0000	1.11
--- m2-typeprint.c	29 Jun 2007 00:34:37 -0000
*************** m2_print_type (struct type *type, char *
*** 72,78 ****
    int demangled_args;
  
    CHECK_TYPEDEF (type);
-   code = TYPE_CODE (type);
  
    QUIT;
  
--- 72,77 ----
*************** m2_print_type (struct type *type, char *
*** 83,88 ****
--- 82,88 ----
        return;
      }
  
+   code = TYPE_CODE (type);
    switch (TYPE_CODE (type))
      {
      case TYPE_CODE_SET:

Re: [OB] pointer ref, m2-typeprint.c

[Daniel Jacobowitz]

Ooh thanks, that seems to have fixed attachments.

On Thu, Jun 28, 2007 at 05:37:03PM -0700, msnyder@sonic.net wrote:
>     CHECK_TYPEDEF (type);
> -   code = TYPE_CODE (type);

Coverity's software is a tool, not a goal, and it's no substitute for
knowing what the code is doing.  Please stop moving things around just
to make it happy.  If the second line I quoted could crash, the first
line definitely will; so if TYPE can really be NULL here, we should
fix it properly (instead of this change, which removes the helpful
notice that something is wrong :-).

-- 
Daniel Jacobowitz
CodeSourcery

Re: [OB] pointer ref, m2-typeprint.c

[msnyder]

> Ooh thanks, that seems to have fixed attachments.
>
> On Thu, Jun 28, 2007 at 05:37:03PM -0700, msnyder@sonic.net wrote:
>>     CHECK_TYPEDEF (type);
>> -   code = TYPE_CODE (type);
>
> Coverity's software is a tool, not a goal, and it's no substitute for
> knowing what the code is doing.

Daniel, I know what the code is doing.
I'm not doing this without analyzing things.

Doesn't mean I can't make a mistake -- but I'm not just
doing things blindly...

> Please stop moving things around just to make it happy.

I wouldn't do that.

> If the second line I quoted could crash, the first
> line definitely will; so if TYPE can really be NULL here, we should
> fix it properly (instead of this change, which removes the helpful
> notice that something is wrong :-).

Nooooo... I don't think my change removes any notice.
Take another look, please?

You may be right that CHECK_TYPEDEF is also a deref.
I confess I took that for granted without checking it.

I always assumed that CHECK_TYPEDEF was safe, since we so often
blythely call it without checking.  I guess it's not...

Re: [OB] pointer ref, m2-typeprint.c

[Daniel Jacobowitz]

On Thu, Jun 28, 2007 at 06:02:41PM -0700, msnyder@sonic.net wrote:
> > If the second line I quoted could crash, the first
> > line definitely will; so if TYPE can really be NULL here, we should
> > fix it properly (instead of this change, which removes the helpful
> > notice that something is wrong :-).
> 
> Nooooo... I don't think my change removes any notice.
> Take another look, please?

Sorry, I meant a notice from Coverity's tools.  The problem is that it
knows TYPE_CODE is a dereference (that's a locally derivable
conclusion within this file) but as far as I can tell it doesn't know
that CHECK_TYPEDEF will always dereference its argument (it will).
So now it's not going to complain, but if there was a problem,
there still is.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [OB] pointer ref, m2-typeprint.c

[Michael Snyder]

----- Original Message ----- 
From: "Daniel Jacobowitz" <drow@false.org>
To: <msnyder@sonic.net>
Cc: <gdb-patches@sourceware.org>
Sent: Friday, June 29, 2007 4:37 AM
Subject: Re: [OB] pointer ref, m2-typeprint.c


> On Thu, Jun 28, 2007 at 06:02:41PM -0700, msnyder@sonic.net wrote:
> > > If the second line I quoted could crash, the first
> > > line definitely will; so if TYPE can really be NULL here, we should
> > > fix it properly (instead of this change, which removes the helpful
> > > notice that something is wrong :-).
> >
> > Nooooo... I don't think my change removes any notice.
> > Take another look, please?
>
> Sorry, I meant a notice from Coverity's tools.  The problem is that it
> knows TYPE_CODE is a dereference (that's a locally derivable
> conclusion within this file) but as far as I can tell it doesn't know
> that CHECK_TYPEDEF will always dereference its argument (it will).
> So now it's not going to complain, but if there was a problem,
> there still is.

I agree.  It would be pretty simple to make CHECK_TYPEDEF safe,
and I think I would rather do that than go hunt down every place that
calls it.  What do you think?  Should I add that to this patch?

It would add a "not equal to null" test to every call to CHECK_TYPEDEF,
of which there are many, but on today's hardware the cost should be less
than negligable...

Re: [OB] pointer ref, m2-typeprint.c

[Daniel Jacobowitz]

On Fri, Jun 29, 2007 at 11:33:08AM -0700, Michael Snyder wrote:
> I agree.  It would be pretty simple to make CHECK_TYPEDEF safe,
> and I think I would rather do that than go hunt down every place that
> calls it.  What do you think?  Should I add that to this patch?
> 
> It would add a "not equal to null" test to every call to CHECK_TYPEDEF,
> of which there are many, but on today's hardware the cost should be less
> than negligable...

Do you think the case of a NULL type is at all common?  I bet
everything that uses CHECK_TYPEDEF then looks inside the type, so if
we want null type checks, they'd be more useful before the
CHECK_TYPEDEF than inside it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [OB] pointer ref, m2-typeprint.c

[Jim Blandy]

Daniel Jacobowitz <drow@false.org> writes:
> On Fri, Jun 29, 2007 at 11:33:08AM -0700, Michael Snyder wrote:
>> I agree.  It would be pretty simple to make CHECK_TYPEDEF safe,
>> and I think I would rather do that than go hunt down every place that
>> calls it.  What do you think?  Should I add that to this patch?
>> 
>> It would add a "not equal to null" test to every call to CHECK_TYPEDEF,
>> of which there are many, but on today's hardware the cost should be less
>> than negligable...
>
> Do you think the case of a NULL type is at all common?  I bet
> everything that uses CHECK_TYPEDEF then looks inside the type, so if
> we want null type checks, they'd be more useful before the
> CHECK_TYPEDEF than inside it.

I think CHECK_TYPEDEF ought to require a non-NULL pointer, as it does
now.  An explicit test should protect calls when the type might be
NULL.