Summary: | figure out how to export webcore symbols from webkit.dll properly | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||
Component: | Platform | Assignee: | 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
Dirk Pranke
2012-04-03 18:58:22 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. 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. Created attachment 135520 [details]
Patch
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. (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. > 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. 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) 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 on attachment 135520 [details]
Patch
Makes sense.
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. > 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.
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 Where would we place that pragma? 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. 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. :) 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 on attachment 135520 [details] Patch Clearing flags on attachment: 135520 Committed r113218: <http://trac.webkit.org/changeset/113218> All reviewed patches have been landed. Closing bug. (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. |