Bug 83068 - enable ULDI with incremental linking of webkit
Summary: enable ULDI with incremental linking of webkit
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-03 14:11 PDT by Dirk Pranke
Modified: 2012-04-03 18:51 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.67 KB, patch)
2012-04-03 14:14 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-04-03 14:11:56 PDT
disable incremental linking for debug of webkit
Comment 1 Dirk Pranke 2012-04-03 14:14:29 PDT
Created attachment 135425 [details]
Patch
Comment 2 Dirk Pranke 2012-04-03 14:17:07 PDT
I'm not 100% sure this is the right long-term fix, but in theory this should at least fix the compile errors we're seeing on the debug win bots. If it does, we can double check this and see that this is right.
Comment 3 Dirk Pranke 2012-04-03 14:18:08 PDT
Comment on attachment 135425 [details]
Patch

Clearing flags on attachment: 135425

Committed r113087: <http://trac.webkit.org/changeset/113087>
Comment 4 Dirk Pranke 2012-04-03 14:18:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Tony Chang 2012-04-03 14:19:51 PDT
I don't understand. Is incremental_chrome_dll not getting set properly in common.gypi?
Comment 6 Scott Graham 2012-04-03 14:37:11 PDT
(In reply to comment #2)
> I'm not 100% sure this is the right long-term fix, but in theory this should at least fix the compile errors we're seeing on the debug win bots. If it does, we can double check this and see that this is right.

I don't understand either. Shouldn't it be 'false' if we're disabling it?
Comment 7 Dimitri Glazkov (Google) 2012-04-03 14:38:10 PDT
The bots are still failing: http://build.chromium.org/p/chromium.webkit/builders/Win%20%28dbg%29

I am trying to clobber to see if it helps.
Comment 8 Dirk Pranke 2012-04-03 14:54:42 PDT
(In reply to comment #5)
> I don't understand. Is incremental_chrome_dll not getting set properly in common.gypi?

(In reply to comment #6)
> I don't understand either. Shouldn't it be 'false' if we're disabling it?

So, the way things are currently, we enable incremental linking (but not ULDI) in debug build, and incremental_chrome_dll is set to False. So, prior to this patch, ULDI was not being enabled for webkit.dll.

The problem we're seeing is that we're trying to export a symbol from a file in webcore_platform, and nothing else in webkit.dll references anything in that file, so the linker would normally treat that file as dead code and strip it.

My understanding of the flags is that by enabling ULDI, we force the linker to include all of the objects in the archive, regardless of whether or not they're referenced by something else in the DLL.

Obviously, this may have side effects, which is why I'm not sure that this is the right long-term fix.
Comment 9 Tony Chang 2012-04-03 15:02:03 PDT
If I understand you correctly, you're saying the bug title is wrong (causing Scott's confusion) and the gyp variable incremental_chrome_dll does not control whether we link incrementally or not (causing my confusion).

Are you saying we should remove incremental_chrome_dll from all the gyp files and assume incremental linking all the time?  Is there some other gyp variable we should be checking instead (or in addition to incremental_chrome_dll)?
Comment 10 Dirk Pranke 2012-04-03 15:12:19 PDT
(In reply to comment #9)
> If I understand you correctly, you're saying the bug title is wrong (causing Scott's confusion)

Oy. Yes. My fault, I've updated the subject correctly - the fix was to turn ULDI on, not turn incremental linking off.

> and the gyp variable incremental_chrome_dll does not control whether we link incrementally or not (causing my confusion).
> 

That is correct. It only controls whether we link chrome.dll incrementally. I suspect it was a hack that it was in webkit.gyp at all, but possibly a hack needed for the same reasons.

> Are you saying we should remove incremental_chrome_dll from all the gyp files and assume incremental linking all the time? 

No, but there's only a couple of linkables where this problem shows up - the main one being chrome.dll. The problem is that incremental linking doesn't play nicely with a DLL comprised of multiple .libs, where the DLL exports symbols from multiple libs that it doesn't use itself.

One way to fix this is to not use .libs at all, and just build the DLL directly from a list of source files; this is what I ended up doing in content (and this is basically what ULDI is doing), but I'm not sure if this would work in webkit since it's so much bigger (we might run into command line length issues and have to do something like what Scott did w/ supalink).

> Is there some other gyp variable we should be checking instead (or in addition to incremental_chrome_dll)?

I'm not sure.
Comment 11 Dimitri Glazkov (Google) 2012-04-03 16:04:33 PDT
Reverted r113087 for reason:

Breaks Windows builds in other unpredictable ways.

Committed r113103: <http://trac.webkit.org/changeset/113103>
Comment 12 Dirk Pranke 2012-04-03 18:51:05 PDT
We worked around the issue in r113105 for now ... I will close this as WONTFIX (since it's probably not the right thing to do) and open a different bug for the underlying problem.