527006 - Numerous compiler warnings in NSPR code and tests

Status: NEW

(NSPR :: NSPR, defect)

Description

[Mikhail T.]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; uk; rv:1.9.1.4) Gecko/20091016 Firefox/3.5.4
Build Identifier: 4.8.2

Even during a "vanilla" build, gcc's -Wall triggers a substantial amount of warnings. Most of them are trivial, but some -- especially, in the tests area -- are a little scarier.

The attached patches fix all of them allowing for a clean `-Wall -Werror' builds as tested on FreeBSD/i386 and FreeBSD/amd64.

Reproducible: Always

Comment 1

[Mikhail T.]

Attachment: Fix warnings in the lib/tests area

* Fixes printf-formats
* removes unused static functions
* removes unused variables

Comment 2

[Mikhail T.]

Attachment: Call thread-ID pthread_t, rather than cast it to a 32-bit value

Change signature of PR_GetThreadID to return pthread_t, instead of PRUint32.

Comment 3

[Mikhail T.]

Attachment: Fix warnings in pr/src/pthreads/ptio.c

This patch is not a pure warning-fixer. Different hunks:

* use socklen_t instead of int for a variable, whose address is sent to getsockopt() -- socklen_t is often unsigned
* set the IPV6_V6ONLY socket option, if available -- this hunk is FreeBSD-specific, but, likely, needn't be
* initialize a variable in two functions, because compiler is concerned, it might be used uninitialized (it might not)

Comment 4

[Mikhail T.]

Attachment: Fix miscellaneous warnings in the code

Miscellaneous warnings/bug fixes throughout the code:

* remove unused variables
* initialize the ones, compiler suspects, could be used unitialized
* declare PR_Assert regardless of the DEBUG flag -- the function is always
  compiled into the library, and is called in at least one location

Comment 5

[Mikhail T.]

Attachment: Fix warnings and bugs in the pr/tests/

The fixes are many:

* printf-formatting
* casting to the big-enough integers
* removal of unused variables

Changes to tests/Makefile.in improved the test-harness, in my opinion.

Comment 6

[Wan-Teh Chang]

Mikhail: thanks for submitting these patches to us.  I
have seen at least some of these patches in the NSPR
package in FreeBSD ports.  Some of these patches I
won't be able to take (such as the API change), but
I will check in as many patches as possible.

Comment 7

[Mikhail T.]

Sure. The patches are part of the FreeBSD port -- although some latest hunks are still awaiting the port-maintainer's action (upgrade to 4.8.2).

As far as the API change (pthread_t), that needs to happen, if you are to ever properly support 64-bit systems... pthread_t is a usually a pointer (to the thread's internal structure). Casting that down to 32 bits, as is currently done by unpatched NSPR, is what will prevent a functional 64-bit multi-threading application from ever appearing :-(

If you have to keep the API-compatibility forever, you'll need to reimplement the function to maintain an internal table mapping pthread_t pointers to integer values. This will be slow, and, likely, break some existing apps, where the developers have already assumed (incorrectly) that what they get from the function is the pthread_t...

Comment 8

[Mikhail T.]

Sorry for the spam -- more to the point of the API-change... On the majority of the 32-bit systems, there will be no ABI-change by switching to pthread_t -- so the existing applications will need no recompilation to work with the fixed NSPR.

And the 64-bit systems could not work properly before anyway -- at least, they'll become usable after a recompile. Maybe, plain pthread_t is too straightforward -- but some sort of PRPthreadId is needed then (meaning simply pthread_t in most cases).

Comment 9

[Jan Beich]

Comment on attachment 410796 [details] [diff] [review]
Fix warnings in pr/src/pthreads/ptio.c

Review of attachment 410796 [details] [diff] [review]:
-----------------------------------------------------------------

This should block meta bug 512076 so it lists all uncommitted downstream patches that are waiting or stalled for review.

(In reply to Mikhail Teterin from comment #2)
> Created attachment 410794 [details] [diff] [review]
> Call thread-ID pthread_t, rather than cast it to a 32-bit value

Duplicate of bug 301986. Please, update patch there.

::: ../pr/src/pthreads/ptio.c
@@ +3498,5 @@
> +#if (defined(_PR_INET6_PROBE) || defined(_PR_INET6)) && \
> +	defined(__FreeBSD__) && defined(IPV6_V6ONLY)
> +		if (domain == PR_AF_INET6) {
> +			int opt = 0;
> +			if (setsockopt(osfd, IPPROTO_IPV6, IPV6_V6ONLY,

Keep non-warnings fixes out, please. And this was fixed by bug 266981. The hunk will be removed on the next gecko ports update, i.e. when 15.0 is released.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.