I want to replace all statements in the kernel of the form
BUG_ON(condition1 || condition2) with:
BUG_ON(condition1);
BUG_ON(condition2);
I was recently bitten by a very sporadic BUG_ON(cond1 || cond2)
statement and was quite annoyed at the greatly reduced opportunity to
debug the problem. Make these changes and someone who experiences
the problem may be able to provide slightly more useful information.
There are only three other places in the kernel besides the
bug I tripped (Matt Dharm and Greg Kroah-Hartmann have already accepted
the patch that I submitted for that one, in drivers/usb/storage). They
are:
2 in arch/arm/mach-iop310/iq80310-pci.c
12 in fs/ntfs/
23 in fs/partitions/ldm.c
Here is the patch for linux-2.5.30/arch/arm/mach-iop310/iq80310-pci.c.
Please let me know if you are going to shepherd this patch to
Linus's linux-2.5 tree, if you want me to submit it to Linus or
someone else, or if there is some other way you'd like me to proceed.
Thanks for your work on Linux on ARM.
--
Adam J. Richter __ ______________ 575 Oroville Road
[email protected] \ / Milpitas, California 95035
+1 408 309-6081 | g g d r a s i l United States of America
"Free Software For The Rest Of Us."
On Mon, Aug 05, 2002 at 01:17:40PM -0700, Adam J. Richter wrote:
> I want to replace all statements in the kernel of the form
> BUG_ON(condition1 || condition2) with:
>
> BUG_ON(condition1);
> BUG_ON(condition2);
Why? In the case below, its one logical error (value out of range).
The register dump tells you more information. In fact, I don't care
which side of less than 1 or greater than 4 pin actually is. It's
indicating a bug in the PCI subsystem either way, and the analysis
is the same in either case.
> I was recently bitten by a very sporadic BUG_ON(cond1 || cond2)
> statement and was quite annoyed at the greatly reduced opportunity to
> debug the problem. Make these changes and someone who experiences
> the problem may be able to provide slightly more useful information.
This would make sense of the two conditions were unrelated to each
other.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
Russell King wrote:
>On Mon, Aug 05, 2002 at 01:17:40PM -0700, Adam J. Richter wrote:
>> I want to replace all statements in the kernel of the form
>> BUG_ON(condition1 || condition2) with:
>>
>> BUG_ON(condition1);
>> BUG_ON(condition2);
>Why? In the case below, its one logical error (value out of range).
>The register dump tells you more information. In fact, I don't care
>which side of less than 1 or greater than 4 pin actually is. It's
>indicating a bug in the PCI subsystem either way, and the analysis
>is the same in either case.
Because knowing which way the failure occurred may give you
a clue about what *caused* it.
>> I was recently bitten by a very sporadic BUG_ON(cond1 || cond2)
>> statement and was quite annoyed at the greatly reduced opportunity to
>> debug the problem. Make these changes and someone who experiences
>> the problem may be able to provide slightly more useful information.
>This would make sense of the two conditions were unrelated to each
>other.
Even when two conditions are related, knowing which one failed
gives you more information, and can make it easier to track down the bug,
even when the two parts of the condition seem very simple, especially
considering that some bugs can be sporadic and tracking down bugs
often involves pearing down an exponential tree of possibilities.
That was pretty much the case with the BUG_ON for two
related conditions that I tripped. The lack of information about
the problem was basically enough to tip the scales so that other
things were a better use of my time than trying to track it down
further, although I may come back to it later.
That will probably never happen with "pin < 1 || pin > 4",
because, like most BUG_ON statements, it will probably never be
tripped. Nevertheless, if one of those BUG_ON statements is tripped,
it will probably save someone some valuable developer time, and
reduce the interactions necessary with some user who might not be
that interested in becoming an ARM kernel developer.
All of the other patches that I have submitted to
eliminate BUG_ON(x || y) statements have been accepted by
their maintainers. If you accept the patch for your two BUG_ON
statements, the practice should be completely eliminated from
the kernel once the other maintainers propagate their next
releases to Linus.
Even if you think this case is trivial, you will at least be
leading by example about accomodating quality requests, at least when
somone submits a patch.
Anyhow, please let me know what you want to do or what you
want me to do.
Adam J. Richter __ ______________ 575 Oroville Road
[email protected] \ / Milpitas, California 95035
+1 408 309-6081 | g g d r a s i l United States of America
"Free Software For The Rest Of Us."
Hi!
> I want to replace all statements in the kernel of the form
> BUG_ON(condition1 || condition2) with:
>
> BUG_ON(condition1);
> BUG_ON(condition2);
>
> I was recently bitten by a very sporadic BUG_ON(cond1 || cond2)
> statement and was quite annoyed at the greatly reduced opportunity to
> debug the problem. Make these changes and someone who experiences
> the problem may be able to provide slightly more useful information.
it makes code slower/bigger... probably bad idea
--
Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.
On Thu, Nov 01, 2001 at 11:48:25PM +0000, Pavel Machek wrote:
> it makes code slower/bigger... probably bad idea
Its actually not in a performance critical area, so the "slower" argument
doesn't apply.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
Pavel Machek writes:
>it makes code slower/bigger... probably bad idea
The costs of expanding these two BUG_ON's is trivial,
considering that they get executed something like once per device,
during an initialization process. This is a section of code where I
would think correctness and ease of debugging would be more important,
especially since they might be from bug reports submitted by users who
might have limited tolerance for repeatedly trying new kernels.
If you're really squeezed for space, you're probably going to
have to define BUG() and BUG_ON() to something smaller that does not
generate a file name and line number (include/asm-i386/page.h an "#if"
for this, although mips does not), or even defining them as nothing.
By the way, if no bug is detected, BUG_ON(cond1 || cond2)
executes the same instructions as BUG_ON(cond1); BUG_ON(cond2),
although there are probably greater instruction prefetch costs due to
the the ~20 bytes of code (that is normally not executed) for the
different call or trap instruction, and instruction cache issues.
Adam J. Richter __ ______________ 575 Oroville Road
[email protected] \ / Milpitas, California 95035
+1 408 309-6081 | g g d r a s i l United States of America
"Free Software For The Rest Of Us."