Bug 58508 - Fix linking with Sun Studio 12: function pointers for extern "C" are treated differently
Summary: Fix linking with Sun Studio 12: function pointers for extern "C" are treated ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-13 20:05 PDT by Ben Taylor
Modified: 2011-04-15 00:56 PDT (History)
2 users (show)

See Also:


Attachments
Fix linking issue on Solaris 10 with Sun Studio 12, Fix a linking problem for a plugin, as function pointers for extern "C" are handled different (2.54 KB, patch)
2011-04-13 21:46 PDT, Ben Taylor
ap: review-
ap: commit-queue-
Details | Formatted Diff | Diff
Updated patch which fixes the linking issue for the plugin, as function pointers for extern "C" are handled differently (1.83 KB, patch)
2011-04-14 12:35 PDT, Ben Taylor
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Taylor 2011-04-13 20:05:37 PDT
The Sun CC compiler treats C functions and C++ functions differently, as if they had a different calling sequence (they don't, but they
could). So if you declare a function in C++ having a function pointer as a parameter, it's understood to be C++ even if it had previously been declared as extern "C".

This could be a compiler error, though. In any case, the end result is that WebKit fails to link because of an undefined reference to
NPN_PluginThreadAsyncCall.

Problem originally identified  by Thiago Macieria in bug https://bugs.webkit.org/show_bug.cgi?id=24932 and
fixed by patch webkit17-17.diff in his suite.
Comment 1 Ben Taylor 2011-04-13 20:14:00 PDT
Here's the current error message without the patch.

pkgbuild: Undefined                     first referenced
pkgbuild:  symbol                           in file
pkgbuild: NPN_PluginThreadAsyncCall           /export/home/bent/packages/BUILD/qt-4.7.2/i386/qt-everywhere-opensource-src-4.7.2/lib/libQtWebKit.so
pkgbuild: ld: fatal: Symbol referencing errors. No output written to ../../../../bin/assistant
pkgbuild: make[4]: *** [../../../../bin/assistant] Error 1
pkgbuild: make[4]: Leaving directory `/export/home/bent/packages/BUILD/qt-4.7.2/i386/qt-everywhere-opensource-src-4.7.2/tools/assistant/tools/assistant'
Comment 2 Ben Taylor 2011-04-13 21:46:51 PDT
Created attachment 89530 [details]
Fix linking issue on Solaris 10 with Sun Studio 12, Fix a linking problem for a plugin,  as function pointers for extern "C" are handled different

Patch to fix the linking issue on Solaris with Sun Studio 12
Comment 3 Alexey Proskuryakov 2011-04-13 23:16:30 PDT
Comment on attachment 89530 [details]
Fix linking issue on Solaris 10 with Sun Studio 12, Fix a linking problem for a plugin,  as function pointers for extern "C" are handled different

View in context: https://bugs.webkit.org/attachment.cgi?id=89530&action=review

> Source/JavaScriptCore/wtf/MainThread.h:40
> +extern "C" {
> +    typedef void MainThreadFunction(void*);
> +}

This doesn't look good to me. We use callOnMainThread in many places, and every time, we supply a C++ function to it (see e.g. Document::postTask()).

> Source/WebCore/ChangeLog:10
> +        No new tests. (OOPS!)

The patch cannot be landed with this line. You can just remove it, since it's obvious why there are no tests.

> Source/WebCore/plugins/npapi.cpp:179
> +extern "C" {
> +    void NPN_PluginThreadAsyncCall(NPP instance, void(*func) (void *), void *userData)
> +    {
> +        PluginMainThreadScheduler::scheduler().scheduleCall(instance, func, userData);
> +    }
>  }

I don't understand why you had to wrap function body in extern "C". Its declaration already has extern "C", so the compiler knows that it has C linkage. Note that you didn't face a problem with NPN_ScheduleTimer, which also takes a function pointer.

Would the following change alone do the trick?

    PluginMainThreadScheduler::scheduler().scheduleCall(instance, (MainThreadFunction)func, userData);

And if that works, would a C++ style cast (probably a reinterpret_cast) work, too?
Comment 4 Ben Taylor 2011-04-14 04:29:30 PDT
(In reply to comment #3) 
> I don't understand why you had to wrap function body in extern "C". Its declaration already has extern "C", so the compiler knows that it has C linkage. Note that you didn't face a problem with NPN_ScheduleTimer, which also takes a function pointer.
> 
> Would the following change alone do the trick?
> 
>     PluginMainThreadScheduler::scheduler().scheduleCall(instance, (MainThreadFunction)func, userData);
> 
> And if that works, would a C++ style cast (probably a reinterpret_cast) work, too?

Here is the output from the compiler when we try your suggestion. 

"plugins/npapi.cpp", line 175: Warning: function void(_NPP*,void(*)(void*),void*) overloads extern "C" void(_NPP*,extern "C" void(*)(void*),void*) because of different language linkages.
"plugins/npapi.cpp", line 176: Error: MainThreadFunction is not defined.
"plugins/npapi.cpp", line 176: Error: Badly formed expression.

Thoughts?
Comment 5 Alexey Proskuryakov 2011-04-14 08:13:58 PDT
WTF::MainThreadFunction
Comment 6 Ben Taylor 2011-04-14 12:35:15 PDT
Created attachment 89627 [details]
Updated patch which fixes the linking issue for the plugin, as function pointers for extern "C" are handled differently

Updated patch, per reviewer comments and suggestions, to fix this linking error in Solaris with Sun Studio 12.
Comment 7 Alexey Proskuryakov 2011-04-14 12:46:54 PDT
Comment on attachment 89627 [details]
Updated patch which fixes the linking issue for the plugin, as function pointers for extern "C" are handled differently

Ideally, we should make PluginMainThreadScheduler::MainThreadFunction extern "C" instead of doing reinterpret_cast. But this is an improvement already.
Comment 8 WebKit Commit Bot 2011-04-15 00:53:34 PDT
The commit-queue encountered the following flaky tests while processing attachment 89627 [details]:

http/tests/xmlhttprequest/basic-auth.html bug 51613 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 9 WebKit Commit Bot 2011-04-15 00:56:04 PDT
Comment on attachment 89627 [details]
Updated patch which fixes the linking issue for the plugin, as function pointers for extern "C" are handled differently

Clearing flags on attachment: 89627

Committed r83957: <http://trac.webkit.org/changeset/83957>
Comment 10 WebKit Commit Bot 2011-04-15 00:56:09 PDT
All reviewed patches have been landed.  Closing bug.