NSS build problem with GCC 3.4.6 on OS/2

RESOLVED FIXED in 3.12.2

Status

NSS
Build
P2
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Peter Weilbacher, Assigned: Julien Pierre)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

9 years ago
As I discovered in bug 454956 comment 7 there is a weird build problem on OS/2 when using the newly created GCC 3.4.6. It complains (with an error!) about the ffs (re)definition in security/nss/cmd/lib/secutil.h (it is already defined as int ffs(int) in string.h). So we have to somehow exclude it, perhaps by changing
   #ifndef XP_UNIX
to
   #if !defined(XP_UNIX) && !(defined(XP_OS2) &&
       (__GNUC__ > 3 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 4)))
but I am not sure yet if the problem actually also occurs with GCC 3.3.5 or with GCC >= 4.

Comment 1

9 years ago
(In reply to comment #0)
> As I discovered in bug 454956 comment 7 there is a weird build problem on OS/2
> when using the newly created GCC 3.4.6. It complains (with an error!) about the
> ffs (re)definition in security/nss/cmd/lib/secutil.h (it is already defined as
> int ffs(int) in string.h). So we have to somehow exclude it, perhaps by
> changing
>    #ifndef XP_UNIX
> to
>    #if !defined(XP_UNIX) && !(defined(XP_OS2) &&
>        (__GNUC__ > 3 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 4)))
> but I am not sure yet if the problem actually also occurs with GCC 3.3.5 or
> with GCC >= 4.
    #if !defined(XP_UNIX) && !(defined(XP_OS2) &&
        (__GNUC__ > 3 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 4 || __GNUC__ >= 4)))
GCC-4.3.2 fails as well, GCC-3.3.5 not yet tested, but the file secutil.h is identical in the mercurial and cvs repositories, therefore I assume the ffs definition is probably needed for GCC-3.3.5

Comment 2

9 years ago
(In reply to comment #1)
Hm, should take myself more time to read more carefully, you already included __GNUC__ > 3, but maybe we have to raise it to >= 4, if we need it for gcc-3.3.5?

>     #if !defined(XP_UNIX) && !(defined(XP_OS2) &&
>         (__GNUC__ >=4 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 4)))

Comment 3

9 years ago
I googled around and ffs is a built-in function of gcc. I therefore tried with
#if !defined(XP_UNIX) && !defined(XP_OS2)
to build with gcc-3.3.5, posting from this build. Probably it's not needed for any GCC build.
Or maybe we do, as we have done in the past, and declare that certain 
versions of GCC are broken and we do not support them.
(Reporter)

Comment 5

9 years ago
Well, if you want to de-support GCC 3.3.x and earlier I guess that would be OK, as at least for XPCOM it's the same. Or did you meant to say that 3.4.x and later are broken?
(Reporter)

Comment 6

9 years ago
Created attachment 342813 [details] [diff] [review]
simple fix

CVS blame doesn't go back far enough to see why XP_UNIX was originally excluded here, but this seems to be the most simple possible fix. 

This is supposed to go into the version of NSS that is likely to be taken next on mozilla-central (not sure what version to set for this).
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #342813 - Flags: review?(nelson)
(Assignee)

Comment 7

9 years ago
Comment on attachment 342813 [details] [diff] [review]
simple fix

Who is calling this ffs function ? I didn't see any callers. If there are none, let's just delete the code.

And if there are some, shouldn't the ifdef be the same in ffs.c and secutil.h ?
Attachment #342813 - Flags: superreview-
(Assignee)

Comment 8

9 years ago
Created attachment 342977 [details] [diff] [review]
don't compile ffs on OS/2 since it's now part of the gcc headers and runtime library
Attachment #342813 - Attachment is obsolete: true
Attachment #342977 - Flags: review?(nelson)
Attachment #342813 - Flags: review?(nelson)
(Assignee)

Comment 9

9 years ago
I found that there is a single caller for ffs() which is the checkcert program, so the code had to stay . The only remaining special case really is Windows now, since its runtime library does not have a header or an implementation for ffs.
Comment on attachment 342977 [details] [diff] [review]
don't compile ffs on OS/2 since it's now part of the gcc headers and runtime library

I'm guessing this is a patch.
Attachment #342977 - Attachment is patch: true
Attachment #342977 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 342977 [details] [diff] [review]
don't compile ffs on OS/2 since it's now part of the gcc headers and runtime library

r=nelson
Attachment #342977 - Flags: review?(nelson) → review+
(Assignee)

Comment 12

9 years ago
Thanks for the review, Nelson. I checked this in to the trunk of NSS.

Checking in lib/ffs.c;
/cvsroot/mozilla/security/nss/cmd/lib/ffs.c,v  <--  ffs.c
new revision: 1.3; previous revision: 1.2
done
Checking in lib/secutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.30; previous revision: 1.29
done
Assignee: mozilla → julien.pierre.boogz
Priority: -- → P2
Target Milestone: --- → 3.12.2
(Assignee)

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Reporter)

Comment 13

9 years ago
Thanks for the help, Julien!
You need to log in before you can comment on or make changes to this bug.