Closed Bug 653662 Opened 13 years ago Closed 13 years ago

Disable incremental linking

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
blocker

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla6

People

(Reporter: cjones, Assigned: standard8)

References

Details

Attachments

(2 files)

Disable incremental linking on windoze because we're running into a stupid hard-coded size limit
1.91 KB, patch
Details | Diff | Splinter Review
Disable incremental linking only on libxul
699 bytes, patch
ted
: review+
Details | Diff | Splinter Review 
When building http://hg.mozilla.org/users/cjones_mozilla.com/mcmq/file/510547d841e2/648484-noodle against m-c 88d3c5bde0ba, I get this error

LINK : xul.dll not found or not built by the last incremental link; performing full link
   Creating library xul.lib and object xul.exp
LINK : fatal error LNK1210: exceeded internal ILK size limit; link with /INCREMENTAL:NO
c:\Users\cjones\mozilla\mozilla-central\config\rules.mk:1153:0: command 'c:/mozilla-build/python/python2.6.exe ../../../mozilla-central/config/pythonpath.py -I../../config ../../../mozilla-central/config/expandlibs_exec.py --uselist -- link -NOLOGO -DLL -OUT:xul.dll -PDB:xul.pdb -SUBSYSTEM:WINDOWS  dlldeps-xul.obj nsStaticXULComponents.obj nsDllMain.obj dlldeps.obj nsGFXDeps.obj dlldeps-zlib.obj nsUnicharUtils.obj nsBidiUtils.obj nsRDFResource.obj    ./module.res -LARGEADDRESSAWARE -NXCOMPAT -DYNAMICBASE -SAFESEH  -DEBUG -DEBUGTYPE:CV [snip]

There's a lot of junk in my mozconfig, but the relevant non-default bits are

ac_add_options --disable-optimize
ac_add_options --enable-debug
ac_add_options --enable-trace-malloc
ac_add_options --enable-tests
ac_add_options --enable-ipdl-tests
ac_add_options --enable-chrome-format=symlink
ac_add_options --disable-xpcom-fastload
ac_add_options --disable-xpconnect-idispatch
ac_add_options --disable-activex
ac_add_options --disable-activex-scripting
ac_add_options --disable-accessibility

MSDN sez http://social.msdn.microsoft.com/Forums/en-US/vcgeneral/thread/c34d5c37-ca4a-4580-9c7c-4379a8c76d1f/ ; in a nutshell, there's a hard-coded limit of 384MB for these .ilk files, and I'm apparently exceeding it.

This patch compiled and linked last week, and I was able to compile and link other patches today on my windows machine.  Some combination of the above patch and code that's landed in the last week has broken the camel's back.

(I've never gotten an incremental link myself; I think I hit bug 644698.)
I hope someone can think of a way around this bug.  I'm glad to help provide debugging info.  Otherwise, I think we need this patch :/.
Assignee: nobody → jones.chris.g
Attachment #529043 - Flags: review?(ted.mielczarek)
Attachment #529043 - Flags: review?(khuey)
Can't we somehow just do this on just the libxul itself?

We have other dlls and binaries that won't be hitting that limit so it seems a shame to lose the capability there (e.g. I think Neil and maybe some devs would like to link mailnews with external API with incremental linking).
Hmmm, are you hitting this because of --enable-ipdl-tests?  I seem to remember that causing a bunch of code to be added.

Thunderbird/Seamonkey have been linking in all of mailnews with no problems ...
Severity: normal → critical
Also, the .ilk for my debug libxul is only 154 MB.
Have you been rebuilding in this objdir? Does the .ilk file just grow over time?
(In reply to comment #3)
> Hmmm, are you hitting this because of --enable-ipdl-tests?  I seem to remember
> that causing a bunch of code to be added.

--enable-ipdl-tests adds some code.  In exchange, I have a few other things turned off.  That's not really a game we can force devs to play, though.

(In reply to comment #4)
> Also, the .ilk for my debug libxul is only 154 MB.

Odd.  Are you using VS2010 or 2008?

(In reply to comment #5)
> Have you been rebuilding in this objdir? Does the .ilk file just grow over
> time?

Yes, but, qpushing the patch in comment 0 => failed build, qpop => successful build.  So I don't think it's growing over time.
(In reply to comment #6)
> (In reply to comment #3)
> > Hmmm, are you hitting this because of --enable-ipdl-tests?  I seem to remember
> > that causing a bunch of code to be added.
> --enable-ipdl-tests adds some code.  In exchange, I have a few other things
> turned off.  That's not really a game we can force devs to play, though.

Sure, we don't want to play that game.  I was under the impression that it added *a lot* of code though.  If that's not true, then disregard that.

> (In reply to comment #4)
> > Also, the .ilk for my debug libxul is only 154 MB.
> Odd.  Are you using VS2010 or 2008?

2010.

I'm really opposed to turning this off unless we absolutely have to, and this is the first report of this problem we've seen ... so I'm hoping this is something weird with your setup.
$ find ipc/ipdl/ -wholename 'ipc/ipdl/P*Test*.obj' -o -wholename 'ipc/ipdl/test/cxx/*.obj' | xargs ls --block-size=1 -s | awk '{sum=sum+$1}END{print sum}'
78177280

(There's a bit more code turned on by --enable-ipdl-tests, but it's peanuts compared to the above.)

So, even if there were one byte in the .ilk per byte of .obj, --enable-ipdl-tests + comment 4 should be well under the 384MB limit.
Comment on attachment 529043 [details] [diff] [review]
Disable incremental linking on windoze because we're running into a stupid hard-coded size limit

Clearing the review requests here until this gets more investigation or more reports.
Attachment #529043 - Flags: review?(ted.mielczarek)
Attachment #529043 - Flags: review?(khuey)
Severity: critical → blocker
Raising to blocker, as this is now affecting both the Thunderbird & SeaMonkey trunk debug boxes. I suspect it is because we've got mailnews being linked into libxul, but we're going to need a fix to this before the next m-c -> m-a merge.

If there's anything useful I can run on our builders, then let me know (comment here or ping me on irc tomorrow).
Blocks: 655558
(In reply to comment #0)
> Some combination of the above
> patch and code that's landed in the last week has broken the camel's back.

Interestingly, (c-c) bug 655558 regression timeframe is a week later than this bug.


(In reply to comment #2)
> Can't we somehow just do this on just the libxul itself?

(And for Debug (+ Win32, not Win64) builds only, if that's not what the patch is already doing.)


(In reply to comment #8)
> So, even if there were one byte in the .ilk per byte of .obj,
> --enable-ipdl-tests + comment 4 should be well under the 384MB limit.

What is your current .ilk file size?
(Does a clobber help?)


(In reply to comment #10)
> Both TB and SM seem to be getting this on their trunk Windows Leak Test

Confirmed: bug 655558.
Version: unspecified → Trunk
I don't have an .ilk file (MSVC blows it away?).  Clobbering didn't help.
FTR, disabling trace-malloc in my local debug build didn't work around this bug.
(In reply to comment #14)
> FTR, disabling trace-malloc in my local debug build didn't work around this
> bug.

You can watch the .ilk file growing as the link happens. I've just done this on a 2005 system, and it seemed to stop at 256MB, which would agree with the article you referenced in comment 0.

I'm double-checking a clobber on our builders, but then I think we'll have to forcibly disable it somehow, maybe via the mozconfig.
(In reply to comment #11)
> Raising to blocker, as this is now affecting both the Thunderbird &
> SeaMonkey trunk debug boxes. I suspect it is because we've got mailnews
> being linked into libxul, but we're going to need a fix to this before the
> next m-c -> m-a merge.

We certainly need some solution for the next m-aurora.

The other concern here is that even as more code is added to Gecko we'll hit this even on the Firefox side, with no easy determinism of the developer who first hits this as to what is wrong.
The thread linked to in comment 0 says that 64-bit Windows has double the limit. Might it pay to switch to that instead of disabling incremental linking? Or at least only disabling incremental linking if we're on 32-bit Windows.
Oh, that's interesting! cjones: are you on a 32-bit Windows?
This disables incremental linking only for libxul and if you're on Windows. I've succeeded in doing a debug build compilation on our windows try builder with this.

I don't think it is ideal, but the only alternatives I can see are:

- Switch to a later Visual Studio version
-- This may help our builders (which seem to have hit this after devs), but not the devs who are seeing this in 2010.
-- We'd also need jemalloc, and I doubt getting that on a later VS version will happen before the next merge.
- Fix Bug 590996 and do this via mozconfigs.
-- I'd be prepared to accept this as an alternative, but landing that patch seems to be going no-where, and it doesn't provide an immediate answer for devs who are hitting the issue.
Attachment #532627 - Flags: review?(ted.mielczarek)
(In reply to comment #17)

> The thread linked to in comment 0 says that 64-bit Windows has double the
> limit. Might it pay to switch to that instead of disabling incremental

I don't think everyone can be expected to switch to 64bit just yet.

> linking? Or at least only disabling incremental linking if we're on 32-bit
> Windows.

Yeah, as I noted in comment 12.


(In reply to comment #19)

> This disables incremental linking only for libxul and if you're on Windows.

Should (not) we do this for Win32 only, ftb?

> - Switch to a later Visual Studio version
> -- This may help our builders (which seem to have hit this after devs), but
> not the devs who are seeing this in 2010.

I was going to say "do it for VS 2005 only", but if it can already affect VS 2010 users then fine.
(In reply to comment #20)
> (In reply to comment #19)
> 
> > This disables incremental linking only for libxul and if you're on Windows.
> 
> Should (not) we do this for Win32 only, ftb?

I saw that only after writing the first version of the patch, I'd prefer to get feedback from Ted and co if it'll be accepted before going any further.

@Ted: note that bugzilla says Chris is away until the 28th.
Comment on attachment 532627 [details] [diff] [review]
Disable incremental linking only on libxul

This sucks, but we'll go with it for now.
Attachment #532627 - Flags: review?(ted.mielczarek) → review+
(In reply to comment #17)
> The thread linked to in comment 0 says that 64-bit Windows has double the
> limit. Might it pay to switch to that instead of disabling incremental
> linking? Or at least only disabling incremental linking if we're on 32-bit
> Windows.

For what its worth I'll be attempting a build on VC8EE with Win64 as a test without this patch, in the next 2 weeks or so...

So we can have some data points.
(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > This disables incremental linking only for libxul and if you're on Windows.
> > Should (not) we do this for Win32 only, ftb?
> I saw that only after writing the first version of the patch
What's actually tipping the ilk over the limit? MOZ_DEBUG is a sledgehammer, so if you do subsequently tweak the condition you might want to consider conditioning on MOZ_DEBUG_SYMBOLS or MOZ_OPTIMIZE instead as appropriate.
Blocks: 657571
Checked in: http://hg.mozilla.org/mozilla-central/rev/ef9c0d8872cc

I've filed bug 657571 on removing this at some stage if we're able to. If folks want to try improvements to the logic, please deal with those in follow-up bugs, if you want to try it out on the Thunderbird try server, here's what I pushed:

* Pre-patch to get broken build: http://hg.mozilla.org/try-comm-central/rev/610ad75e49ab
* With patch to check get fixed build: http://hg.mozilla.org/try-comm-central/rev/c75bb6e2560b
Assignee: jones.chris.g → mbanner
Status: NEW → RESOLVED
Closed: 13 years ago
tracking-firefox6: ? → ---
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
(In reply to comment #23)
> For what its worth I'll be attempting a build on VC8EE with Win64 as a test
> without this patch, in the next 2 weeks or so...
VC8x64/Win64, 369MB .ilk file. But my VC8/Win32 build fails without the patch...
(In reply to comment #18)
> Oh, that's interesting! cjones: are you on a 32-bit Windows?

Yes.
Okay, sounds like we can probably make this disabling even tighter then, and only do it on 32-bit host systems.
Today I hit this bug building with VC 2010 Express. In the toolkit/library/Makefile.in I see the following lines (different from the patch above):

# See bug 653662 - some builders are hitting an internal size limit
# on incremental builds. Disable this for debug builds using VC8/9.
ifeq ($(OS_ARCH),WINNT)
ifeq (,$(filter-out 1400 1500,$(_MSC_VER)))
ifdef MOZ_DEBUG
EXTRA_DSO_LDOPTS += -INCREMENTAL:NO
endif
endif
endif


I had to comment out
#ifeq (,$(filter-out 1400 1500,$(_MSC_VER)))
(and relevant endif, of course) to be able to compile.
(In reply to Mike Kaganski from comment #29)
> In the
> toolkit/library/Makefile.in I see the following lines (different from the
> patch above):

In which case, according to blame, you'll be more interested in bug 696627.
Thank you. I already found it, excuse me for noise.
(In reply to neil@parkwaycc.co.uk from comment #24)
> What's actually tipping the ilk over the limit? MOZ_DEBUG is a sledgehammer,
> so if you do subsequently tweak the condition you might want to consider
> conditioning on MOZ_DEBUG_SYMBOLS or MOZ_OPTIMIZE instead as appropriate.

(In reply to Ted Mielczarek [:ted, :luser] from comment #28)
> only do it on 32-bit host systems.

I filed bug 700959.
(In reply to Serge Gautherie (:sgautherie) from comment #32)
> (In reply to neil@parkwaycc.co.uk from comment #24)
> > What's actually tipping the ilk over the limit? MOZ_DEBUG is a sledgehammer,
> > so if you do subsequently tweak the condition you might want to consider
> > conditioning on MOZ_DEBUG_SYMBOLS or MOZ_OPTIMIZE instead as appropriate.
> 
> I filed bug 700959.

Forwarded to bug 703845.
(In reply to neil@parkwaycc.co.uk from comment #26)
> (In reply to comment #23)
> > For what its worth I'll be attempting a build on VC8EE with Win64 as a test
> > without this patch, in the next 2 weeks or so...
> VC8x64/Win64, 369MB .ilk file. But my VC8/Win32 build fails without the
> patch...

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

Attachment

General

Creator:
Created:
Updated:
Size: