[RFC] logic change in m2-valprint.c

[RFC] logic change in m2-valprint.c

[msnyder]

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

I'm really not sure about this, call it a shot in the dark.

The logic doesn't make sense as it is (the condition can't be true),
so I figured maybe the conditional had been reversed by mistake.

Either that, or the two lines should be removed, 'cause they're dead.


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

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

	* m2-valprint.c (m2_print_long_set): Logical condition reversed
	(Coverity).

Index: m2-valprint.c
===================================================================
RCS file: /cvs/src/src/gdb/m2-valprint.c,v
retrieving revision 1.12
diff -p -r1.12 m2-valprint.c
*** m2-valprint.c	9 Jan 2007 17:58:51 -0000	1.12
--- m2-valprint.c	29 Jun 2007 00:25:56 -0000
*************** m2_print_long_set (struct type *type, co
*** 132,138 ****
  	      previous_high = i;
  	      if (! element_seen)
  		{
! 		  if (! empty_set)
  		    fprintf_filtered (stream, ", ");
  		  print_type_scalar (target, i, stream);
  		  empty_set    = 0;
--- 132,138 ----
  	      previous_high = i;
  	      if (! element_seen)
  		{
! 		  if (empty_set)
  		    fprintf_filtered (stream, ", ");
  		  print_type_scalar (target, i, stream);
  		  empty_set    = 0;

Re: [RFC] logic change in m2-valprint.c

[Daniel Jacobowitz]

On Thu, Jun 28, 2007 at 05:29:55PM -0700, Michael Snyder wrote:
> I'm really not sure about this, call it a shot in the dark.
> 
> The logic doesn't make sense as it is (the condition can't be true),
> so I figured maybe the conditional had been reversed by mistake.
> 
> Either that, or the two lines should be removed, 'cause they're dead.
> 

> 2007-06-28  Michael Snyder  <msnyder@access-company.com>
> 
> 	* m2-valprint.c (m2_print_long_set): Logical condition reversed
> 	(Coverity).

It can't be backwards; this is trying to print "1" or "1, 2" but your
change would make it print ", 1" and ", 12".

I think it's also trying to shorten ranges to "{1..3, 6..7}".  The
bug's got to be in there somewhere.  Why's the code dead?  I'm not
seeing it... element_seen can be reset by the bit clear case, and then
we'll get into the test you're changing again.


-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] logic change in m2-valprint.c

[Michael Snyder]

> On Thu, Jun 28, 2007 at 05:29:55PM -0700, Michael Snyder wrote:
> > I'm really not sure about this, call it a shot in the dark.
> >
> > The logic doesn't make sense as it is (the condition can't be true),
> > so I figured maybe the conditional had been reversed by mistake.
> >
> > Either that, or the two lines should be removed, 'cause they're dead.
> >
>
> > 2007-06-28  Michael Snyder  <msnyder@access-company.com>
> >
> > * m2-valprint.c (m2_print_long_set): Logical condition reversed
> > (Coverity).
>
> It can't be backwards; this is trying to print "1" or "1, 2" but your
> change would make it print ", 1" and ", 12".
>
> I think it's also trying to shorten ranges to "{1..3, 6..7}".  The
> bug's got to be in there somewhere.  Why's the code dead?  I'm not
> seeing it... element_seen can be reset by the bit clear case, and then
> we'll get into the test you're changing again.

'Cause we're not in a loop, and it's not a static variable.
The code is serial.  At entry we set empty_set to one, and
then we test to see if it's zero.  It can't be zero.

Re: [RFC] logic change in m2-valprint.c

[Daniel Jacobowitz]

On Sun, Jul 01, 2007 at 08:54:19AM -0700, Michael Snyder wrote:
> > It can't be backwards; this is trying to print "1" or "1, 2" but your
> > change would make it print ", 1" and ", 12".
> >
> > I think it's also trying to shorten ranges to "{1..3, 6..7}".  The
> > bug's got to be in there somewhere.  Why's the code dead?  I'm not
> > seeing it... element_seen can be reset by the bit clear case, and then
> > we'll get into the test you're changing again.
> 
> 'Cause we're not in a loop, and it's not a static variable.
> The code is serial.  At entry we set empty_set to one, and
> then we test to see if it's zero.  It can't be zero.

How sure are you sure we're not in a loop? :-)

      for (i = low_bound; i <= high_bound; i++)
        {
          bitval = value_bit_index (TYPE_FIELD_TYPE (type, field),
                                    (TYPE_FIELD_BITPOS (type, field) /
				    8) +
                                    valaddr + embedded_offset, i);
          if (bitval < 0)
            error (_("bit test is out of range"));
          else if (bitval > 0)
            {
              previous_high = i;
              if (! element_seen)
                {
                  if (! empty_set)
                    fprintf_filtered (stream, ", ");


-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] logic change in m2-valprint.c

[msnyder]

> On Sun, Jul 01, 2007 at 08:54:19AM -0700, Michael Snyder wrote:
>> > It can't be backwards; this is trying to print "1" or "1, 2" but your
>> > change would make it print ", 1" and ", 12".
>> >
>> > I think it's also trying to shorten ranges to "{1..3, 6..7}".  The
>> > bug's got to be in there somewhere.  Why's the code dead?  I'm not
>> > seeing it... element_seen can be reset by the bit clear case, and then
>> > we'll get into the test you're changing again.
>>
>> 'Cause we're not in a loop, and it's not a static variable.
>> The code is serial.  At entry we set empty_set to one, and
>> then we test to see if it's zero.  It can't be zero.
>
> How sure are you sure we're not in a loop? :-)

I'm sorry -- too many balls in the air.  Try this:

91   	  int empty_set        = 1;
92   	  int element_seen     = 0;
...
123  	      for (i = low_bound; i <= high_bound; i++)
...
133  		      if (! element_seen)
134  			{
135  			  if (! empty_set)
136  			    fprintf_filtered (stream, ", ");
137  			  print_type_scalar (target, i, stream);
138  			  empty_set    = 0;
139  			  element_seen = 1;
140  			  previous_low = i;
141  			}

So, the reasoning is as follows: line 136 is dead code, because
line 135 can never test true, because the first time we enter
the block beginning at line 133, empty_set will be true (there
is nowhere else for it to be set false), and there is no second
time -- we will never enter this block again because we will set
"element_seen" to true (and there is nowhere else for it to be
set false again).

Was that clear?

So on pass "n", we enter the block.  Empty_set must be true.
We now set empty set false, and element_seen to true, and
therefore on pass "n + m" we cannot enter the block.  So there
is no point at which we can execute line 136.

Re: [RFC] logic change in m2-valprint.c

[Daniel Jacobowitz]

On Mon, Jul 02, 2007 at 05:05:12PM -0700, Michael Snyder wrote:
> is nowhere else for it to be set false), and there is no second
> time -- we will never enter this block again because we will set
> "element_seen" to true (and there is nowhere else for it to be
> set false again).
> 
> Was that clear?

Clear, but not right.

    152                   element_seen = 0;

On entry, empty_set = 1 and element_seen = 0.  We see an element,
which causes us set empty_set = 0 and element_seen = 1.  Then we see a
clear bit in the set and set element_seen to 0 and not change
empty_set.  Then we see a set bit, and element_seen == 0 with
empty_set == 0.  We print the comma.

I would test it, but I don't have an M-2 compiler and the expression
parser can't create sets.  Am I missing something?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] logic change in m2-valprint.c

[msnyder]

> On Mon, Jul 02, 2007 at 05:05:12PM -0700, Michael Snyder wrote:
>> is nowhere else for it to be set false), and there is no second
>> time -- we will never enter this block again because we will set
>> "element_seen" to true (and there is nowhere else for it to be
>> set false again).
>>
>> Was that clear?
>
> Clear, but not right.
>
>     152                   element_seen = 0;
>
> On entry, empty_set = 1 and element_seen = 0.  We see an element,
> which causes us set empty_set = 0 and element_seen = 1.  Then we see a
> clear bit in the set and set element_seen to 0 and not change
> empty_set.  Then we see a set bit, and element_seen == 0 with
> empty_set == 0.  We print the comma.
>
> I would test it, but I don't have an M-2 compiler and the expression
> parser can't create sets.  Am I missing something?

You've convinced me -- I don't understand this one.
I'm going to kick it back to Coverity -- consider this one withdrawn.

Re: [RFC] logic change in m2-valprint.c

[Daniel Jacobowitz]

On Mon, Jul 02, 2007 at 06:20:58PM -0700, Michael Snyder wrote:
> You've convinced me -- I don't understand this one.
> I'm going to kick it back to Coverity -- consider this one withdrawn.

Thanks, could you let me know what they say?  I can't see the mistake,
but probably it has to do with why they marked empty_set as const...

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] logic change in m2-valprint.c

[msnyder]

> On Mon, Jul 02, 2007 at 06:20:58PM -0700, Michael Snyder wrote:
>> You've convinced me -- I don't understand this one.
>> I'm going to kick it back to Coverity -- consider this one withdrawn.
>
> Thanks, could you let me know what they say?  I can't see the mistake,
> but probably it has to do with why they marked empty_set as const...

Of course.  FYI, I already have two "false hit" reports filed with
them, and since they claim that most reported false hits aren't really
false, it will be quite interesting to see how they turn out.

Re: [RFC] logic change in m2-valprint.c

[Mark Kettenis]

> Date: Mon, 2 Jul 2007 21:11:47 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Mon, Jul 02, 2007 at 05:05:12PM -0700, Michael Snyder wrote:
> > is nowhere else for it to be set false), and there is no second
> > time -- we will never enter this block again because we will set
> > "element_seen" to true (and there is nowhere else for it to be
> > set false again).
> > 
> > Was that clear?
> 
> Clear, but not right.
> 
>     152                   element_seen = 0;
> 
> On entry, empty_set = 1 and element_seen = 0.  We see an element,
> which causes us set empty_set = 0 and element_seen = 1.  Then we see a
> clear bit in the set and set element_seen to 0 and not change
> empty_set.  Then we see a set bit, and element_seen == 0 with
> empty_set == 0.  We print the comma.
> 
> I would test it, but I don't have an M-2 compiler and the expression
> parser can't create sets.  Am I missing something?

Dunno, but it is obvious that this bit of code is in need of a rewrite.

Re: [RFC] logic change in m2-valprint.c

[Jim Blandy]

Mark Kettenis <mark.kettenis@xs4all.nl> writes:
>> Date: Mon, 2 Jul 2007 21:11:47 -0400
>> From: Daniel Jacobowitz <drow@false.org>
>> 
>> On Mon, Jul 02, 2007 at 05:05:12PM -0700, Michael Snyder wrote:
>> > is nowhere else for it to be set false), and there is no second
>> > time -- we will never enter this block again because we will set
>> > "element_seen" to true (and there is nowhere else for it to be
>> > set false again).
>> > 
>> > Was that clear?
>> 
>> Clear, but not right.
>> 
>>     152                   element_seen = 0;
>> 
>> On entry, empty_set = 1 and element_seen = 0.  We see an element,
>> which causes us set empty_set = 0 and element_seen = 1.  Then we see a
>> clear bit in the set and set element_seen to 0 and not change
>> empty_set.  Then we see a set bit, and element_seen == 0 with
>> empty_set == 0.  We print the comma.
>> 
>> I would test it, but I don't have an M-2 compiler and the expression
>> parser can't create sets.  Am I missing something?
>
> Dunno, but it is obvious that this bit of code is in need of a rewrite.

Yeah.  There's actually a nested loop there, but it's not easy to
see.  And the name 'empty_set' suggests something one can't know until
the traversal is complete...

I should stay away from 'beautiful code' contents, but is this any
better?

static void
m2_print_long_set (struct type *type, const gdb_byte *valaddr,
		   int embedded_offset, CORE_ADDR address,
		   struct ui_file *stream, int format,
		   enum val_prettyprint pretty)
{
  int field, len;
  LONGEST low_bound, high_bound;
  int printed_any = 0;
  struct type *target = NULL;

  /* We want to print runs of members using the '1..10' notation, but
     we don't know when a run has ended until we see a zero bit.  We
     set run_pending when we see what may be the start of a run; when
     run_pending is set, run_low and run_high record the known extent
     of the run.  */
  int run_pending = 0;
  LONGEST run_low = 0;
  LONGEST run_high = 0;

  CHECK_TYPEDEF (type);

  if (! get_long_set_bounds (type, &low_bound, &high_bound))
    {
      fprintf_filtered (stream, "{ %s }", _("<unknown bounds of set>"));
      return;
    }

  /* Walk the fields of the set, printing elements from each.  */
  fprintf_filtered (stream, "{");
  len = TYPE_NFIELDS (type);
  for (field = TYPE_N_BASECLASSES (type); field < len; field++)
    {
      struct type *field_range;
      LONGEST field_range_low, field_range_high;
      int i;

      field_range = TYPE_INDEX_TYPE (TYPE_FIELD_TYPE (type, field));
      if (get_discrete_bounds (field_range,
                               &field_range_low, &field_range_high)
          < 0)
        break;

      target = TYPE_TARGET_TYPE (field_range);
      if (target == NULL)
        target = builtin_type_int;
      
      for (i = field_range_low; i <= field_range_high; i++)
        {
          int bitval = value_bit_index (TYPE_FIELD_TYPE (type, field),
                                        (TYPE_FIELD_BITPOS (type, field) / 8) +
                                        valaddr + embedded_offset, i);
          if (bitval < 0)
            error (_("bit test is out of range"));
          else if (bitval > 0)
            {
              if (run_pending)
                /* Extend the existing run.  */
                run_high = i;
              else
                {
                  /* Start a new run.  */
                  run_pending = 1;
                  run_low = run_high = i;
                }

              /* We print things when we find the end of the run.  */
            }
          else
            {
              /* If we had a run going, we've now found its end, so
                 print it.  */
              if (run_pending)
                {
                  if (printed_any)
                    fprintf_filtered (stream, ", ");

                  print_type_scalar (target, run_low, stream);
                  if (run_low < run_high)
                    {
                      fprintf_filtered (stream, "..");
                      print_type_scalar (target, run_high, stream);
                    }

                  run_pending = 0;
                  printed_any = 1;
                }
            }
        }
    }

  /* Flush any pending range abutting the end of the set.  */
  if (run_pending)
    {
      if (printed_any)
        fprintf_filtered (stream, ", ");

      print_type_scalar (target, run_low, stream);
      if (run_low < run_high)
        {
          fprintf_filtered (stream, "..");
          print_type_scalar (target, run_high, stream);
        }
    }
                  
  fprintf_filtered (stream, "}");
}