Bug 83105

Summary: figure out how to export webcore symbols from webkit.dll properly
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: PlatformAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, fischman, fishd, jamesr, maruel, rvargas, scottmg, tkent, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Dirk Pranke 2012-04-03 18:58:22 PDT
We've stumbled onto an issue in attempting to export a symbol out of chromium/public/WebMediaStreamSourcesRequest.h, which is part of webcore_platform now.

Since WebMediaStreamSourcesRequest.cpp is not referenced by anything in WebKit directly (apparently), it looks like we run into an issue on windows where, when building webkit.dll, we think the file is dead/unused code, and it isn't linked into the DLL.

We attempted to work around this by enabling ULDI for webkit.dll in bug 83068, but that appears to have caused other weird issues downstream in chromium, so we reverted that.

For now we've hacked around it in http://trac.webkit.org/changeset/113105 by forcing a reference from WebKit.cpp to the file, but this is not a good nor scalable solution.

It seems like the "correct" solution is to not build webcore_platform as a .lib at all, but to include the objects directly into the link of webkit.lib/webkit.dll (this is basically what I did for content.dll). However, we suspect that we'll hit the 128KB command line limit for the linker, like we did when we tried this w/ chrome.dll, and what caused Scott to write 'supalink' to break up the command line.

So, it's not clear what the best way forward is ... thoughts?
Comment 1 James Robinson 2012-04-03 20:24:11 PDT
Could we make a dedicated section of the .gypis for implementations of WebKit API that aren't in webkit.lib (this would be Source/WebCore/platform/chromium/support and Source/Platform/chromium/src, I believe) and have those objects be stuck in webkit.dll?  We need to be pretty careful about where we put implementations of exported symbols anyway to make sure we don't break APIs so that won't be an unbounded set.
Comment 2 Adam Barth 2012-04-03 22:55:51 PDT
Yeah, Comment #1 is what I was thinking about on the drive home tonight too.  We should just compile Source/WebCore/platform/chromium/support/* as part of webkit.lib to make sure the obj files are in the DLL, causing the linker to export the symbols.  Apparently we do similar things in other parts of Chrome.
Comment 3 Adam Barth 2012-04-03 23:18:46 PDT
Created attachment 135520 [details]
Patch
Comment 4 WebKit Review Bot 2012-04-03 23:22:32 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 5 Ami Fischman 2012-04-03 23:43:28 PDT
(In reply to comment #2)
> Yeah, Comment #1 is what I was thinking about on the drive home tonight too.  We should just compile Source/WebCore/platform/chromium/support/* as part of webkit.lib to make sure the obj files are in the DLL, causing the linker to export the symbols.  Apparently we do similar things in other parts of Chrome.

Doesn't this fly in the face of bug 82948?  (and lose the benefit of the compiler/linker verifying that code in webcore_platform not depend on code above webcore (specifically, in webkit))

I expected the right fix to mark WebMediaStreamSourcesRequest (and any other public-API classes in Platform) as exported, via something like WEBKIT_EXPORT (although presumably using a (new) PLATFORM_EXPORT macro, instead).  

Maybe I'm just confused.
Comment 6 Adam Barth 2012-04-03 23:57:10 PDT
> Doesn't this fly in the face of bug 82948?  (and lose the benefit of the compiler/linker verifying that code in webcore_platform not depend on code above webcore (specifically, in webkit))

We do lose some benefits in the sense that the include rules are more relaxed on these compilation units than we would like.  My understanding is that there's a limitation of GYP that you cannot use different include paths within a given lib.  If we need to link these obj files as part of webkit.lib, that implies that we need to use the looser include paths.

Note, however, that we will be able to achieve the goal of Bug 82948, which is to remove PlatformSupport.  Logically, these files are part of WebCore/platform.  We just happen to compile and link them as part of webkit.lib rather than as part of webcore_platform.lib.

> I expected the right fix to mark WebMediaStreamSourcesRequest (and any other public-API classes in Platform) as exported, via something like WEBKIT_EXPORT (although presumably using a (new) PLATFORM_EXPORT macro, instead).  

If you know of a way to do that and make the Windows component build work, I'm all ears!  That would be much better than the approach in this patch.

My understanding of the issue is as follows:

When constructing webkit.dll, the linker starts with all the obj files in webkit.lib and constructs the list of unmet dependencies.  While there are still at least one unmet dependency, the linker searches the libs on the link line for obj files that contain that dependency.  Upon finding such an obj file, the obj file is added to webkit.dll and any new unmet dependencies are added to the list of unmet dependencies.  This process terminates either when the list of unmet dependencies is empty or when the linker fails to find an obj file to satisfy an unmet dependency.

Finally, the linker exports all the symbols in webkit.dll that are marked for export, and creates a stub lib containing those symbols, etc.

Now, the problem in this case is that none of the symbols in WebMediaStreamSourcesRequest.obj are unmet dependencies of webkit.lib and hence WebMediaStreamSourcesRequest.obj never gets pulled into webkit.dll.  Because WebMediaStreamSourcesRequest.obj never gets pulled into webkit.lib, the linker never sees that some of the symbols in WebMediaStreamSourcesRequest.obj are marked for export and hence those symbols are not exported by webkit.dll.

We tried a fix earlier today that forced the link to slam all the obj files into webkit.dll.  At that point, the linker sees the exported symbols in WebMediaStreamSourcesRequest.obj and exports them in webkit.dll.  However, that change caused a compile error for Aura (for reason I don't fully understand).  It's also frowned upon by folks who know infinitely more about our build system than I do.

This change similarly forces the linker to include WebMediaStreamSourcesRequest.obj in webkit.lib by moving the obj file from webcore_platform.lib to webkit.lib.  It's certainly not ideal, but I think it should work.
Comment 7 Ami Fischman 2012-04-04 00:23:21 PDT
Is the problem that MSVC's linker doesn't have an equivalent to ld's --whole-archive (which would allow webkit.dll to include wholesale webcore_platform.lib)?
(I know nothing about MSVC but google points me at http://msdn.microsoft.com/en-US/library/bxwfs976%28v=VS.80%29.aspx)
Comment 8 Adam Barth 2012-04-04 00:26:05 PDT
We did try one, which is what I was referring to with "We tried a fix earlier today that forced the link to slam all the obj files into webkit.dll."  Maybe there are other ways to do that better?
Comment 9 Dimitri Glazkov (Google) 2012-04-04 09:44:52 PDT
Comment on attachment 135520 [details]
Patch

Makes sense.
Comment 10 Dimitri Glazkov (Google) 2012-04-04 09:46:19 PDT
We could also add an explicit exclusion of chromium/support files in WebKit.gyp, so that they never get accidentally added to the build files.
Comment 11 Adam Barth 2012-04-04 10:26:31 PDT
> We could also add an explicit exclusion of chromium/support files in WebKit.gyp, so that they never get accidentally added to the build files.

Thats and interesting idea.  I'd be inclined to do that after the first time we mess up.
Comment 12 Marc-Antoine Ruel 2012-04-04 10:31:53 PDT
General note:

To force MSVC to include a symbol in the final PE (dll or exe) that is located in a static library (lib), you can use :

#pragma comment(linker, '/include:<symbol>')

where the symbol is preferably an extern "C" to not have to manage the C++ mangling.

General doc: http://msdn.microsoft.com/en-us/library/7f0aews7.aspx
Comment 13 Adam Barth 2012-04-04 10:45:02 PDT
Where would we place that pragma?
Comment 14 Marc-Antoine Ruel 2012-04-04 10:50:46 PDT
IIRC, any object that is ultimately included for consideration in the final link step is fine. This means any .obj inside any .lib is fine.

FTR, this is what ATL does to inject globals in the final executable.

But my memory is failing. So if this fails, at worst put it an header file which will be included in one .obj that will be linked directly in the final link step.
Comment 15 Adam Barth 2012-04-04 10:53:48 PDT
Thanks for the pointer.  I don't think that's a great option for us here because this is a C++ API, which means we'd need to use the mangled symbol.  However, it's a good trick to add to the basket.  :)
Comment 16 Marc-Antoine Ruel 2012-04-04 10:58:04 PDT
Assign the address of a C++ symbol to a C symbol.

#ifdef _MSC_VER

// Make sure the symbol is included.
// Note the lack of function call. It's taking the function's address.
extern "C" const void * const force_include_platform_support = static_cast<const void*>(&webKitPlatformSupport);

#pragma comment(linker, '/include:__force_include_platform_support')

#endif // _MSC_VER


or something roughly like that, I haven't tested.
Comment 17 WebKit Review Bot 2012-04-04 11:14:33 PDT
Comment on attachment 135520 [details]
Patch

Clearing flags on attachment: 135520

Committed r113218: <http://trac.webkit.org/changeset/113218>
Comment 18 WebKit Review Bot 2012-04-04 11:14:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Dirk Pranke 2012-04-04 12:22:12 PDT
(In reply to comment #7)
> Is the problem that MSVC's linker doesn't have an equivalent to ld's --whole-archive (which would allow webkit.dll to include wholesale webcore_platform.lib)?
> (I know nothing about MSVC but google points me at http://msdn.microsoft.com/en-US/library/bxwfs976%28v=VS.80%29.aspx)

The option we tried (for reference) was "Use Library Dependency Inputs" (which is a VS option, not a linker option, but I think is a closer analog to --whole-archive); I'm not familiar with /opt:noref but it sounds like it does dead code stripping *after* it's figured out which objects it's supposed to link, so I would hazard a guess that it wouldn't cause the archive member to be pulled in.

IIRC, the downstream failure was on the Chromium "Win Aura"; I hadn't looked at the error until now, but I think it was probably from this build: http://build.chromium.org/p/chromium/builders/Win%20Aura/builds/9390 , and it looks like we just blew out the 128KB command line length to the linker, which doesn't surprise me (as mentioned in comment #1).

Note that you can run into other weirdness when you do something like --whole-archive, which is that you can multiply defined symbols and, sometimes worse, multiple copies of singletons. Much of the work of making things work in component builds is restructuring the libraries so that we don't have these issues.