Closed Bug 399700 Opened 12 years ago Closed 4 years ago

Lots of warnings when compiling on OS/2

Categories

(NSS :: Build, defect, P3, minor)

x86
OS/2

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: mozilla, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [build_warning])

Attachments

(3 files, 2 obsolete files)

When compiling NSS on OS/2 I get thousands of warnings in many of the C files. The warnings are probably all of minor importance as NSS has worked for quite a while (the warnings were always present as long as I have built Mozilla and NSS). They include stuff like

- missing initializers for structure members.
- comparison of unsigned expression < 0 is always false
- comparison between signed and unsigned
- signed and unsigned type in conditional expression
- dereferencing type-punned pointer will break strict-aliasing rules

I attach the NSS part of a log file that I got from a recent trunk compile of SeaMonkey which used the NSS_3_12_ALPHA1B tag for checkout.
Attached file gzipped NSS build log (obsolete) —
OK, the plain text log might have been to big, so I attach it after compressing it with gzip.
At least some warnings are covered by bug288730
Target Milestone: --- → 3.12
Version: 3.12 → trunk
Unsetting target milestone in unresolved bugs whose targets have passed.
Target Milestone: 3.12 → ---
Priority: -- → P3
These warning logs are fairly outdated unfortunately. Much code has changed.
Attached file updated gzipped NSS build log (obsolete) —
OK, here is an excerpt from a log building Firefox from mozilla-1.9.1 today. The warnings may have changed or originate in different code but their number did not shrink, on the contrary. :-(
Attachment #284754 - Attachment is obsolete: true
Can you tell me which NSS CVS tag this log matches ? mozilla-1.9.1 doesn't speak to me as an NSS developer.

Also, which version of gcc are you using ? This looks like way too many warnings indeed.

We don't get most of those warnings on other platforms that use gcc, such as Linux. I just checked with gcc 3.4.6 on Linux (RHEL4).

If you point me to OS/2 mozilla build information, that will be useful. I have just done a fresh install of both an OS/2 virtual machine to run, and a native OS/2 install (though the later currently is very bare - no network interface setup yet). I can't guarantee when I will do this, but I will try to check it for myself at home.
I looked at OS2.mk and figured out which compiler flag is causing the extra warnings. It is -W . 

OS/2 uses the following flags for warnings :
-Wall -W -Wno-unused -Wpointer-arith -Wcast-align -Wno-switch

Linux uses :

-Wall -Werror-implicit-function-declaration -Wno-switch

I am quite surprised at this - I would have thought -Wall included all warnings. But amazingly, it doesn't ! 

The gcc doc on warnings covers this :

http://gcc.gnu.org/onlinedocs/gcc-3.4.6/gcc/Warning-Options.html#Warning-Options

See in particular the end of the page, starting at :

"The following -W... options are not implied by -Wall."

These are warnings do seem legit, but there are 38,000 lines of them . sigh.
OS: OS/2 → All
Hardware: x86 → All
Summary: Lots of warnings when compiling NSS on OS/2 → Lots of warnings when compiling with gcc -W
Attachment #377569 - Flags: review?(mozilla)
Attachment #377569 - Flags: review?(mozilla) → review+
Comment on attachment 377569 [details] [diff] [review]
Stop using gcc -W on OS/2 (checked in)

While I haven't tried this in practice, it makes sense to use the same flags as other platforms. (It never occurred to me to compare...)

I'll try NSS trunk tonight with this and upload a new log if there are warnings left.

Btw, I searched for a while for the NSS tag last night, but couldn't find it. As it's now directly imported looking at client.mk doesn't work any more... I now found "Update mozilla-1.9.1 to pick up NSS 3.12.3" as checkin comment to <http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f160224c5221> which was the last update of nss/Makefile.
Attached file nss build log
OK, so this is the build log from current NSS HEAD from CVS copied into mozilla-1.9.1, with -W removed in coreconf/OS2.mk. Now there are only 297 warnings left, many of them are caused by the same headers which are included multiple times. Using

grep -i warning c:\Desktop\nss_build_new.log | sed "s,^.*warn,," | sort | uniq

I now see the following 19 unique warnings:

"EFTYPE" redefined
`crv' might be used uninitialized in this function
`error' might be used uninitialized in this function
`lib_len' might be used uninitialized in this function
`modulusLen' might be used uninitialized in this function
`name_len' might be used uninitialized in this function
`pecParams' might be used uninitialized in this function
`rv_cache' might be used uninitialized in this function
`uint' previously declared here
comparison is always true due to limited range of data type
dereferencing type-punned pointer will break strict-aliasing rules
implicit declaration of function `close'
implicit declaration of function `getpid'
implicit declaration of function `isatty'
int format, APIRET arg (arg 2)
redefinition of `uint'
suggest parentheses around assignment used as truth value
this is the location of the previous definition
unreachable code at beginning of switch statement

The APIRET, EFTYPE, and possibly uint warnings are OS/2 specific, the others could be the same for all platforms.
Attachment #377545 - Attachment is obsolete: true
"implicit declaration" warnings are fatal errors on win32, so they must be 
peculiar to OS/2.
Comment on attachment 377569 [details] [diff] [review]
Stop using gcc -W on OS/2 (checked in)

Checking in OS2.mk;
/cvsroot/mozilla/security/coreconf/OS2.mk,v  <--  OS2.mk
new revision: 1.30; previous revision: 1.29
done
Attachment #377569 - Attachment description: Stop using gcc -W on OS/2 → Stop using gcc -W on OS/2 (checked in)
"implicit declaration" being non-fatal is specific to GCC, I think. At least I frequently see this on Linux, too. Will take a look to see if I can create a patch to fix these remaining warnings.
This patch (against NSS CVS HEAD) fixes all the patches that I can see on OS/2, with some exceptions:
- there are still many warnings about dereferencing type-punned pointers.
  I looked at a few but it seems that it needs a real NSS developer to
  fix these (I always run into macros that I don't understand). An
  example is the one in security/nss/lib/libpkix/pkix/top/pkix_build.c
  where a PKIX_CertChainChecker** is passed as a PKIX_PL_Object**.
  AFAICT those structures don't have any similiarity, so the warning
  could be very valid.

- I don't know what to do about security/nss/lib/freebl/pqg.c:157:
  warning: comparison is always true due to limited range of data type
  That test is probably there for 64bit platforms, but 32bit platforms
  give that error

I also don't know how to _correctly_ fix the EFTYPE warning correctly, because EFTYPE is 79 on OS/2 while EINVAL is 22:
   #define EINVAL   22   /* Invalid argument */
   #define EFTYPE   79   /* Inappropriate file type or format */
Is it important that EFTYPE==EINVAL in dbm?

It seems to me that the warning in security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_crldp.c actually pointed to a real mistake...

Finally, I made a separate bug (bug 493378) about the warning originating in NSPR.
Attachment #377852 - Flags: review?(julien.pierre.boogz)
Attachment #377761 - Attachment description: nss → nss build log
I am changing the description of this bug back to OS/2-specific, since I don't think we want to fix gcc -W warnings on any OS.
OS: All → OS/2
Hardware: All → x86
Summary: Lots of warnings when compiling with gcc -W → Lots of warnings when compiling on OS/2
Comment on attachment 377852 [details] [diff] [review]
fix most warnings visible on OS/2

There are a few issues with this patch.

1) mcom_db.h 

There seems to be some pre-existing inconsistencies that affect all platforms.

See 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/dbm/include/mcom_db.h&rev=3.43#191

and 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/dbm/include/mcom_db.h&rev=3.43#219

I don't think the later code can ever be reached.

In any case, the specific value for EFTYPE doesn't seem to matter.

I think the correct fix is to add

#ifndef EFTYPE

around the definition on line 191 .

2) r+ for secpwd.c

3) I'm not sure if the compiler suggestion for secname.c is correct.

4) the change for ocsp.c is correct, but strangely I didn't get the warning in my OS/2 build unlike you. I am running gcc 3.3.5 CSD3 .

5) r+ for os2_rand.c . 

6) re: pkix_list.c, I'm not sure that the change is correct. It is certainly odd-looking.

7) r+ for pkix_pl_crldp.c

8) r- for the change in pk11merge.c My version of gcc does not, warn, correctly.

In the first hunk, it is actually not possible for error to be used unless lrv is not SECSuccess, in which case error is set.

In the second hunk, the warning is incorrect too.

9) r- for the change in pkcs11c.c . Again, I don't get the warning in my OS/2 build. Your compiler is incorrect in reporting one, but maybe it can't figure out that modulusLen is only used if ckk = CKK_RSA .

10)
for the change in pkcs11u.c, r-

However, in this case, my compiler doesn't warn, and it should. crv may definitely be used uninitialized if the initial PORT_Alloc fails.

However, the fix is not to initialize crv to CKR_OK . In the failure block after
if (myattribute == NULL)
crv should be set to something other than CKR_OK , specifically CKR_HOST_MEMORY .

11) r+ for the change in sftkmod.c

12) r- for the change in lgattr.c .
crv should be set to CKR_ATTRIBUTE_TYPE_INVALID in the default case of the switch(type) statement.

13) r+ for the change in derive.c (though inexplicably my gcc won't warn)

14) r+ for the change in ssl3con.c

15) r+ for sslimpl.h



There is a code freeze on the mozilla/security/nss/lib/freebl, mozilla/security/nss/lib/softoken/*, and perhaps also mozilla/dbm directories at this time, due to FIPS validation. After you take my review comments into account, please generate separate patches for the directories under code freeze, and the directories which are not.
Attachment #377852 - Flags: review?(julien.pierre.boogz) → review-
Yes, mozilla/dbm is also frozen.
Julien, thanks for the detailed review, it will take a bit more time before I can create new patches. For the moment just another remark: your are correct that GCC is wrong about the unintialized variable warnings, because you have if/else cases for the possible outcomes. But it doesn't hurt to pre-initialize variables to some value if that is the case. In all my code I usually want to get as few warnings as possible (if they are right or not), so that when I compile I can see the warnings that point me to the real problems (which would otherwise be hidden in the mess of other warnings). Is that not what you would want?
Peter,

Re: comment 18, pre-initialization does not hurt, but local variables need to be initialized to the correct value for the "default case" where they are not being set. Your patch did not do that correctly, hence my comments.

I think in cases where the uninitialized code paths are success cases, it's OK to use pre-initialization of the local variable. For error cases, I'm more reluctant to pre-initialize to a specific error state.
Here are some additional unsolicited review comments.
1. Premature initialization of variables can have detrimental effects on 
the optimization of the code with some optimizers.  However, I would not 
worry about that outside of freebl.

2. I object to the addition of unnecessary parentheses to silence gcc's 
pedantic warnings that suggest adding parentheses.  I would give those 
changes r- if I was formally reviewing this patch.  I refuse to let gcc 
developers dictate coding style.  If those warnings can be suppressed by 
other means, feel free to submit a patch that does so.

3. I will do more review to the proposed change to ssl3con.c.
The ssl3con.c changes are ok.
Whiteboard: [build_warning]
Are there anyone following up this bug?
(In reply to Takanori MATSUURA from comment #22)
> Are there anyone following up this bug?

OS/2 has been removed from the tree
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.