Bug 83105 – figure out how to export webcore symbols from webkit.dll properly

• Component: Platform
• Status: RESOLVED FIXED

[Dirk Pranke]

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?

[James Robinson]

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.

[Adam Barth]

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.

[Adam Barth]

Created attachment 135520 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Ami Fischman]

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

[Adam Barth]

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

[Ami Fischman]

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)

[Adam Barth]

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?

[Dimitri Glazkov (Google)]

Comment on attachment 135520 [details]
Patch

Makes sense.

[Dimitri Glazkov (Google)]

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.

[Adam Barth]

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

[Marc-Antoine Ruel]

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

[Adam Barth]

Where would we place that pragma?

[Marc-Antoine Ruel]

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.

[Adam Barth]

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.  :)

[Marc-Antoine Ruel]

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.

[WebKit Review Bot]

Comment on attachment 135520 [details]
Patch

Clearing flags on attachment: 135520

Committed r113218: <http://trac.webkit.org/changeset/113218>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

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