410677 - [PATCH] Fix NSPR warnings with -Wstrict-prototypes

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P2)

Description

[rlaager]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en; rv:1.8.1.11) Gecko/20071204 Epiphany/2.20 Firefox/2.0.0.11
Build Identifier: libnspr4-dev 4.6.6-3 from Ubuntu

I compiled Pidgin with -Wstrict-prototypes and got some warnings from NSPR. Attached is a patch to fix those warnings. I haven't done any more looking into NSPR to find other instances of this problem, so I can't say for sure if this is it or not.

Reproducible: Always

Comment 1

[rlaager]

Attachment: A proposed patch to fix this (checked in).

Comment 2

[Wan-Teh Chang]

Thanks for the bug report and the patch.

Since prlink.h is a public header file and this changes two typedefs, it may break
source compatibility, so I'm afraid that we can't take this patch.  Is it possible to
tell gcc to ignore strict prototype warnings about these two typedefs?

Comment 3

[rlaager]

(In reply to comment #2)
> Thanks for the bug report and the patch.
> 
> Since prlink.h is a public header file and this changes two typedefs, it may
> break
> source compatibility, so I'm afraid that we can't take this patch.

Are these functions supposed to take arguments or not? In other words, what is the correct prototype here?

Comment 4

[Dan Horák]

The same bug has been filled for Fedora/RHEL - https://bugzilla.redhat.com/show_bug.cgi?id=451616 
So is there any chance for a solution?

Comment 5

[Kai Engert [:KaiE:]]

If I understand correctly, the type PRFuncPtr is supposed to work with ANY function signature.

The functions that use this function-pointer type appear to operate on generic functions, which have any signature.

I conclude the proposed patch is wrong, as it would only work with functions that have a specific function signature: "empty parameter list".

Is my understanding right or wrong?

Comment 6

[Wan-Teh Chang]

The type PRFuncPtr is like the void * pointer for function pointers.
PRFuncPtr is intended to be a universal function pointer type.
It should be cast to a specific function pointer type before calling
the function.  So the function signature of PRFuncPtr doesn't matter.

I'm worried that the proposed change may break the compilation of
existing source code.  Here is an example I made up:

PRBool foo(PRLibrary *lib, const char *name)
{
    /*
     * Instead of
     *     PRFuncPtr func_ptr;
     * we declare the variable like this:
     */
    void (*func_ptr)();

    func_ptr = PR_FindFunctionSymbol(lib, name);
    return func_ptr != NULL;
}

I'm worried that changing the definition of PRFuncPtr
would make the compilation fail.  We should test the
above code on all the platforms we support.  I've tested
GCC on Linux and the code still compiles.

Comment 7

[rlaager]

(In reply to comment #5)
> If I understand correctly, the type PRFuncPtr is supposed to work with ANY
> function signature.
> 
> The functions that use this function-pointer type appear to operate on generic
> functions, which have any signature.
> 
> I conclude the proposed patch is wrong, as it would only work with functions
> that have a specific function signature: "empty parameter list".

This seems to be the case. Why not just use void * instead? This is similar to what glib does (FYI, gpointer === void * and they're using an output parameter rather than returning it): http://library.gnome.org/devel/glib/stable/glib-Dynamic-Loading-of-Modules.html#g-module-symbol

Comment 8

[Wan-Teh Chang]

Any pointer to an object may be converted to void *.
But a function pointer is not an object pointer.  So
for portability we want to use a generic function pointer
type rather than void *.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

I recommend WONTFIX.

Comment 10

[Dan Horák]

I think better would be first to know an opinion from some GCC or C standard people what is the best practice in such situation.

Comment 11

[Wan-Teh Chang]

We have several options.

1. Change the implementation of the GCC -Wstrict-prototypes
to only warn about actual invocation of a function declared
without specifying the argument types.   That is, the mere
declaration of a function without specifying the argument
types should not cause a warning.  Only the act of calling
such a function should be warned.

This is to recognize the existence of code that defines a
generic function pointer type like this:

  typedef int (*func_ptr_t)();

or this:

  typedef void (*func_ptr_t)();

2. Do not use -Wstrict-prototypes on files that include "prlink.h".

3. Ignore the -Wstrict-prototypes warnings in "prlink.h".

4. Change "prlink.h" to suppress this warning.  I think

#pragma GCC diagnostic ignored "-Wstrict-prototypes"

should work.  But I don't know how to make it apply to
only certain lines in "prlink.h", and restore to the *original
setting*.

5. Change the definition of PRFuncPtr in "prlink.h".  As
I explained before, NSPR needs to be backward compatible
(at both source and binary levels), which means existing
code should continue to compile against new versions of
NSPR headers.  This is why I have to perform a lot of
experiments (GCC, Visual C++, Sun Studio, HP C, and IBM
C compilers) to see if changing the definition of
PRFuncPtr breaks the compilation or generates compiler
warnings on the sample code I gave in comment 6.

Comment 12

[Guenter]

Folks, this bug is now 20 months old, and still no solution to this.
This is really bad practice to just ignore such bug reports since we deal here with a security-related library, and you make folks more and more accept compiler warnings which seem to be fixable from your side. When I f.e. compile a project like libcurl with NSS support these warnings in prlink.h add up to ~100 times! And disabling the warnings with -Wno-strict-prototypes is really not a solution.
BTW. I found some more places where I get similar warnings:
--- cert.h.orig	2009-07-01 01:05:24.000000000 +0200
+++ cert.h	2009-09-06 18:11:04.000000000 +0200
@@ -1610,25 +1610,25 @@
  * Returns a pointer to a static structure.
  */
 extern const CERTRevocationFlags*
-CERT_GetPKIXVerifyNistRevocationPolicy();
+CERT_GetPKIXVerifyNistRevocationPolicy(void);
 
 /*
  * Returns a pointer to a static structure.
  */
 extern const CERTRevocationFlags*
-CERT_GetClassicOCSPEnabledSoftFailurePolicy();
+CERT_GetClassicOCSPEnabledSoftFailurePolicy(void);
 
 /*
  * Returns a pointer to a static structure.
  */
 extern const CERTRevocationFlags*
-CERT_GetClassicOCSPEnabledHardFailurePolicy();
+CERT_GetClassicOCSPEnabledHardFailurePolicy(void);
 
 /*
  * Returns a pointer to a static structure.
  */
 extern const CERTRevocationFlags*
-CERT_GetClassicOCSPDisabledPolicy();
+CERT_GetClassicOCSPDisabledPolicy(void);
 
 /*
  * Verify a Cert with libpkix
@@ -1662,7 +1662,7 @@
 
 /* The function return PR_TRUE if cert validation should use
  * libpkix cert validation engine. */
-PRBool CERT_GetUsePKIXForValidation();
+PRBool CERT_GetUsePKIXForValidation(void);
 
 SEC_END_PROTOS

I think you should just apply the void fix, and as time passes by we will see if others will report new problems with the fixes (which I doubt).

Comment 13

[Guenter]

Hi  Wan-Teh Chang,
(In reply to comment #11)
> We have several options.
> 4. Change "prlink.h" to suppress this warning.  I think
> 
> #pragma GCC diagnostic ignored "-Wstrict-prototypes"
> 
> should work.  But I don't know how to make it apply to
> only certain lines in "prlink.h", and restore to the *original
> setting*.
http://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html
this works for me:
#pragma GCC diagnostic ignored "-Wstrict-prototypes"
#include <nspr.h>
#pragma GCC diagnostic warning "-Wstrict-prototypes"

I have tested this pragma with lowest gcc version 4.2.1 - a test with 4.0.2 shows that the pragma is not understood. Will attach a patch against prlink.h with the proper ifdefs surrounded.

Comment 14

[Guenter]

Attachment: new patch against prlink.h to avoid warnings with pragmas

Comment 15

[Guenter]

caveat to the patch I've posted: the pragma
#pragma GCC diagnostic warning "-Wstrict-prototypes"
also overwrites -Werror - that means if -Werror was specified from commandline then after including prlink.h the above pragma would turn it back into warnings.

Comment 16

[Wan-Teh Chang]

Comment on attachment 398986 [details] [diff] [review]
new patch against prlink.h to avoid warnings with pragmas

Guenter,

Thanks a lot for this patch.  Since prlink.h is a header
file included by many non-NSPR files, it'd be inappropriate
for us (NSPR) to turn on the -Wstrict-prototypes warning as
a result of including prlink.h.  So I'm sorry that I can't
accept this patch.

It's too bad that "#pragma GCC diagnostic" doesn't have
an option for restoring to the original setting of a
warning.

Comment 17

[Wan-Teh Chang]

(In reply to comment #12)
Guenter, could you attach your patch for cert.h to NSS
bug 515870 and ask me to review it?  Thanks.

For this bug, I'll just check in [e-mail]'s
patch.  That patch requires testing with several compilers
we need to support -- we need to make sure by silencing
this GCC warning, we don't break compilation or introduce
warnings with other compilers.  So it's not as trivial
as it seems.

Comment 18

[Guenter]

Hi  Wan-Teh Chang,
(In reply to comment #17)
> Guenter, could you attach your patch for cert.h to NSS
> bug 515870 and ask me to review it?  Thanks.
I guess thats not needed - after I posted to this bug I checked here:
http://mxr.mozilla.org/security/source/security/nss/lib/certdb/cert.h
and from that it seems that my cert.h patch went already in before I even posted it :)
I came over these warnings with 3.12.3, but surprisingly with 3.12.4 they were gone, so I checked MXR ...

> For this bug, I'll just check in [e-mail]'s
> patch.  That patch requires testing with several compilers
> we need to support -- we need to make sure by silencing
> this GCC warning, we don't break compilation or introduce
> warnings with other compilers.  So it's not as trivial
> as it seems.
Yes, the void patch seems to be the best approach to try. But you should probably attach a test sample here which others can pick up, and check on their platform if it doesnt harm.
thanks, Günter.

Comment 19

[Wan-Teh Chang]

You're right.  Someone else submitted the same patch for cert.h
in bug 491919.

Comment 20

[Robert Relyea]

> Since prlink.h is a public header file and this changes two typedefs, it 
> may break source compatibility, so I'm afraid that we can't take this patch.

Sigh, the world is complicated. We just had an instance where this broke source code compatibility in NSS because prlink.h was added to a new NSS public header and the lack of ansi compatibility broke several packages.

I just did a search through the NSPR headers and found numerous uses of void in void returning functions, including such mainstays as PR_NewLock(). Is there really any doubt that all compiliers that handle NSPR will handle the voids just fine?

bob

Comment 21

[Wan-Teh Chang]

Bob, shall we check in [e-mail]'s patch (attachment 295261 [details] [diff] [review])
as I proposed in comment 17?  Will that fix the problem you just reported?

Comment 22

[Robert Relyea]

>as I proposed in comment 17?  Will that fix the problem you just reported?

yes, and yes. Elio, no vuvuzela's please.

Comment 23

[Elio Maldonado]

Nagging and annoyance weapons put away. Setting the 4.8.5 target flag. Thanks.

Comment 24

[Wan-Teh Chang]

Comment on attachment 295261 [details] [diff] [review]
A proposed patch to fix this (checked in).

I checked in this patch on the NSPR trunk (NSPR 4.8.6).

Checking in prlink.h;
/cvsroot/mozilla/nsprpub/pr/include/prlink.h,v  <--  prlink.h
new revision: 3.17; previous revision: 3.16
done