Fix the GCC compiler warnings in nss/lib

RESOLVED FIXED in 3.12.1

Status

NSS
Libraries
--
trivial
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 321404 [details] [diff] [review]
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.
Attachment #321404 - Flags: review?(julien.pierre.boogz)

Comment 1

9 years ago
Wan-Teh,

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

Comment 2

9 years ago
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

9 years ago
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.
Attachment #321404 - Flags: review?(julien.pierre.boogz) → review-
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.
Attachment #321404 - Flags: review-
(Assignee)

Comment 5

9 years ago
Created attachment 323194 [details] [diff] [review]
Fix an assignment to a PRBool variable and a MSVC warning (checked in on trunk)
Attachment #323194 - Flags: review?(nelson)
(Assignee)

Comment 6

9 years ago
Created attachment 323243 [details] [diff] [review]
Proposed patch v2

I broke the patch into three patches.  This patch contains
the straightforward changes.
Attachment #321404 - Attachment is obsolete: true
Attachment #323243 - Flags: review?(julien.pierre.boogz)
(Assignee)

Comment 7

9 years ago
Created attachment 323245 [details] [diff] [review]
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.
Attachment #323245 - Flags: review?(julien.pierre.boogz)
(Assignee)

Comment 8

9 years ago
Created attachment 323247 [details] [diff] [review]
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.
Attachment #323247 - Flags: review?(nelson)
Comment on attachment 323194 [details] [diff] [review]
Fix an assignment to a PRBool variable and a MSVC warning (checked in on trunk)

r=nelson
Attachment #323194 - Flags: review?(nelson) → review+
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 :)
Attachment #323247 - Flags: review?(nelson) → review+
(Assignee)

Comment 11

9 years ago
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);
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 */
}
(Assignee)

Comment 13

9 years ago
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
Attachment #323194 - Attachment description: Fix an assignment to a PRBool variable and a MSVC warning → Fix an assignment to a PRBool variable and a MSVC warning (checked in on trunk)

Updated

9 years ago
Attachment #323245 - Flags: review?(julien.pierre.boogz) → review+

Updated

9 years ago
Attachment #323243 - Flags: review?(julien.pierre.boogz) → review+
(Assignee)

Comment 14

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

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

Comment 15

9 years ago
Comment on attachment 323245 [details] [diff] [review]
Fix nssCertificateStore_Check

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

Comment 16

9 years ago
Created attachment 323972 [details] [diff] [review]
Fix the comments in sftk_DeleteObject

Bob, I'd also appreciate your answers to our questions in comment 11
and comment 12.
Attachment #323247 - Attachment is obsolete: true
Attachment #323972 - Flags: review?(rrelyea)

Updated

9 years ago
Attachment #323972 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 17

9 years ago
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
(Assignee)

Comment 18

9 years ago
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.)
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.