Bug 83068 – enable ULDI with incremental linking of webkit

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Dirk Pranke]

disable incremental linking for debug of webkit

[Dirk Pranke]

Created attachment 135425 [details]
Patch

[Dirk Pranke]

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.

[Dirk Pranke]

Comment on attachment 135425 [details]
Patch

Clearing flags on attachment: 135425

Committed r113087: <http://trac.webkit.org/changeset/113087>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

I don't understand. Is incremental_chrome_dll not getting set properly in common.gypi?

[Scott Graham]

(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?

[Dimitri Glazkov (Google)]

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.

[Dirk Pranke]

(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.

[Tony Chang]

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)?

[Dirk Pranke]

(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.

[Dimitri Glazkov (Google)]

Reverted r113087 for reason:

Breaks Windows builds in other unpredictable ways.

Committed r113103: <http://trac.webkit.org/changeset/113103>

[Dirk Pranke]

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.