STR #2878: Many warnings during MSWindows 7 64-bit compilation.

STR #2878

Application:	FLTK Library
Duplicate Of:	STR #2813
Status:	1 - Closed w/Resolution
Priority:	2 - Low, e.g. a documentation error or undocumented side-effect
Scope:	2 - Specific to an operating system
Subsystem:	WIN32
Summary:	Many warnings during MSWindows 7 64-bit compilation.
Version:	1.3-current
Created By:	manolo
Assigned To:	manolo
Fix Version:	1.3.4
Update Notification:	Receive EMails Don't Receive EMails

Trouble Report Files:

#1	manolo 07:09 Oct 08, 2012

union.diff
4k

#2	manolo 07:07 Apr 02, 2015

union.patch
4k

Trouble Report Comments:

#1	manolo 07:09 Oct 08, 2012

I compiled FLTK for MSWindows 7 64-bit using the MinGW 64 bit compiler (gcc 4.7.0) and found that there are 3 warnings for each source file plus a few more for those that include FL/Fl_Menu_Item.H. They come from cast operations between the long and void* types that differ in length on the 64-bit Windows7 platform. I believe the correct solution for that is to replace void* user_data_; by union { void* user_data_; long long_value_; }; in the declaration of the Fl_Widget class, and to use long_value_ instead of user_data_ wherever a long quantity is handled. The attached file contains a patch with this change, protected by #if FLTK_ABI_VERSION >= 10302, although I think the change does not modify the class layout on real platforms. Could another developer test this change with Microsoft's 64-bit compiler, and report what happens?

#2	ianmacarthur 15:54 Sep 04, 2014

It looks like we now have multiple STR's covering this ground (the STR numbers for which I can not recall now...) where 64-bit Windows is a pain because sizeof(long) != sizeof(pointer). We should probably try and link them all together and sir this just once! Do we need to union, or can we just cast this away? Or, would using fl_intptr_t be a better solution? And will it alter the ABI...?

#3	ianmacarthur 03:55 Sep 05, 2014

See also; #2878 #2813 #3038

#4	manolo 07:09 Apr 02, 2015

Attached file #2 (union.patch) fixes all compilation warnings under 64-bit MSWindows.

#5	AlbrechtS 09:07 Apr 02, 2015

Unfortunately this patch does not solve the problems we have - it changes semantics, and this will certainly break existing programs. Furthermore it introduces endianness issues. The problem is that we allow to store a long value and still call the callback with 'void *' (pointer) semantics. As long as the programmer knows what s/he does this works, although we have warnings. The patched software would store undefined data in the userdata_ field: FL/Fl_Widget.H: - void* user_data_; + union { + void *user_data_; + long user_value_; + }; - void argument(long v) {user_data_ = (void*)v;} + void argument(long v) {user_value_ = v;} The above method would store user_value_ (the smaller item), but leave the rest of the union as-is (undefined). Fl_Widget.H, line #851: void do_callback() {do_callback(this,user_data_);} This accesses user_data_, i.e. the whole union, and hence undefined data. There is a clear rule that you MUST access data in a union only by the same item you used to store the data. We can't guarantee this, since we don't know what the user's program code does. In practice unions may often be used to do exactly that (store in one item, access another item), but that is undefined behavior and certainly doesn't work here. Here is an untested example with currently legal and (hopefully) working code): button->callback(my_cb); button->argument(5); void my_cb(Fl_Widget *w, void *v) { int val = (int)v; // do something with val } In our current implementation `val' will always be 5, no matter what the endianness of the processor is. There is no undefined data at all. If the button callback is called with the patched code (Fl_Widget.H, line #851) we access undefined data. For such a call see Fl_Button.cxx, line #105. The casts make sure that we always store the whole `void *' item, and they also deal with proper sign extension and endianness, although there are warnings. If we changed anything using a union, this would definitely cause trouble, i.e. break existing programs. Even if it was only the example code above! What happens in this example ? (a) Little endian: the low order (first) part of the union is 5, the high order part is undefined. If it is 0, then everything is okay, but if not: trouble. Imagine what happens with negative values (the high order part would have to be -1 (all one's) to make it work on a little endian machine). (b) Big endian: the high order (first) part of the union is 5, the low order part is undefined. Trouble is guaranteed! I suggest to add more casts or leave this as-is for FLTK 1.3.x. I can propose another solution for FLTK 1.4, but this will need user code changes, although it will be widely source compatible. I'll try to file an STR with a proposal over the next weekend if time allows.

#6	manolo 23:53 Apr 02, 2015

Thanks Albrecht for this detailed analysis showing how casts between pointer and integral values can be tricky. union.patch was conceived under the assumption that if a user calls Fl_Widget::argument(long), (s)he would also use a callback of type Fl_Callback1, that is, with a long as 2nd argument. I believe the analysis above shows, and have checked with various architectures, that the patched FLTK runs well in this case. I have also considered this usage-case: void cb(Fl_Widget *w, void *data) { int val = (long)data; } int main(int argc, char **argv) { Fl_Box *b = new Fl_Box(0,0,10,10); b->callback(cb); b->argument(-5); b->do_callback(); } Remarkably, with gcc for 64-bit MSWindows, it fails at compile-time with the error that a pointer-to-long cast is not allowed. I don't know if another compiler would accept this cast. A double cast is needed : void cb(Fl_Widget *w, void *data) { long val = (long)(fl_intptr_t)data; } to remove the compile-time error. This is also what is done at line #603 of Fl_Widget.H, for the same reason. I haved checked that, under these conditions, union.patch runs OK: - under 64-bit MSWindows (I am not aware of another architecture where the long and void* types differ in length) - gcc compiler - transmit a long value to a callback function with Fl_Widget::argument(long) - don't use the adequate Fl_Callback1 type for this function, but use Fl_Callback instead - perform a double cast in the callback function to recover the long value I will check a similar situation with the opposite endianness (emulating it with types void*/short under Apple ppc) and see what happens.

#7	manolo 07:49 Apr 03, 2015

With the compiler option -fpermissive it's possible to compile void cb(Fl_Widget *w, void *data) { long val = (long)data; } with just a warning. The union.patch works OK under 64-bit MSWindows in this case also. I tried with and without setting the bits of the pointer outside the integer zone, with positive and negative integer values.

#8	manolo 09:28 Apr 03, 2015

With the big-endian architecture, I have checked that the union.patch approach always fails, with callback functions of both types Fl_Callback and Fl_Callback1. A more complex layout is compatible with both big-endian and little-endian architectures: union { void *user_data_; struct { SPACER long user_value_; }; }; where SPACER is for now the empty string #define SPACER but would have to be defined to a concrete type for a hypothetical big-endian platform in which the long type is smaller than void* My conclusions for now: 1) the union.patch approach would work with the platforms that FLTK currently supports, 2) but it would dramatically fail on a hypothetical bigendian platform where the long type is smaller than a pointer, 3) unless the more complex layout shown above in this post is used. But there may be another solution somewhere else?

#9	AlbrechtS 13:01 Apr 03, 2015

(Note: this was intended to be posted hours ago, but somehow hid in my browser's tabs. Continuing anyway...) I believe the double casts are necessary and probably the only possible solution that lets us eliminate the warnings and keep the API unchanged. It gets tricky if another compiler doesn't "like" these double casts, maybe for different reasons. I can't imagine such a case though. Meanwhile I fixed several casting issues from Csaba's long patch in STR #2813. There are still some (3) open patches, but they are unrelated to our problem here. I consider patch file #9 of STR #2813 (the extract of Csaba's patch) as final and good to go. It fixes all issues with (double) casts where needed. This is IMHO a better solution for the next patch release (1.3.4) since it doesn't change internal structures. As Manolo wrote, similar double casts have already been used elsewhere in the FLTK code. http://www.fltk.org/strfiles/2813/callback.patch If you agree we could commit "callback.patch" of STR #2813 and close all related STR's as resolved (except that we should take care of the three open patches in STR #2813 before closing it finally). Note: I started a thread in fltk.coredev about a proposal of a new design with small API changes intended to be used in the next minor release (1.4.0) with the subject: RFC: Fl_User_Data in FLTK 1.4 ("the future")

#10	manolo 01:02 Apr 04, 2015

The callback.patch at STR#2813 is a perfect solution for this STR.