83068 2012-04-03 14:11:56 -0700 enable ULDI with incremental linking of webkit 2012-04-03 18:51:05 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED WONTFIX P2 Normal --- 1 dpranke dpranke abarth dglazkov scottmg tony oldest_to_newest 594873 0 dpranke 2012-04-03 14:11:56 -0700 disable incremental linking for debug of webkit 594877 1 135425 dpranke 2012-04-03 14:14:29 -0700 Created attachment 135425 Patch 594879 2 dpranke 2012-04-03 14:17:07 -0700 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. 594880 3 135425 dpranke 2012-04-03 14:18:08 -0700 Comment on attachment 135425 Patch Clearing flags on attachment: 135425 Committed r113087: <http://trac.webkit.org/changeset/113087> 594881 4 dpranke 2012-04-03 14:18:12 -0700 All reviewed patches have been landed. Closing bug. 594882 5 tony 2012-04-03 14:19:51 -0700 I don't understand. Is incremental_chrome_dll not getting set properly in common.gypi? 594907 6 scottmg 2012-04-03 14:37:11 -0700 (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? 594909 7 dglazkov 2012-04-03 14:38:10 -0700 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. 594925 8 dpranke 2012-04-03 14:54:42 -0700 (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. 594935 9 tony 2012-04-03 15:02:03 -0700 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)? 594942 10 dpranke 2012-04-03 15:12:19 -0700 (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. 594995 11 dglazkov 2012-04-03 16:04:33 -0700 Reverted r113087 for reason: Breaks Windows builds in other unpredictable ways. Committed r113103: <http://trac.webkit.org/changeset/113103> 595125 12 dpranke 2012-04-03 18:51:05 -0700 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. 135425 2012-04-03 14:14:29 -0700 2012-04-03 14:18:08 -0700 Patch bug-83068-20120403141428.patch text/plain 1706 dpranke U3VidmVyc2lvbiBSZXZpc2lvbjogMTEzMDY4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2No cm9taXVtL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCmluZGV4 IGI0Y2NhNjc2NDZmY2I1ZmVkOTIxMTU2NTRmMjM3YTJmZWU4NzlmMzQuLjIxNmFhYzNjY2RkMjYw NzgxNDZiN2JjYzNmMmQyYWM5MzVmOTlhYzcgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvY2hy b21pdW0vQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCkBA IC0xLDMgKzEsMTYgQEAKKzIwMTItMDQtMDMgIERpcmsgUHJhbmtlICA8ZHByYW5rZUBjaHJvbWl1 bS5vcmc+CisKKyAgICAgICAgZGlzYWJsZSBpbmNyZW1lbnRhbCBsaW5raW5nIGZvciBkZWJ1ZyBv ZiB3ZWJraXQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTgzMDY4CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgQWRhbSBCYXJ0aC4KKworICAgICAgICBOb3cg dGhhdCB3ZSBuZWVkIHRvIGV4cG9ydCBzeW1ib2xzIGZyb20gd2Via2l0LmRsbCB0aGF0IGFyZQor ICAgICAgICBkZWZpbmVkIGluIHdlYmNvcmVfcGxhdGZvcm0sIHdlIGhhdmUgdG8gZW5hYmxlIFVM REkgaW4gb3JkZXIgZm9yCisgICAgICAgIGluY3JlbWVudGFsIGxpbmtpbmcgdG8gd29yayAod2hp Y2ggaXMgdXNlZCBpbiBkZWJ1ZyBtb2RlKS4KKworICAgICAgICAqIFdlYktpdC5neXA6CisKIDIw MTItMDQtMDMgIEFkYW0gQmFydGggIDxhYmFydGhAd2Via2l0Lm9yZz4KIAogICAgICAgICBBbm90 aGVyIGF0dGVtcHQgdG8gZml4IHRoZSBXaW5kb3dzIGJ1aWxkLiAgVGhpcyB1c2VzIFdlYlNlY3Vy aXR5T3JpZ2luCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2Nocm9taXVtL1dlYktpdC5neXAg Yi9Tb3VyY2UvV2ViS2l0L2Nocm9taXVtL1dlYktpdC5neXAKaW5kZXggZTAyYTIzY2E2MTE5MTZj OTU1YWIxOWZmZTI4OGJmM2NmNTkzNDFiMy4uZjkwZDA2ZGJlOTVmZTM0NWJhZWM0NmJkNjA4NTIy MDhiNDgwZWJlYyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9jaHJvbWl1bS9XZWJLaXQuZ3lw CisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vV2ViS2l0Lmd5cApAQCAtNzQ3LDExICs3NDcs NyBAQAogICAgICAgICAgICAgICAgICAgICAgICAgICAgIF0sCiAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgJ21zdnNfc2V0dGluZ3MnOiB7CiAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAnVkNMaW5rZXJUb29sJzogewotICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAnY29u ZGl0aW9ucyc6IFsKLSAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBbJ2luY3JlbWVu dGFsX2Nocm9tZV9kbGw9PTEnLCB7Ci0gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAnVXNlTGlicmFyeURlcGVuZGVuY3lJbnB1dHMnOiAidHJ1ZSIsCi0gICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgfV0sCi0gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIF0s CisgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICdVc2VMaWJyYXJ5RGVwZW5kZW5jeUlu cHV0cyc6ICJ0cnVlIiwKICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIH0sCiAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgfSwKICAgICAgICAgICAgICAgICAgICAgICAgIH1dLAo=