Closed Bug 856404 Opened 8 years ago Closed 5 years ago

Enable libraries folding on mingw

Categories

(Firefox Build System :: General, defect, P2)

x86
Windows 7
defect

Tracking

(firefox42 fixed)

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: jacek, Assigned: jacek)

References

Details

Attachments

(4 files)

Bug 855675 disabled libraries folding on mingw targets as a short term fix to get mingw builds working in the tree. Long term, however, we want to enable filding on mingw. This will most likely require binutils changes as well as some fixes to m-c build system.
I worked on this and it seems very tricky to support directly in binutils. However, I found a workaround for that by using -mnop-fun-dllimport compiler flag. This is not perfect (see bug 1151447), but at least the known problem won't exist once NSS and NSPR live in the same DLL.
This is build system part. Sadly, it's not enough because it reveals some problems in NSPR.
Assignee: nobody → jacek
Attachment #8592232 - Flags: review?(mh+mozilla)
This is similar to bug 1060401. This time it's about static library files that need lib prefix on mingw.
Attachment #8592241 - Flags: review?(ted)
The problem that we need to solve is: we pass -mnop-fun-dllimport, which is valid only on mingw target, but it gets populated to HOST_CFLAGS, where it's invalid. I could work around it in top level configure (eg. by passing HOST_FLAGS=" ", which prevents defaulting HOST_CFLAGS). Gonk already has hackish solution inside NSPR configure, which does just that. IMO we should simply avoid assigning CFLAGS to HOST_CFLAGS for all platforms when cross compiling. It's not expected that setting target flags may break host parts.
Attachment #8592245 - Flags: review?(ted)
Comment on attachment 8592241 [details] [diff] [review]
NSPR: Use correct library prefix on mingw

This part is broken on MSVC for some reason. Here is green try push without that part:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7dfce2d77c2
Attachment #8592241 - Flags: review?(ted)
Comment on attachment 8592245 [details] [diff] [review]
NSPR: configure part

Review of attachment 8592245 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, that seems reasonable. I don't know why anyone would want their host CFLAGS to default to their target CFLAGS anyway.
Attachment #8592245 - Flags: review?(ted) → review+
Comment on attachment 8592232 [details] [diff] [review]
build system part

Review of attachment 8592232 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/external/nss/Makefile.in
@@ +263,5 @@
>  # it, creating race conditions. See bug #836220
>  DEFAULT_GMAKE_FLAGS += TARGETS='$$(LIBRARY) $$(SHARED_LIBRARY) $$(PROGRAM)'
>  
> +ifdef MOZ_FOLD_LIBS_FLAGS
> +DEFAULT_GMAKE_FLAGS += OS_CFLAGS='$(MOZ_FOLD_LIBS_FLAGS)'

You should use XCFLAGS instead.
Attachment #8592232 - Flags: review?(mh+mozilla) → review+
Thanks for reviews. I can't reproduce try failure locally with MSVC, which is strange. It seems like MSC_VER is not defined on try for some reason. I will keep investigating. Meantime, the other NSPR part is ready to be committed. m-c part depends on both MSVC parts, so it will have to wait.
Attachment #8599412 - Flags: checkin?
Keywords: checkin-needed
Whiteboard: [NSPR checkin][keep open]
Whiteboard: [NSPR checkin][keep open] → [NSPR checkin][leave-open]
Comment on attachment 8592241 [details] [diff] [review]
NSPR: Use correct library prefix on mingw

I found the reason why it didn't work on try. MSVC version detection is broken because NSPR can't handle properly the way sccache is used. I will file a separate bug for that, but this patch itself is fine.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0884b5bf3893
Attachment #8592241 - Flags: review?(ted)
Depends on: 1160125
Attachment #8599412 - Flags: checkin? → checkin+
Ryan, thanks for the commit, but shouldn't this go to NSPR repo first?
Attachment #8592241 - Flags: review?(ted) → review+
Attachment #8592241 - Flags: checkin?
Thanks for the review. This may be landed after patch from bug 1160125. I could land it on m-i like the previous patch was landed myself, but I don't think that's the right thing to do.
Keywords: checkin-needed
Flags: needinfo?(ryanvm)
I don't have the ability to land in the NSPR repo.
Flags: needinfo?(ryanvm)
wtc, can you please help get this NSPR patch landed?
Flags: needinfo?(wtc)
Comment on attachment 8592241 [details] [diff] [review]
NSPR: Use correct library prefix on mingw

Review of attachment 8592241 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. This seems like a continuation of the changeset
https://hg.mozilla.org/projects/nspr/rev/133b835b9ad9

in NSPR 4.10.8. Note that both of these changes break
the OS/2 build. I guess we have dropped OS/2 support?

Patch checked in: https://hg.mozilla.org/projects/nspr/rev/cfda142e3c2b
Attachment #8592241 - Flags: review+
Attachment #8592241 - Flags: checkin?
Attachment #8592241 - Flags: checkin+
Status: NEW → ASSIGNED
Flags: needinfo?(wtc)
Priority: -- → P2
Whiteboard: [NSPR checkin][leave-open] → [leave-open]
Thank you for checkins. Could you please also commit attachment 8599412 [details] [diff] [review]? It has been already committed to m-c, but not NSPR.


BTW, I don't know much about OS/2, but I think it should be fine with the patch.
Attachment #8599412 - Flags: checkin+ → checkin?(wtc)
Comment on attachment 8599412 [details] [diff] [review]
NSPR: configure part (for commit)

Patch checked in: https://hg.mozilla.org/projects/nspr/rev/337fef162531
Attachment #8599412 - Flags: checkin?(wtc) → checkin+
Comment on attachment 8592241 [details] [diff] [review]
NSPR: Use correct library prefix on mingw

Review of attachment 8592241 [details] [diff] [review]:
-----------------------------------------------------------------

::: nsprpub/config/rules.mk
@@ -80,5 @@
>  # Win95 and OS/2 require library names conforming to the 8.3 rule.
>  # other platforms do not.
>  #
>  ifeq (,$(filter-out WIN95 WINCE WINMO OS2,$(OS_TARGET)))
> -LIBRARY		= $(OBJDIR)/$(LIBRARY_NAME)$(LIBRARY_VERSION)_s.$(LIB_SUFFIX)

Jacek: note the "OS2" on line 83. This is why I think this patch
affects OS/2.
NSPR parts were merged to m-c, so I landed the last part.
Whiteboard: [leave-open]
https://hg.mozilla.org/mozilla-central/rev/cde7888b5ce1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Depends on: 1248552
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.