81385 - extern vs. non extern warnings.

Status: RESOLVED WONTFIX

(Core :: XPConnect, defect)

Description

[timeless]

"/tmp/mozilla/js/src/xpconnect/src/xpcstring.cpp", line 190: Warning 
(Anachronism): Formal argument
finalizer of type extern "C" void(*)(JSContext*,JSString*) in call to 
JS_AddExternalStringFinalizer(extern "C" void(*)(JSContext*,JSString*)) is 
being passed void(*)(JSContext*,JSString*).
"/tmp/mozilla/js/src/xpconnect/src/xpcstring.cpp", line 222: Warning 
(Anachronism): Formal argument
etor of type extern "C" 
JSDHashOperator(*)(JSDHashTable*,JSDHashEntryHdr*,unsigned,void*) in call to
 JS_DHashTableEnumerate(JSDHashTable*, extern "C" 
JSDHashOperator(*)(JSDHashTable*,JSDHashEntryHdr*,unsigned,void*), void*) is 
being passed JSDHashOperator(*)(JSDHashTable*,JSDHashEntryHdr*,unsigned,void*).
"/tmp/mozilla/js/src/xpconnect/src/xpcstring.cpp", line 226: Warning 
(Anachronism): Formal argument
finalizer of type extern "C" void(*)(JSContext*,JSString*) in call to 
JS_RemoveExternalStringFinalizer(extern "C" void(*)(JSContext*,JSString*)) is 
being passed void(*)(JSContext*,JSString*).
3 Warning(s) detected.
xpcthreadcontext.cpp

this stuff happens a lot. I'm trying to get a tinderbox running so people can 
access my logs.  [Solaris7Sparc Sun ForteC5 Mozilla:Xlib]

Comment 1

[John Bandhauer]

Looks like a bogus warning to me. This is shaver's code.

Comment 2

[Mike Kaply [:mkaply]]

There are 1600 of these warnings and none of them are bogus.

According to the C/C++ spec, if you say that a function is extern "C" and 
receives extern "C" functions, then everything you pass must be extern "C".

All throughout Mozilla, we pass static C++ member functions to things that are 
not supposed to get them.

Fixing this is a real monster, probably a post 1.0 issue.

Every static C++ function that has this problem must be pulled out of the 
class, usually made a friend, and then put in an extern "C" block.
This also means that they all become extern rather than static because you 
cannot have both (see http://bugzilla.mozilla.org/show_bug.cgi?id=74224).

So we should mark this a future and keep it on the radar.

Comment 3

[John Bandhauer]

mkaply: OK, fine. They are not bogus. What you did not say is what is broken by 
this pattern. Does this cause some platform to not work? Is this only a 
correctness issue? What is the 'badnes'?

Comment 4

[Scott Collins]

This could potentially break any platform where C and C++ had different calling
conventions, which is explicitly allowed.  Bill Law works on such a platform, I
believe it's OS2 or perhaps AIX.

Comment 5

[Mike Kaply [:mkaply]]

Although at this point it is totally a correctness issue.

It was an OS/2 issue a long time ago, but we solved it differently.

Rather than:

extern "C" {
   void foo()
}

We did

void PR_CALLBACK foo()

which accomplished the same thing - getting code to compile, and having the 
right calling convention. We did this because we didn't understand that the 
core of the problem was the extern "C" mismatch, not the calling convention 
mismatch.

However, even if the extern "C" is put in properly we would still need the 
PR_CALLBACK, though because for correctness you should put the calling 
convention on the API to guarantee things match.

So we know of the problem, and when new stuff arises we want to do it the right 
way, but I don't know of anyone that wants to go through the trouble of fixing 
this when there are so many other things to worry about.

Comment 6

[Mike Shaver (:shaver emeritus)]

Handing off to timeless, so he can produce a list from his build and put it up
somewhere for people to pay really close attention to.  Like all our other warnings.

Comment 7

[timeless]

I should have done this *before* I lost access to my sparcs. I wonder if I could use dcran's or just get their output.

Comment 8

[Donnie Cranford]

timeless,

Sure thing, I have Forte 7.0 Fully licensed now, so All you need is to pull a
branch, get in touch with me and Ill set you up ...

dcran

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Looking at the C++ standard (the out-of-date 1998 version, I admit), as far as I
can tell a conformant compiler is *required* to give a diagnostic for this.  In
particular, section 7.5 [dcl.link] clause 1 says that function types with
different linkage are different types, section 4 [conv] does not describe any
standard conversions that could convert between them, and section 5.2.2
[expr.call] clause 4 and section 8.5 [dcl.init] clauses 12 and 14 seem to
require that such a conversion exist.

It seems like there are two workarounds for this:

  1) Especially when using static member functions as the callbacks, those
functions can be declared using a typedef that is declared in a manner like:
  extern "C" typedef void (PR_CALLBACK FUNC)();
  /* Note no "*".  C++ makes a distinction between function types and
     pointer-to-function types, and provides for implicit conversion from
     the former to the latter.  C doesn't seem to make that distinction (see
     C99 (ISO/IEC 9899:1999), section 6.3.2.1, clause 4).
     Our type declarations in C all seem to use the "*" form; I'm not sure
     what's allowed in C and what's portable (i.e., whether we could change
     it within C files) */
  /* I hope that use of PR_CALLBACK works! */
as something like:
  static FUNC MyHandler;
and then defined as:
  void PR_CALLBACK Class::MyHandler() { ... }
or
  void PR_CALLBACK MyHandler() { ... }

  2) (This doesn't work for static member functions.)  The function could be
initially declared within an extern "C" block, and then defined:

  extern "C" { PR_STATIC_CALLBACK(void) MyHandler(); }
  void PR_CALLBACK MyHandler() { ... }


bz may test whether technique (1) is portable when he lands bug 294114 (see bug
294114 comment 11 and on)

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I commented on http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8680 to see if
someone would resurrect it.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Technique 1 seemed to compile on all our tinderboxes, including ports.

Comment 12

[Atul Aggarwal]

Ginn, Can you please share current build warning logs so that we can work on removing them?

Comment 13

[Ginn Chen]

Attachment: full buildlog on Solaris x86

Comment 14

[Ginn Chen]

Attachment: warnings about extern vs non extern (5337 lines)

Comment 15

[Chris Peterson [:cpeterson]]

Ginn: are these extern "C" warnings still an issue on SPARC?

Comment 16

[Ginn Chen]

For Firefox/Thunderbird after 31.0, I've switched to GNU compiler.
And I don't think warnings are priority for Solaris platform now.

Comment 17

[Chris Peterson [:cpeterson]]

Thanks, Ginn. I just wanted to make sure before I retire this bug.