Re: [PATCH 1/4] localename: -Wtautological-pointer-compare

[Paul Eggert <eggert@cs.ucla.edu>]

On 2023-01-14 03:00, Bruno Haible wrote:
> Is that what you are worried about? Should I work on this?

No, please don't bother. I was partly confused by the situation and your 
email helped clear up some of it. Hope you don't mind this followup.

My confusion arose partly because I am accustomed to languages where the 
distinction between null and non-null pointers is checked statically and 
reliably, and I keep forgetting that with C, GCC and Clang are only poor 
approximations to that - though I hope the approximations are slowly 
getting better.


>   * To avoid garbage-in - garbage-out behaviour, which in general encourages
>     the presence of bugs.

Yes, that's a good thing, though in this particular case abort and 
dereferencing NULL have similar effects on typical platforms so this 
doesn't argue for one method or the other.


>   * So that when an invalid argument gets passed and the developer happened
>     to compile with -g, the debugger will point to the exact line of the
>     abort(). When you say "the input should be checked anyway ... dynamically
>     by the MMU", what the user would see is a SIGSEGV, and would start wondering
>     whether the bug is in his code or in our code.

In my experience users don't care whether the application died due to 
SIGABRT or to SIGSEGV.

Also, in my experience the debugger doesn't always point to the exact 
line of the abort(). For example, if there are two abort() calls in the 
same function they are routinely coalesced. And come to think of it, I 
have somewhat better luck with gdb's reports of SIGSEGV from 
dereferencing null pointers (though there are obviously issues there 
too) than I do from abort() calls.

To give a different example: I wouldn't bother with the following code 
(where M and N are int arguments to a function):

     if (n == 0)
       abort ();
     if (n == -1 && m < -INT_MAX)
       abort ();
     return m / n;

and would instead write this:

     return m / n;

as the user and debugging experiences are similar and the shorter form 
simplifies code maintenance. Sure, the longer form is safer for oddball 
platforms, but it's not worth the aggravation.


>   * For documentation purposes: So that it's clear that we have considered
>     the doc of the function, and NULL at this particular place is invalid.

I'm willing to trust the intelligence of the reader to know that if a 
function computes *P, then it's invalid for P to be null.

Admittedly there's a lot of low-quality code out there where this isn't 
true, but here I'm focusing on the relatively high-quality code in 
Gnulib and GNU apps.


>     (This is not evident: For example, does glibc's error_at_line() support a
>     NULL fname argument? Hard to say from
>     <https://www.gnu.org/software/libc/manual/html_node/Error-Messages.html>.)

The documentation is indeed unclear there, but this sort of thing is 
inevitable in any large system using C. For each such function where the 
spec is unclear, we should document whether a null pointer arg is 
allowed, and if it's not allowed the implementation of the function 
shouldn't need to worry about null pointer args (implementers already 
have too much to worry about, and part of the point of an API is 
simplification).