74224 - PNG APIs should be using extern "C" functions, not C++ class members

Status: VERIFIED WONTFIX

(Core :: Graphics: ImageLib, defect)

Description

[Mike Kaply [:mkaply]]

See attached diff.

The functions that are passed to PNG should be extern "C" functions, not C++ 
class members since that is what it expects.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Change png callbacks to extern "C"

Comment 2

[Mike Kaply [:mkaply]]

I'm going to do some more thinking on this.

PNG defines PNGAPI for use when defining these APIs. That wouldn't fix OS/2 
bustage because PNGAPI isn't defined right in pngconf.h, but it is probably the 
"right" thing to do.

PNG created PNGAPI specifically for this purpose.

There is still the extern "C" issue even with PNGAPI though. in png.g, all the 
typedefs are in extern "C", so I believe these functions should be in extern "C" 
as well. I'll work on a better solution.

Comment 3

[Mike Kaply [:mkaply]]

OK, I'm attaching a "correct" diff.

I had to change png.h.

The PNG people did a bad thing. They created PNGAPI for 
specifying calling conventions and the like, but they only 
use it in the C files, not the header. So you get type 
mismatches when compiling. This is just a mistake on their 
part.

I added a definition of PNGAPI for IBM OS/2 compilers that 
allows us to specify calling convention.

I changed the code that uses PNG (libpr0n and libimg) to 
use PNGAPI when defining callbacks.

I also made those callbacks extern "C".

Thanks for any input.

Comment 4

[Mike Kaply [:mkaply]]

Attachment: Cleanup of PNG and callbacks

Comment 5

[tor]

Attachment: adding back static to mkaply's changes

Comment 6

[Mike Kaply [:mkaply]]

Does that compile for you?

extern "C" and static are incompatible on my compiler.

Comment 7

[tor]

Compiles fine with gcc.

Comment 8

[Mike Kaply [:mkaply]]

You wouldn't see this problem, but using static in an 
extern "C" { } overrides the extern "C".

So we end up back at the original problem that the calling 
convention is wrong for the callbacks.

In theory this should work, but because the C/C++ people 
chose to overload the extern keyword, it does not.

So if you make a function extern "C", it cannot be static.

We could at least make these names more unique...

Comment 9

[Mike Kaply [:mkaply]]

SunOS gives warnings about these that explain exactly what 
the error is:

"/builds/tinderbox/SeaMonkey/SunOS_5.7_Clobber/mozilla/modu
les/libpr0n/decoders/png/nsPNGDecoder.cpp", line 101: 
Warning (Anachronism): Formal argument info_fn of type 
extern "C" void(*)(png_struct_def*,png_info_struct*) in 
call to png_set_progressive_read_fn(png_struct_def*, void*, 
extern "C" void(*)(png_struct_def*,png_info_struct*), 
extern "C" void(*)(png_struct_def*,unsigned char*,unsigned 
long,int), extern "C" 
void(*)(png_struct_def*,png_info_struct*)) is being passed 
void(*)(png_struct_def*,png_info_struct*).
"/builds/tinderbox/SeaMonkey/SunOS_5.7_Clobber/mozilla/modu
les/libpr0n/decoders/png/nsPNGDecoder.cpp", line 101: 
Warning (Anachronism): Formal argument row_fn of type 
extern "C" void(*)(png_struct_def*,unsigned char*,unsigned 
long,int) in call to 
png_set_progressive_read_fn(png_struct_def*, void*, extern 
"C" void(*)(png_struct_def*,png_info_struct*), extern "C" 
void(*)(png_struct_def*,unsigned char*,unsigned long,int), 
extern "C" void(*)(png_struct_def*,png_info_struct*)) is 
being passed void(*)(png_struct_def*,unsigned 
char*,unsigned long,int).
"/builds/tinderbox/SeaMonkey/SunOS_5.7_Clobber/mozilla/modu
les/libpr0n/decoders/png/nsPNGDecoder.cpp", line 101: 
Warning (Anachronism): Formal argument end_fn of type 
extern "C" void(*)(png_struct_def*,png_info_struct*) in 
call to png_set_progressive_read_fn(png_struct_def*, void*, 
extern "C" void(*)(png_struct_def*,png_info_struct*), 
extern "C" void(*)(png_struct_def*,unsigned char*,unsigned 
long,int), extern "C" 
void(*)(png_struct_def*,png_info_struct*)) is being passed 
void(*)(png_struct_def*,png_info_struct*).

These cannot be static, they must be extern "C".

Can I Get an r= on this patch?

Comment 10

[Mike Kaply [:mkaply]]

Attachment: New diff for libimg/png only based on comments from PNG owner

Comment 11

[Mike Kaply [:mkaply]]

Tim: Could I get an r= on the PNG changes (last attachment)

For posterity, here is the info on extern vs. static - we will need to make 
these "extern" to be correct.

From scc:

  >extern "C" {
  >     static void foo(int blah);
  >}
  >
  >Do you know if later standards clarify this?

Yeah, I checked this out.  This is illegal according to the actual
standard.  It's illegal as written, it's illegal in the combined form

  extern "C" static void foo(int); // error

It's just plain illegal.  You can't have both external and
internal-only linkage specified for the same object.  Also note that
|extern "C"| has no effect on member variables or functions in case
this comes up (though it may impact the types of their parameters).

And from the C compiler folks:

The level of the C++ standard draft (Sept. 92) that the 3.6.5 compiler follows 
states "Functions and objects may be declared static within the {} of a linkage 
specification.  The linkage directive is ignored for such a function or 
object." (section 7.4, para 5).  The 3.6.5 compiler is correct.  Any other 
compiler that follows this draft should be producing the same output as we do. 
I doubt the other compilers use the same level of the draft as we do. The new 
drafts might allow the customers testcase. I would classify this as either a 
change in the language specification or a difference in the implementation of 
non-standard language extensions.

Comment 12

[tor]

sr=tor

(hmm... maybe we should update libpng to the current version, which
includes these changes)

Comment 13

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 14

[Vedran Miletic]

retargeting

Comment 15

[timeless]

.

Comment 16

[Mike Kaply [:mkaply]]

Per tor, the new LIBPNG fixes this stuff.

Comment 17

[timeless]

Attachment: remainder of attachment 31631

It appears that the libpng owner made most of the changes suggested in
attachment 31631 [details] [diff] [review] <tor%cs.brown.edu Oct 3 2002 update libpng to 1.2.5>

This is all that remains of attachment 31631 [details] [diff] [review].

Comment 18

[timeless]

vrfy