Bug 58508 – Fix linking with Sun Studio 12: function pointers for extern "C" are treated differently

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ben Taylor]

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.

[Ben Taylor]

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'

[Ben Taylor]

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

[Alexey Proskuryakov]

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?

[Ben Taylor]

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

[Alexey Proskuryakov]

WTF::MainThreadFunction

[Ben Taylor]

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.

[Alexey Proskuryakov]

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.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 89627 [details]:

http/tests/xmlhttprequest/basic-auth.html bug 51613 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.