Closed Bug 405297 Opened 12 years ago Closed 11 years ago

Problems building nss/lib/ckfw/capi/ with MingW GCC

Categories

(NSS :: Libraries, defect, P3)

3.11.1
x86
Windows XP
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.12.4

People

(Reporter: jfwfreo, Assigned: wtc)

References

Details

Attachments

(2 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.9) Gecko/20071030 SeaMonkey/1.1.6
Build Identifier: 

2 problems that need to be fixed in NSS Code in order for nss/lib/ckfw/capi/ to build with MingW GCC:
1.makefile needs to link with -lcrypt32,-ladvapi32 and -lrpcrt4 on MingW GCC
and 2.this line:
if (len + (src-start) > (unsigned int)size) {
in cobject.c needs to be fixed since MingW GCC chokes on using the - operator to subtract one pointer from another.


Reproducible: Always
Attached patch propsed patch (obsolete) — Splinter Review
First part of the patch (linking to the libraries) should be fine. Not 100% sure of the correct way to get MingW GCC to do the pointer subtraction though.
Attachment #290095 - Flags: superreview?(wtc)
Attachment #290095 - Flags: review?(nelson)
Comment on attachment 290095 [details] [diff] [review]
propsed patch

r- for several reasons, including c++ style comment in c code.

I really doubt that any version of gcc disallows all pointer subtraction.  What is the error code produced?

There are numerous places in NSS where pointer subtraction is done.  Does the compiler fail on all of them?  
If not, then what is special about this particular line of code?  Maybe the pointers are of different types?

We're not going to change every place that does pointer subtraction in NSS source code with an abhorrent hack to
work around a single broken version of a single compiler 
that can't handle basic K&R c code.  But if there's some 
issue that makes this one pointer subtraction different from the others, then when we know what it is, we can consider the matter at that time.
Attachment #290095 - Flags: review?(nelson) → review-
Casting a pointer to unsigned int will truncate the pointer's value
on 64-bit Windows.

I suspect that the compiler doesn't allow subtraction of pointers
to different types.
Attached patch alternative patch (obsolete) — Splinter Review
I gather that the source code in this directory never was code reviewed.
I found lots of mixture of pointers to signed and unsigned objects.
I believe this patch fixes the problems I found and should compile 
correctly with either gcc or MSVC.  

Reporter, please test this patch.  (I left out the Makefile part of the patch).

Bob, please review this patch.

Note that given the declaration
  unsigned char * src;
the expression
    (unsigned) *src++ 
doesn't do what (I think) it was expected to do.  
I believe the desire was to ensure that the char fetched from *src 
was to be treated as an unsigned char, so that sign extension would 
not occur when converting it to a wider integer size.  
But the expression above doesn't do that.
I believe that the expression
     *(unsigned char *)src++ 
does what you wanted.
I also eliminated a warning in a related code file.
Attachment #290937 - Flags: review?(rrelyea)
Comment on attachment 290937 [details] [diff] [review]
alternative patch

Why not just declare "start" and "end" as "char *" ?

    char *start = src;
    char *end = src+size;

Then you don't need any of the (char *) casts in this patch.

I think

    (unsigned char) *src++

is better than

    *(unsigned char *)src++;
Comment on attachment 290937 [details] [diff] [review]
alternative patch

If you declare "len" as "int", then you don't need this cast:

>-  *outSize = len;
>+  *outSize = (int)len;
Attached patch propsed patch v2 (obsolete) — Splinter Review
Please review and test this patch with MinGW.

This patch fixes the problem by changing the types of
the variables.  It also deletes the ununsed variable "end".

See the patches in bug 298612 for the preferred way
to cast a "char" representing an unsigned 8-bit integer
to an "int".  It uses (unsigned char) cast.
Attachment #290095 - Attachment is obsolete: true
Attachment #291031 - Flags: superreview?(rrelyea)
Attachment #291031 - Flags: review?(jfwfreo)
Attachment #290095 - Flags: superreview?(wtc)
I looked at the code more carefully and found that
we really should use "unsigned char" for the "start"
and "next" parameters and the return value of this
function.  But I'd need to review and update the callers
of this function, so I'll let Bob do the right thing.

This patch is the minimum change required to fix the
MinGW build errors.  Please test it.
Attachment #291031 - Attachment is obsolete: true
Attachment #291145 - Flags: review?(jfwfreo)
Attachment #291031 - Flags: superreview?(rrelyea)
Attachment #291031 - Flags: review?(jfwfreo)
Comment on attachment 290937 [details] [diff] [review]
alternative patch

r+ rrelyea
Attachment #290937 - Flags: review?(rrelyea) → review+
Comment on attachment 290937 [details] [diff] [review]
alternative patch

Wan-Teh's comment 8 describes the right fix for this bug, which none of the patches above does.
So, I'm obsoleting my previous patch.
Attachment #290937 - Attachment is obsolete: true
Comment on attachment 291145 [details] [diff] [review]
propsed patch v2.1

Please see comment 8 for a description of this patch.
Attachment #291145 - Flags: review?(rrelyea)
Comment on attachment 291145 [details] [diff] [review]
propsed patch v2.1

r+ This is the cleanest patch.

bob
Attachment #291145 - Flags: review?(rrelyea) → review+
FYI, I ran into some additional problems when trying to build today's firefox from CVS trunk using a mingw cross-compiler.  CRYPT_KEY_PROV_INFO isn't being defined in the mingw-runtime 3.14 or w32api 3.11 headers.  After adding the appropriate structs & defines from a patch based upon wine, I ran into the problem of CRYPT_E_NOT_FOUND not being defined.  After that, I gave up and disabled capi for win32 gcc builds. 

I can't tell if the code has changed since the original bug report (which obviously doesn't have those problems) or if my build setup is still out of date.
Blocks: 421095
I would send a bug back to mingw.

One of my problems with these psuedo-unix compile environments is the fact they often have difficulty accessing native libraries and functionality.

It's not an issue in FF 3, but at some point I suspect we'll have  to address it.

bob
Duplicate of this bug: 439614
This is the patch from bug 427479, r=nelson
Attachment #332563 - Flags: review+
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 332563 [details] [diff] [review]
Change include case (checked in on trunk)

I checked this in.  The commit comment was:

Bug 427479: Include file case changes for mingw cross compilation on a linux host
patch by Mook <mook.moz+mozbz@gmail.com>, r=nelson
Attachment #332563 - Attachment description: Change include case → Change include case (checked in on trunk)
Attachment #332563 - Flags: review+
(In reply to comment #12)
> (From update of attachment 291145 [details] [diff] [review])
> r+ This is the cleanest patch.

I was able to setup build environment and verified that this patch fixes the problem. Can it be checked in?
Attachment #291145 - Flags: review?(jfwfreo)
Comment on attachment 291145 [details] [diff] [review]
propsed patch v2.1

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

Checking in Makefile;
/cvsroot/mozilla/security/nss/lib/ckfw/capi/Makefile,v  <--  Makefile
new revision: 1.6; previous revision: 1.5
done
Checking in cobject.c;
/cvsroot/mozilla/security/nss/lib/ckfw/capi/cobject.c,v  <--  cobject.c
new revision: 1.6; previous revision: 1.5
done
Assignee: nobody → wtc
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.12.4
Priority: -- → P3
Version: unspecified → 3.11.1
Duplicate of this bug: 406113
You need to log in before you can comment on or make changes to this bug.