Closed Bug 81385 Opened 19 years ago Closed 4 years ago

extern vs. non extern warnings.

Categories

(Core :: XPConnect, defect)

Sun
Solaris
defect
Not set

Tracking

()

RESOLVED WONTFIX
Future

People

(Reporter: timeless, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: arch, helpwanted, Whiteboard: [build_warning][~1600 warnings all bad])

Attachments

(2 files)

"/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]
Looks like a bogus warning to me. This is shaver's code.
Assignee: jband → shaver
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.
Keywords: arch, helpwanted
Whiteboard: [~1600 warnings all bad]
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'?
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.
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.
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.
Assignee: shaver → 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.
Status: NEW → ASSIGNED
Target Milestone: --- → Future
Depends on: 92571
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
Blocks: buildwarning
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)
Technique 1 seemed to compile on all our tinderboxes, including ports.
QA Contact: pschwartau → xpconnect
Whiteboard: [~1600 warnings all bad] → [build_warning][~1600 warnings all bad]
Ginn, Can you please share current build warning logs so that we can work on removing them?
Ginn: are these extern "C" warnings still an issue on SPARC?
Assignee: timeless → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(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.
Flags: needinfo?(ginn.chen)
Thanks, Ginn. I just wanted to make sure before I retire this bug.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.