Last Comment Bug 434187 - Fix the GCC compiler warnings in nss/lib
: Fix the GCC compiler warnings in nss/lib
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.12
: All All
: -- trivial (vote)
: 3.12.1
Assigned To: Wan-Teh Chang
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-05-17 09:10 PDT by Wan-Teh Chang
Modified: 2008-08-25 15:11 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (16.75 KB, patch)
2008-05-17 09:10 PDT, Wan-Teh Chang
julien.pierre: review-
nelson: review-
Details | Diff | Review
Fix an assignment to a PRBool variable and a MSVC warning (checked in on trunk) (2.53 KB, patch)
2008-05-30 22:12 PDT, Wan-Teh Chang
nelson: review+
Details | Diff | Review
Proposed patch v2 (13.40 KB, patch)
2008-05-31 14:53 PDT, Wan-Teh Chang
julien.pierre: review+
Details | Diff | Review
Fix nssCertificateStore_Check (7.06 KB, patch)
2008-05-31 15:02 PDT, Wan-Teh Chang
julien.pierre: review+
Details | Diff | Review
Fix lib/softoken/pkcs11u.c (1.66 KB, patch)
2008-05-31 15:06 PDT, Wan-Teh Chang
nelson: review+
Details | Diff | Review
Fix the comments in sftk_DeleteObject (1.75 KB, patch)
2008-06-05 18:39 PDT, Wan-Teh Chang
rrelyea: review+
Details | Diff | Review

Description Wan-Teh Chang 2008-05-17 09:10:22 PDT
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.
Comment 1 Julien Pierre 2008-05-20 15:05:31 PDT
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 2008-05-20 15:23:20 PDT
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 2008-05-20 15:28:10 PDT
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) 2008-05-20 16:37:22 PDT
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 2008-05-30 22:12:45 PDT
Created attachment 323194 [details] [diff] [review]
Fix an assignment to a PRBool variable and a MSVC warning (checked in on trunk)
Comment 6 Wan-Teh Chang 2008-05-31 14:53:20 PDT
Created attachment 323243 [details] [diff] [review]
Proposed patch v2

I broke the patch into three patches.  This patch contains
the straightforward changes.
Comment 7 Wan-Teh Chang 2008-05-31 15:02:28 PDT
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.
Comment 8 Wan-Teh Chang 2008-05-31 15:06:08 PDT
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.
Comment 9 Nelson Bolyard (seldom reads bugmail) 2008-05-31 17:35:54 PDT
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) 2008-05-31 17:59:19 PDT
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 2008-05-31 22:29:28 PDT
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) 2008-06-03 13:00:08 PDT
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 2008-06-03 18:29:32 PDT
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 2008-06-05 18:17:00 PDT
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 2008-06-05 18:20:13 PDT
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 2008-06-05 18:39:44 PDT
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.
Comment 17 Wan-Teh Chang 2008-08-25 15:10:51 PDT
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 2008-08-25 15:11:51 PDT
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.)

Note You need to log in before you can comment on or make changes to this bug.