570340 - freebl fails to compile on mingw-w64

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Jacek Caban]

Attachment: fix v1.0

It tries to use MSC assembler files for mingw-w64, which obviously fails. GCC assembly is also incompatible, so I've changed Makefile to not use fast implementation. The patch is attached.

Comment 1

[Jacek Caban]

wtc, could you please review the patch?

Comment 2

[Jacek Caban]

Attachment: fix v2.0

The attached version ports MASM assembly to be compatible with mingw. It's done the same way as a few other files, like bug 633924. It's kept as similar to MASM as possible so that it will be easy to maintain in the future. The nice trick for reviewing assembly files it to see only meaningful changes, without mechanic s/;/#/ (for comments):

sed 's/;/#/g' masm_file >/tmp/masm
diff -u /tmp/masm mingw_asm_file

Comment 3

[Jacek Caban]

wtc, ping. (See my previous comment for the sed trick, the patch is big, but there is not that much to review)

Comment 4

[Jacek Caban]

Comment on attachment 541664 [details] [diff] [review]
fix v2.0

Brian, can you please review it? This blocks 64-bit mingw builds for quite long already...

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I appreciate the effort that went into this patch. However, I do not think that we should maintain new assembler source files just for MinGW. Isn't there any way to get gas to read the MASM syntax, so we could standardize on the MASM source files? Or, even better, can't we get MinGW GAS to output PE-COFF .obj files that could then be linked with obj files compiled with MSVC so we could standardize on the GAS syntax, removing the MASM syntax?

If we can't do either of those, then let's let MinGW builds use the C implementations.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 541664 [details] [diff] [review]
fix v2.0

r- as described above.

Comment 7

[Jacek Caban]

(In reply to Brian Smith (:bsmith) from comment #5)
> I appreciate the effort that went into this patch. However, I do not think
> that we should maintain new assembler source files just for MinGW. Isn't
> there any way to get gas to read the MASM syntax, so we could standardize on
> the MASM source files? Or, even better, can't we get MinGW GAS to output
> PE-COFF .obj files that could then be linked with obj files compiled with
> MSVC so we could standardize on the GAS syntax, removing the MASM syntax?

As far as I know, it's possible to share assembly between GCC and MSVC only using yasm. It's already used in Mozilla sources for libvpx, but I guess that's not the kind of dependency NSS should have.

> If we can't do either of those, then let's let MinGW builds use the C
> implementations.

OK, at least for now that sounds like the way we should go. That's exactly what my v1 of the patch does.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Bob and Wan-Teh, do you agree with comment 5 above--that it is not worth the maintenance burden of maintaining separate source files for MinGW-Win64 support?

Comment 9

[Wan-Teh Chang]

bsmith: I agree we should not go out of our way to support the
MinGW build configuration.  I support the simple approach taken by
the v1 patch: just use the C code for MinGW-Win64.  That's what
we do for MinGW 32-bit Windows:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/freebl/Makefile&rev=1.119&mark=154-163#152

The glibc developer Ulrich Drepper described the high cost of
supporting many configurations in this blog post:
http://udrepper.livejournal.com/7326.html

Comment 10

[Jacek Caban]

Brian, ping, can we proceed further with Wan-Teh agreeing with the approach?

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Jacek, which msys/mingw-w64 package(s) do I need in order to test this?

Comment 12

[Jacek Caban]

I've updated bug 570342 with how I currently do the builds. That describes cross compilation, which I do on daily basis, so I'm confident it works. I haven't tried native compilation for a while, but if you'd prefer that, I will make sure it works and write instructions.

BTW, I realize that setting up an environment is quite problematic for a review. Most people who review my patches don't do that. If I can make it easier somehow or you would like to get my builds, please let me know.

Comment 13

[Wan-Teh Chang]

Attachment: fix v1.1, by Jacek Caban

I edited Jacek's fix v1.0 (attachment 449468 [details] [diff] [review]) to look like
the 32-bit mingw code above and checked it on on the NSS
trunk (NSS 3.13.5).

Checking in Makefile;
/cvsroot/mozilla/security/nss/lib/freebl/Makefile,v  <--  Makefile
new revision: 1.120; previous revision: 1.119
done

Comment 14

[Jacek Caban]

Thank you!