Re: [PATCH 5/7] dlopen.3: Remove unneeded cast

["Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>]

Hi Alex,

On 9/6/20 3:22 PM, Alejandro Colomar wrote:
> Hola Michael,
> 
> On 9/6/20 3:02 PM, Michael Kerrisk (man-pages) wrote:
>> Hello Alex,
>>
>> On 9/5/20 5:14 PM, Alejandro Colomar wrote:
>>> Casting `void *` to `double (*cosine)(double)` is already done
>>> implicitly.
>>> I had doubts about this one, but `gcc -Wall -Wextra` didn't complain
>>> about it.
>>> Explicitly casting can silence warnings when mistakes are made, so it's
>>> better to remove those casts when possible.
>>>
>>> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
>>> ---
>>>  man3/dlopen.3 | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/man3/dlopen.3 b/man3/dlopen.3
>>> index 8e18f70c0..2de358ea3 100644
>>> --- a/man3/dlopen.3
>>> +++ b/man3/dlopen.3
>>> @@ -581,7 +581,7 @@ main(void)
>>>
>>>      dlerror();    /* Clear any existing error */
>>>
>>> -    cosine = (double (*)(double)) dlsym(handle, "cos");
>>> +    cosine = dlsym(handle, "cos");
>>>
>>>      /* According to the ISO C standard, casting between function
>>>         pointers and 'void *', as done above, produces undefined results.
>>
>> This cast really is needed. See the comment just below, and also try
>> compiling the code with your patch applied:
>>
>> cc -pedantic -Wall prog.c
>> d.c: In function ‘main’:
>> d.c:21:19: warning: ISO C forbids assignment between function pointer
> and ‘void *’ [-Wpedantic]
>>    21 |            cosine = dlsym(handle, "cos");
>>       |                   ^
> 
> Hmmm, not sure about it.
> 
> The thing is, standard C doesn't allow this, no matter how.  

Agreed.

> POSIX does allow it, however.

Yes, POSIX is explicit on this point, and the specification
gives an example of the use casts in the manner shown in the
manual page.

> The only thing with the casts is to avoid the warning, but they don't
> avoid the possible undefined behaviour (only in non-POSIX systems).

Yes. (But, dlopen() is a "UNIX-only" API, and thus non-POSIX
doesn't matter here.)

> But that warning, `-pedantic`, is specifically targeted to warn about
> whatever code that is not strict standard C, which this code isn't,
> so the warning is legit IMHO, and anyone using `-pedantic` would
> probably be warned about this line, and anyone not wanting to be warned
> about this line should probably disable `-pedantic`.
> 
> So, in POSIX, without `-pedantic`, that line without casts will result
> in correct code and no warnings, as expected.
> 
> And in non-POSIX, with `-pedantic`, that line without casts will
> correctly result in a warning.
> 
> And more importatnly, in non-POSIX, with `-pedantic`, that line with
> casts will result in no warnings but undefined results.
> 
> I'd say that no casting is less problematic than casting, although both
> have their problems.

Two things:

* The standard uses the casts, and allows the extension on top of
what the C standard permits here. So, I think the manuial page
better use the casts also.
* Sometimes people have to compile a large body of code using
certain compiler options, perhaps including "-pedantic", so they
need to at least be aware of the warning that the cast may incur.

To address the second point, I make use of the appropriate pragma,
to eliminate the waring from -pedantic:

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wpedantic"
    funcp = (void (*)(void)) dlsym(libHandle, name);
#pragma GCC diagnostic pop

Thanks,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/