fs/minix/bitmap.c is broken in CVS

fs/minix/bitmap.c is broken in CVS

[Manuel Novoa III]

Hello,

Apparently during the recent changes, someone replaced the code
sequence

    if (j == find_first_zero_bit(bh->b_data, 8192))
	panic("still zero bit!%d\n", j);

with

    j == (block_t) find_first_zero_bit(bh->b_data, 8192);
    if (j)
	panic("still zero bit!%d\n", j);

in function minix_new_block in file fs/minix/bitmap.c.

Manuel

Re: fs/minix/bitmap.c is broken in CVS

[Riley Williams]

Hi Manuel.

> Apparently during the recent changes, someone replaced the code
> sequence
>
>    if (j == find_first_zero_bit(bh->b_data, 8192))
>	panic("still zero bit!%d\n", j);
>
> with
>
>    j == (block_t) find_first_zero_bit(bh->b_data, 8192);
>    if (j)
>	panic("still zero bit!%d\n", j);
>
> in function minix_new_block in file fs/minix/bitmap.c.

If they did, it wouldn't compile. However, there is only one = sign
(it's the assignment operator, not the equality comparison operator
that occurs there).

I also replaced similar sequences elsewhere in the kernel source.

I've been running the ELKS kernel source through the splint source
checking tool, and cleaning the source up so that splint doesn't report
any warnings whatsoever in its -weak analysis mode. Here's the version
of those two lines that reports clean as far as splint is concerned...

>    if ((j = ((block_t) find_first_zero_bit(bh->b_data, 8192))))
>	panic("still zero bit!%d\n", j);

I don't know about you, but there's far too many brackets in there for
my liking, which is why I split it into two lines. It doesn't affect the
code produced.

Best wishes from Riley.

Re: fs/minix/bitmap.c is broken in CVS

[Manuel Novoa III]

Hello Riley,

On Thu, Jun 20, 2002 at 06:45:03PM +0100, Riley Williams wrote:
> Hi Manuel.
> 
> > Apparently during the recent changes, someone replaced the code
> > sequence
> >
> >    if (j == find_first_zero_bit(bh->b_data, 8192))
> >	panic("still zero bit!%d\n", j);
> >
> > with
> >
> >    j == (block_t) find_first_zero_bit(bh->b_data, 8192);
> >    if (j)
> >	panic("still zero bit!%d\n", j);
> >
> > in function minix_new_block in file fs/minix/bitmap.c.
> 
> If they did, it wouldn't compile. 

Sure it would compile.  A better compiler than bcc would probably issue
a warning because the result of the == operator would be discarded, but
it is still valid C.

>           However, there is only one = sign
> (it's the assignment operator, not the equality comparison operator
> that occurs there).

Riley, look at the code.  It is _supposed_ to be == since the intent
is to check and make sure the bit you want set by set_bit has actually
been set.  I found this because trying to write to a new file was
causing a kernel panic.

> I also replaced similar sequences elsewhere in the kernel source.

Unfortunatly, doing so can be an anti-optimization wrt size when
using bcc.  My size reduction patch probably reversed a number of
your changes.

> I've been running the ELKS kernel source through the splint source
> checking tool, and cleaning the source up so that splint doesn't report
> any warnings whatsoever in its -weak analysis mode. Here's the version
> of those two lines that reports clean as far as splint is concerned...
> 
> >    if ((j = ((block_t) find_first_zero_bit(bh->b_data, 8192))))
> >	panic("still zero bit!%d\n", j);

If you check the older versions in cvs, you'll see that it was == for
comparison, not = for assignment.

> I don't know about you, but there's far too many brackets in there for
> my liking, which is why I split it into two lines. It doesn't affect the
> code produced.

The parenthesis don't bother me as long as the code is formatted
reasonably. And there are situations were it _does_ affect the code
produced.  If you'd like, I'd be happy to provide you with some
examples.  That would have to wait until Monday or so as I am currently
on the road.

> Best wishes from Riley.

And to you,

Manuel

Re: fs/minix/bitmap.c is broken in CVS

[Riley Williams]

Hi Manuel.

>>> Apparently during the recent changes, someone replaced the
>>> code sequence
>>>
>>>     if (j == find_first_zero_bit(bh->b_data, 8192))
>>>         panic("still zero bit!%d\n", j);
>>>
>>> with
>>>
>>>     j == (block_t) find_first_zero_bit(bh->b_data, 8192);
>>>     if (j)
>>>         panic("still zero bit!%d\n", j);
>>>
>>> in function minix_new_block in file fs/minix/bitmap.c.

>> If they did, it wouldn't compile. 

> Sure it would compile. A better compiler than bcc would probably
> issue a warning because the result of the == operator would be
> discarded, but it is still valid C.

My apologies for that thinko - you are of course right there.

>> However, there is only one = sign (it's the assignment operator,
>> not the equality comparison operator that occurs there).

> Riley, look at the code. It is _supposed_ to be == since the intent
> is to check and make sure the bit you want set by set_bit has
> actually been set. I found this because trying to write to a new
> file was causing a kernel panic.

I have to admit that this is one case where I misread the code as I was
dealing with the error messages produced by `make lint` with the SPLINT
code-checking tool installed on my system.

>> I also replaced similar sequences elsewhere in the kernel source.

> Unfortunatly, doing so can be an anti-optimization wrt size when
> using bcc. My size reduction patch probably reversed a number of
> your changes.

The sequences I replaced (with the exception of that one) were all cases
of single = being used inside if statements, and splint complains like
mad concerning the existing versions. One thing I have checked right
through my splint'ing the ELKS source is that the code size as compiled
using `make defconfig && make dep && make Image` does not increase, and
in fact, I have reduced the compiled size by 370 bytes simply by fixing
the warnings that splint produced.

>> I've been running the ELKS kernel source through the splint source
>> checking tool, and cleaning the source up so that splint doesn't
>> report any warnings whatsoever in its -weak analysis mode. Here's
>> the version of those two lines that reports clean as far as splint
>> is concerned...
>> 
>>>    if ((j = ((block_t) find_first_zero_bit(bh->b_data, 8192))))
>>>        panic("still zero bit!%d\n", j);

> If you check the older versions in cvs, you'll see that it was ==
> for comparison, not = for assignment.

If I could get into the CVS then I would, but at the moment, as far as I
can tell, the SourceForge CVS service is down. I'll try again in the
morning and report back then.

>> I don't know about you, but there's far too many brackets in there
>> for my liking, which is why I split it into two lines. It doesn't
>> affect the code produced.

> The parenthesis don't bother me as long as the code is formatted
> reasonably. And there are situations were it _does_ affect the code
> produced.

In this case, assuming it was == beforehand, then such is of course
true. However, where it consists of a single = rather than two, the
ONLY times that I have seen this change affect the code produced is
when it reduces the codesize, and in my book, that's what we want.

> If you'd like, I'd be happy to provide you with some examples. That
> would have to wait until Monday or so as I am currently on the road.

No problem either way.

I'll revert that mistake for you once I can get into the CVS anyway, and
will also be trying out your optimisations to verify that the test
configuration here still works with them. Assuming it does, they'll get
implemented as well.

Best wishes from Riley.

Re: fs/minix/bitmap.c is broken in CVS

[Riley Williams]

Hi Manuel.

> Apparently during the recent changes, someone replaced the code
> sequence
>
>    if (j == find_first_zero_bit(bh->b_data, 8192))
>        panic("still zero bit!%d\n", j);
>
> with
>
>    j == (block_t) find_first_zero_bit(bh->b_data, 8192);
>    if (j)
>        panic("still zero bit!%d\n", j);
>
> in function minix_new_block in file fs/minix/bitmap.c.

I've now reverted the above, and have also applied the first patchset
you issued. I expected that particular hunk to fail, but so did both of
the other hunks in that particular file, so obviously something went
wrong just there. I ignored those failures, and applied your second
patchset over it, and that all applied cleanly.

I have now tested the results of doing the above, and it both compiles
and boots cleanly on my test system, so the above has now been committed
to the ELKS CVS tree (which is working again now), with your tweaks
credited to you. I've also done a few tweaks of my own and committed
them as well, but these tweaks are all involved with cleaning up some of
the boot-time messages or with adding debugging lines to the kernel.

If you would care to redo the tweaks you did to fs/minix/bitmap.c
relative to the current CVS tree and let me have them, I'll add those
for you as well, and ditto any other tweaks you may do.

Best wishes from Riley.

Re: fs/minix/bitmap.c is broken in CVS

[Manuel Novoa III]

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

Hi Riley,

On Sat, Jun 22, 2002 at 10:41:57AM +0100, Riley Williams wrote:
> If you would care to redo the tweaks you did to fs/minix/bitmap.c
> relative to the current CVS tree and let me have them, I'll add those
> for you as well, and ditto any other tweaks you may do.

Patch is attached for fs/minix/bitmap.c.  I doubt I'll have much time
in the next week or two to do any further tweaking.

Manuel


[-- Attachment #2: bitmap.diff.gz --]
[-- Type: application/octet-stream, Size: 716 bytes --]

Re: fs/minix/bitmap.c is broken in CVS

[Riley Williams]

Hi Manuel.

>> If you would care to redo the tweaks you did to fs/minix/bitmap.c
>> relative to the current CVS tree and let me have them, I'll add
>> those for you as well, and ditto any other tweaks you may do.

> Patch is attached for fs/minix/bitmap.c. I doubt I'll have much
> time in the next week or two to do any further tweaking.

No problem. I'm committing your tweaks as I type. There is one thing I
don't like which is the ?: operator on the parameter to printk so I've
tweaked round that as well. I checked, and my tweaks didn't change the
actual compiled size at all.

In addition to the above, I've also tweaked it to split one error
message into two separate error messages that need to be separated.
This particular patch increased the codesize by 16 bytes, but this
means that the errors produced are more meaningful in those two cases.

Best wishes from Riley.

Anybody want this stuff?

[Dustin Lang]

Hi,

My girlfriend is making me cull some of my junk... err... treasures.  If
anyone wants any of these chips (free, of course), contact me off-list.

Cheers,
dstn.


CPUs, etc.:

2	NEC V20 (D70108-8)
4	Zilog Z-80 (clones):
		2 Sharp LH0080A (Z80A-CPU-D)
		2 Mostek MK3880 (Z80-CPU)
1	Motorola 6502 clone? UM6502
1	Motorola 6800? ATI CW16800-A
2	8080:
		1 Intel P8080A
		1 NEC D8080A
4	Intel P8216 (4-bit bus driver for 8080)
2	Intel 8212:
		1 Intel D8212
		1 Intel P8212
1	Intel P8228 (system controller for 8080)
1	Intel P8085AH (3 MHz?)
1	Intel P8214
1	Intel D8089-3
1	Mitsubishi? M50747ESP
1	Panasonic? HD64180RP6
4	MC68B45 (CRT controller)
2	Zilog Z0853006PSC (SCC)
2	Motorola MC146818P
1	COM2601
1	TK-10
1	WDC FD1771PL-01 (Floppy disk controller)
1	NEC D765A


DRAM (old!):

16	NEC D41256C-15
8	NEC D4164C-15
32	Oki M3764A-15
8	? KM44C256AP-8
16	? KM4164B-10
16	? KM41256-15
16	? MT1259-10
8	? V53C464P


EPROMs (I can erase these if you want):

3	2716
34	2732
3	2764
6	27128
12	27256