On 12/10/2015 03:09 PM, Markus Armbruster wrote:
Marcel Apfelbaum <marcel.apfelb...@gmail.com> writes:
On 12/10/2015 12:29 PM, Markus Armbruster wrote:
isa_bus_irqs(), isa_create() and isa_try_create() call hw_error() when
passed a null bus. Use of hw_error() has always been questionable,
because these are used only during machine initialization, and
printing CPU registers isn't useful there.
Since the previous commit, passing a null bus is a programming error.
Drop the hw_error() and simply let it crash.
Maybe we can be a little nicer add an assert ? :)
assert(p) before dereferencing p only converts one kind of crash into
another one. I tend to do it only when the assert(p) does double-duty
as useful documentation. Or perhaps when I think there's a real risk of
running into !p in an environment where core dumps are off[*] and
reproducing the failure with a debugger attached could be hard.
To use these three functions, you need an ISABus *. How could you end
up with a bad one?
* You forget to create the ISA bus, and the compiler is too confused to
notice. You'll pass an unitialized ISABus, and asserting it's not
null is unlikely to help.
* You create multiple ISA buses (that's the only way creating one can
fail) *and* forget to check for errors. If you pull that off, I'd
expect it to explode even in light testing.
This is the scenario I was referring to.
You are perfectly right that using assert will change a crash into another,
but when I am "succeeding" to crash some code (and I am good at it :)),
if I see a NULL pointer de-reference I am starting to wonder if *is possible*
to have a NULL pointer there and the developer didn't take that into account.
(it couldn't me my bug, right, it got be his :))
However, if I see an assert crash *I know* the developer did not expect a NULL
pointer to be passed at all.
For this specific scenario, (multiple ISA buses) maybe is such a strange case
that we don't need to bother with an assert.
Thanks for the detailed explanation,
Marcel
* Your pointer gets corrupted between correct initialization and use.
Asserting it's not null is unlikely to help.
[*] Switching them off on a development machine forfeits your
developer's license, as far as I'm concerned :)
