Enable libraries folding on mingw

RESOLVED FIXED in Firefox 42

Status

defect
P2
normal
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: jacek, Assigned: jacek)

Tracking

unspecified
mozilla42
x86
Windows 7
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(4 attachments)

Assignee

Description

6 years ago
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.
Assignee

Comment 1

4 years ago
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.
Assignee

Comment 2

4 years ago
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)
Assignee

Comment 3

4 years ago
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)
Assignee

Comment 4

4 years ago
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)
Assignee

Comment 6

4 years ago
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+
Assignee

Comment 9

4 years ago
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?
Assignee

Updated

4 years ago
Keywords: checkin-needed
Whiteboard: [NSPR checkin][keep open]

Updated

4 years ago
Whiteboard: [NSPR checkin][keep open] → [NSPR checkin][leave-open]
Assignee

Comment 10

4 years ago
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)
Assignee

Updated

4 years ago
Depends on: 1160125
Attachment #8599412 - Flags: checkin? → checkin+
Assignee

Comment 12

4 years ago
Ryan, thanks for the commit, but shouldn't this go to NSPR repo first?
Attachment #8592241 - Flags: review?(ted) → review+
Assignee

Updated

4 years ago
Attachment #8592241 - Flags: checkin?
Assignee

Comment 14

4 years ago
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
Assignee

Updated

4 years ago
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 17

4 years ago
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+

Updated

4 years ago
Status: NEW → ASSIGNED
Flags: needinfo?(wtc)
Priority: -- → P2
Whiteboard: [NSPR checkin][leave-open] → [leave-open]
Assignee

Comment 18

4 years ago
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.
Assignee

Updated

4 years ago
Attachment #8599412 - Flags: checkin+ → checkin?(wtc)

Comment 19

4 years ago
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 20

4 years ago
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.
Assignee

Comment 22

4 years ago
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: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42

Updated

3 years ago
Depends on: 1248552

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.