[Bug gas/30860] Possible usage of NULL in GAS

[nickc at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=30860

Nick Clifton <nickc at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |nickc at redhat dot com

--- Comment #2 from Nick Clifton <nickc at redhat dot com> ---
(In reply to jacob from comment #1)

> 
>            know(fragP->fr_next); // Not active if NDEBUG is defined
>            after = fragP->fr_next->fr_address + stretch;
> We know that fragP->fr_next CAN BE NULL, since the controlling condition of
> this loop (the switch is inside a loop) is:
>  for (fragP = segment_frag_root; fragP; fragP = fragP->fr_next) {
> 
> So, we RELY on fr_next being NULL to stop the loop. But we do not test for
> this condition before using it here. As you know the "know" construct is
> NOT active if NDEBUG is active.
> PRPOSED FIX:
> Replace "know" by "gas_assert"

Actually this is wrong.  The point of compiling with NDEBUG active
is that sanity checks like the one above are removed making the code
(theoretically) faster.  Anyone using programs compiled with NDEBUG
must take on the risk that the program will fail in an unhelpful way
if the input is not as expected.

So replacing know() with gas_assert() will remove the benefit of
compiling with NDEBUG.  Of course this leads to the question - why 
have a gas_assert() function at all, why not just use know() all 
the time ?  The answer is partially programmer preference and
partially assumption of risk.  If the programmer is sure that a 
failing a particular test can only happen if something else is wrong
elsewhere in the code, eg a rs_org fragment not being followed by
another fragment, then using know() makes sense.  If the test fails
then there is a bug elsewhere and this code is not at fault.  So it
should be safe to remove the test when compiling with NDEBUG - using
NDEBUG implies that the user is happy with the code's behaviour and
does not want extra tests.

If on the other hand, the programmer knows that an issue can arise
because of input over which the code has no control - eg the assembler
source input file - and there is no easy way to report this error back
to the user, then using gas_assert() makes sense.  The code cannot
continue and cannot go back, so halting with an error message is the
only option.

Having said all of that, there probably are places in the current code 
where it would be better to use gas_assert() instead of know() and 
places where it would be better to report an error back to the function 
caller rather than halting.  It is just that, in my opinion, write.c:3014
is not one of them.

-- 
You are receiving this mail because:
You are on the CC list for the bug.