Bug 41930 – [Qt] QtWebKit doesn't build in debug on Windows

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Mahesh Kulkarni]

qtwebkit on windows for debug version complains with linker errors. Disabling incremental build fixes the issue with msvs

[Mahesh Kulkarni]

Created attachment 61018 [details]
fixes build issue

On both win32-msvc2005|win32-msvc2008, qtwebkit doesn't build for debug. 
Release would anyway have this flag set. This patch proposes to add the flag to debug version as well.

[Andreas Kling]

This is a 32-bit only issue, no?

AFAIK incremental linking works just fine on 64-bit. We don't want to turn it off for everyone in that case.

Could you paste the error message you're getting from the linker?

[Mahesh Kulkarni]

(In reply to comment #2)
> This is a 32-bit only issue, no?
> 
> AFAIK incremental linking works just fine on 64-bit. We don't want to turn it off for everyone in that case.
> 
> Could you paste the error message you're getting from the linker?

Yes mine is 32-bit machine. As per your comments probably this change needs to be done only for 32-bit machines.

The error we get on all 32 bit machines here while linking webkit dll, 
"LINK : fatal error LNK1210: exceeded internal ILK size limit; link with 
/INCREMENTAL:NO"

[Mahesh Kulkarni]

(In reply to comment #3)
> (In reply to comment #2)
> > This is a 32-bit only issue, no?
> > 
> > AFAIK incremental linking works just fine on 64-bit. We don't want to turn it off for everyone in that case.
> > 
> > Could you paste the error message you're getting from the linker?
> 
> Yes mine is 32-bit machine. As per your comments probably this change needs to be done only for 32-bit machines.
> 
> The error we get on all 32 bit machines here while linking webkit dll, 
> "LINK : fatal error LNK1210: exceeded internal ILK size limit; link with 
> /INCREMENTAL:NO"

opps, the patch does it only for 32-bit windows machines already!

[Andreas Kling]

(In reply to comment #4)
> > The error we get on all 32 bit machines here while linking webkit dll, 
> > "LINK : fatal error LNK1210: exceeded internal ILK size limit; link with 
> > /INCREMENTAL:NO"
> 
> opps, the patch does it only for 32-bit windows machines already!

Ok, this makes sense to me. CC'ing Simon for review.

[Laszlo Gombos]

I might be wrong but I do not think "win32" is just for 32 bit. To test for 64 bit one should try "contains(QMAKE_HOST.arch, x86_64)".

Best is to try/test on 64 bit machine.

[Simon Hausmann]

Comment on attachment 61018 [details]
fixes build issue

If you look at mkspecs/win32*/qmake.conf in Qt, then you can see that we use INCREMENTAL:NO for release builds:

QMAKE_LFLAGS_RELEASE    = /INCREMENTAL:NO

I think a cleaner fix would be to use either
QMAKE_LFLAGS_DEBUG +=
or at the very least add a comment in the .pro file what exactly this magic line tries to achieve.

For example:

# Release builds disable incremental
# linking. Disable it also for debug
# builds because WebKit is so big
# that the linker fails to link
# incrementally in debug builds.
QMAKE_LFLAGS_DEBUG += /INCREMENTAL:NO

[Mahesh Kulkarni]

Created attachment 61618 [details]
updated comments

updated patch as per comments #7 from Simon.

[Mahesh Kulkarni]

(In reply to comment #6)
> I might be wrong but I do not think "win32" is just for 32 bit. To test for 64 bit one should try "contains(QMAKE_HOST.arch, x86_64)".
> 
> Best is to try/test on 64 bit machine.

I do not have 64-bit machine to try the latest patch out :( 

Andreas Kling, would it be able possible for you to test this on your 64-bit machine?

[Jocelyn Turcotte]

(In reply to comment #9)

I tried contains(QMAKE_HOST.arch, x86_64) but this would disable incremental linking when qmake is compiled as 32bit on a 64bit machine.

Something like this might work, the PROCESSOR_ARCHITEW6432 environment variable is only defined when running a 32bit process on a 64bit Windows.
( info: http://blogs.msdn.com/b/david.wang/archive/2006/03/26/howto-detect-process-bitness.aspx )

ARCH = $$(PROCESSOR_ARCHITECTURE)
WOW64ARCH = $$(PROCESSOR_ARCHITEW6432)
equals(ARCH, x86): isEmpty(WOW64ARCH): QMAKE_LFLAGS_DEBUG += /INCREMENTAL:NO

Also please span your comment on less lines (something like 100 chars wide), this would help keeping the scrolling convenint in that file while searching for a specific line.

[Mahesh Kulkarni]

(In reply to comment #10)
> (In reply to comment #9)
> 
> I tried contains(QMAKE_HOST.arch, x86_64) but this would disable incremental linking when qmake is compiled as 32bit on a 64bit machine.
> 
> Something like this might work, the PROCESSOR_ARCHITEW6432 environment variable is only defined when running a 32bit process on a 64bit Windows.
> ( info: http://blogs.msdn.com/b/david.wang/archive/2006/03/26/howto-detect-process-bitness.aspx )
> 
> ARCH = $$(PROCESSOR_ARCHITECTURE)
> WOW64ARCH = $$(PROCESSOR_ARCHITEW6432)
> equals(ARCH, x86): isEmpty(WOW64ARCH): QMAKE_LFLAGS_DEBUG += /INCREMENTAL:NO
> 
> Also please span your comment on less lines (something like 100 chars wide), this would help keeping the scrolling convenint in that file while searching for a specific line.

Jocelyn, we see the same issue even on 32-bit windows OS on 64 bit machine. 
We tested this again on 64 bit machine, as long as its a 32-bit windows OS, incremental build fails. 

I will upload the patch again with lesser lines in comments section. 

Laszlo, any comments on existing fix?

[Jocelyn Turcotte]

(In reply to comment #11)
> Jocelyn, we see the same issue even on 32-bit windows OS on 64 bit machine. 
> We tested this again on 64 bit machine, as long as its a 32-bit windows OS, incremental build fails. 
> 
> I will upload the patch again with lesser lines in comments section. 
> 
> Laszlo, any comments on existing fix?

I meant a 64bit OS on a 64bit machine when I wrote "64bit machine", sorry for the confusion.

[Andreas Kling]

Comment on attachment 61618 [details]
updated comments

>WebCore/WebCore.pro:110
> +  win32-msvc2005|win32-msvc2008:{
This will affect both 32-bit and 64-bit Windows.

r- because we need to find a solution that doesn't unnecessarily hurt linking time on 64-bit Windows.

[Mahesh Kulkarni]

Created attachment 74634 [details]
patch

Jocelyn Turcotte, Thanks. 

Updated patch as per comment #10

[Kenneth Rohde Christiansen]

Comment on attachment 74634 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74634&action=review

> WebCore/WebCore.pro:131
> +    # Disable incremental linking for windows 32bit OS debug build as WebKit so big

WebKit so big? Do you mean "WebKit is so big" ?

> WebCore/WebCore.pro:132
> +    # that linker failes to link incrementally in debug mode

You need a dot at the end; that is WebKit comment style.

[Eric Seidel (no email)]

Comment on attachment 74634 [details]
patch

Seems OK besides the above comment comments.  Are you going to post a new patch for krc?

[Mahesh Kulkarni]

Created attachment 77098 [details]
patch

Changes made as per comment #15. Please review.

[Andreas Kling]

Comment on attachment 77098 [details]
patch

Much better! r=me :)

[WebKit Commit Bot]

Comment on attachment 77098 [details]
patch

Clearing flags on attachment: 77098

Committed r74409: <http://trac.webkit.org/changeset/74409>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.