434187 - Fix the GCC compiler warnings in nss/lib

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Wan-Teh Chang]

Attachment: Proposed patch

The attached patch fixes most of the compiler warnings
in nss/lib reported by GCC on Mac OS X.

Most of these warnings are unused variables.  None of
these is a real bug.

I didn't fix the following warnings:

1. All the "xxx may be used uninitialized" warnings
(reported only in optimized builds).

2. const issues.  I don't want to just cast away the
const, but don't have time to investigate these.

3. char vs. unsigned char issues.  Same as above, I
don't want to just add a cast but don't have time to
investigate these.

Comment 1

[Julien Pierre]

Wan-Teh,

I think your patch is fine except for the change to nssCertificateStore_Check . What warning did gcc give about that ?

Comment 2

[Wan-Teh Chang]

The warning is that nssCertificateStore_Check is defined but not
used in the .c files that include pkistore.h but don't use
nssCertificateStore_Check.  nssCertificateStore_Check is defined
as a static function in pkistore.h.

Comment 3

[Julien Pierre]

Comment on attachment 321404 [details] [diff] [review]
Proposed patch

I see. I did this intentionally to allow the optimizer to remove the no-op function in the optimized build. With your patch, the code becomes a real function and calls are added unnecessarily.

This should be fixed by making nssCertificate_StoreCheck a macro that is a no-op in the optimized build, and calls the extern function in the debug build like in your patch.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 321404 [details] [diff] [review]
Proposed patch


> CK_RV
> sftk_DeleteObject(SFTKSession *session, SFTKObject *object)
> {
>     SFTKSlot *slot = sftk_SlotFromSession(session);
>     SFTKSessionObject *so = sftk_narrowToSessionObject(object);
>-    SFTKTokenObject *to = sftk_narrowToTokenObject(object);

Did you do a DEBUG build with this patch?

"to" is referenced in an assertion later in this function. 
If you remove the definition of "to", I think debug builds will 
stop building.

Comment 5

[Wan-Teh Chang]

Attachment: Fix an assignment to a PRBool variable and a MSVC warning (checked in on trunk)

Comment 6

[Wan-Teh Chang]

Attachment: Proposed patch v2

I broke the patch into three patches.  This patch contains
the straightforward changes.

Comment 7

[Wan-Teh Chang]

Attachment: Fix nssCertificateStore_Check

Looking at nssCertificateStore_Check more closely, I found
that it and nssCertificateStore_Unlock are always used together.
(There is a standalone nssCertificateStore_Check call in
lib/pki/certificate.c, but that call is redundant with the
two nssCertificateStore_Check calls above it.)

So this patch merges the six PORT_Asserts of nssCertificateStore_Check
into nssCertificateStore_Unlock and removes the redundant ones.
I think this is a better solution than defining a macro that
expands to nssCertificateStore_Check in debug build and
to nothing in optimized build.

Comment 8

[Wan-Teh Chang]

Attachment: Fix lib/softoken/pkcs11u.c

Nelson, you're right, I didn't test the old patch in
a debug build again.  I looked for other uses of |to|
and #ifdef in the function, but missed the PORT_Assert(to)
statement.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 323194 [details] [diff] [review]
Fix an assignment to a PRBool variable and a MSVC warning (checked in on trunk)

r=nelson

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 323247 [details] [diff] [review]
Fix lib/softoken/pkcs11u.c

After looking at the implementation of these two functions:

>     SFTKSessionObject *so = sftk_narrowToSessionObject(object);
>-    SFTKTokenObject *to = sftk_narrowToTokenObject(object);

I conclude that, given that "object" is non-NULL, one of the two 
values (so, to) is guaranteed to be equal to object and the other
is guaranteed to be NULL.  Therefore, this assertion:

>-	PORT_Assert(to);
>+	PORT_Assert(sftk_narrowToTokenObject(object));

is completely superfluous.  It cannot fail, unless object is NULL.  
One COULD restructure the code slightly, so that we check "object"
for being non-NULL before passing it to either "narrow" function.
That check could be an assertion.  But I think it's unnecessary.

So, I suggest you just delete both lines, rather than changing the 
assert from one expression that cannot fail to another.  

r+ for the patch that does that.  (I don't need to see it :)

Comment 11

[Wan-Teh Chang]

Comment on attachment 323247 [details] [diff] [review]
Fix lib/softoken/pkcs11u.c

(In reply to comment #10)

Nelson, thanks for the careful code review.  You're right
about the relation between |so| and |to|.  PORT_Assert(to)
would be superfluous if the "if" test were
    if (so) {
But the "if" test is
    if (so && so->session) {

Perhaps we should do this instead?
    if (so) {
 	SFTKSession *session = so->session;
+       PORT_Assert(session);

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 11, I think we need to ask Bob whether the condition 
where so is non-NULL but so->session is NULL/zero is an allowable and 
occasionally expected occurance inside sftk_DeleteObject, or whether it 
is an NSS internal software error, and therefore reasonable to assert 
that it never occurs.  

If it is an allowable condition, then perhaps the code in sftk_DeleteObject should be structured as:

if (so && so->session) {
 ...
} else if (to) {
 ...
} else { 
 /* deal with error */
}

Comment 13

[Wan-Teh Chang]

Comment on attachment 323194 [details] [diff] [review]
Fix an assignment to a PRBool variable and a MSVC warning (checked in on trunk)

I checked in this patch on the NSS trunk (NSS 3.12.1).

Checking in lgattr.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/lgattr.c,v  <--  lgattr.c
new revision: 1.6; previous revision: 1.5
done
Checking in derive.c;
/cvsroot/mozilla/security/nss/lib/ssl/derive.c,v  <--  derive.c
new revision: 1.11; previous revision: 1.10
done

Comment 14

[Wan-Teh Chang]

Comment on attachment 323243 [details] [diff] [review]
Proposed patch v2

I checked in this patch on the NSS trunk (NSS 3.12.1).

Comment 15

[Wan-Teh Chang]

Comment on attachment 323245 [details] [diff] [review]
Fix nssCertificateStore_Check

I checked in this patch on the NSS trunk (NSS 3.12.1).

Comment 16

[Wan-Teh Chang]

Attachment: Fix the comments in sftk_DeleteObject

Bob, I'd also appreciate your answers to our questions in comment 11
and comment 12.

Comment 17

[Wan-Teh Chang]

Comment on attachment 323972 [details] [diff] [review]
Fix the comments in sftk_DeleteObject

I checked in this comment patch on the NSS trunk (NSS 3.12.2).

Checking in pkcs11u.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11u.c,v  <--  pkcs11u.c
new revision: 1.77; previous revision: 1.76
done

Comment 18

[Wan-Teh Chang]

Marked the bug fixed in NSS 3.12.1.  (All the compiler
warning fixes are in NSS 3.12.1.  NSS 3.12.2 only has
a comment patch.)