| | [ Return to Bugs & Features | Roadmap 1.3 | SVN ⇄ GIT ]
STR #2308
| Application: | FLTK Library |
|---|
| Status: | 1 - Closed w/Resolution |
|---|
| Priority: | 3 - Moderate, e.g. unable to compile the software |
|---|
| Scope: | 2 - Specific to an operating system |
|---|
| Subsystem: | Core Library |
|---|
| Summary: | Compiling FLTK 1.3 with mingw-w64 |
|---|
| Version: | 1.3.0 |
|---|
| Created By: | odinsbane |
|---|
| Assigned To: | AlbrechtS |
|---|
| Fix Version: | 1.3.0 (SVN: v7982) |
|---|
| Update Notification: | |
|---|
Trouble Report Files:
Trouble Report Comments:
| |
| #1 | odinsbane
05:44 Jan 22, 2010 |
| I compiled FLTK with mingw-w64 and I found a couple errors that needed to be fixed. There were 3 errors one of them was repeated.
i) casting (void*) to long -> fixed by usings <stdint.h> and intptr_t
ii) casting HINSTANCE to int -> fixed by using INT_PTR
iii) linking wsock32 fixed by using ws2_32
Here is a diff file of two fltk-1.10 directories after an auto configure with cygwin. ( I manually removed the .o file references since the -x ".o" wouldn't ignore them for some reason.)
mbs | |
| |
| #2 | d.zimmer
18:08 Jan 24, 2010 |
| Your fix #1 will change the API, so it might be ok for 1.3 but not 1.1
For many compilers (eg. VC), this is just a warning, not an error. It think can be silenced by:
- long argument() const {return (long)user_data_;}
+ long argument() const {return (long)(intptr_t)user_data_;}
If you are using gcc, I think it can be downgraded to a warning using -fpermissive
Don. | |
| |
| #3 | odinsbane
04:30 Jan 26, 2010 |
| Yes the other way works fine, casting to intptr_t then to long. I'm working (very slowly) on compiling 1.3 but it has the some of the same casting issues. (the guys at the #mingw-w64 say its one of the most common issues right now.)
thanks
mbs | |
| |
| #4 | matt
10:58 Jan 31, 2010 |
| FLTK 1.1 does not officially support 64 bit compilation. It works in pretty much all cases anyway, but we decided not to modify the code anymore. FLTK 1.3 should compile 64 bit, though. | |
| |
| #5 | matt
02:03 Apr 05, 2010 |
| Is this still an issue? Is intptr_t a good solution? | |
| |
| #6 | odinsbane
13:29 Apr 05, 2010 |
| I moved to 1.3 and I had to go through and make the changes, which seemed to work though it was a bit troublesome. The suggestion by don worked, by casting to an intptr_t then to a long. I suspect using the intptr_t seems to offer better 32bit/64bit support, but really I don't know, I just made a small interface.
Thanks | |
| |
| #7 | HenryN
16:56 Jun 11, 2010 |
| Here is a patch that is generally a better choice as the current pointer cast:
===================================================================
--- src/fl_open_uri.cxx (revision 7637)
+++ src/fl_open_uri.cxx (working copy)
@@ -114,7 +114,7 @@
#ifdef WIN32
if (msg) snprintf(msg, msglen, "open %s", uri);
- return (int)ShellExecute(HWND_DESKTOP, "open", uri, NULL, NULL, SW_SHOW) > 32;
+ return (int)(ShellExecute(HWND_DESKTOP, "open", uri, NULL, NULL, SW_SHOW) > (void*)32);
#elif defined(__APPLE__)
char *argv[3]; // Command-line arguments | |
| |
| #8 | yuri
07:10 Oct 01, 2010 |
| hm. intptr_t works, but :
void cb_smth(Fl_Widget*,long);
Fl_Menu_Item itm[]={
...
{"abra",0,(Fl_Callback*)cb_smth,1},
...
cause an error in conversion from void (*)(Fl_Widget*,long int) to void (*)(Fl_Widget*,void*)
it seems that we must use _int64 instead of long on WIN64 platforms
because sizeof(long)==4 but sizeof(void)==8. | |
| |
| #9 | yuri
07:38 Oct 01, 2010 |
| we must change long to intptr_t in all callbacks functions. | |
| |
| #10 | AlbrechtS
05:07 Oct 02, 2010 |
| intptr_t seems to be defined in C99 [1] and in c++0x [2], but both definitions are *optional*. Citation from [1]:
7.18 Integer types <stdint.h>
...
7.18.1.4 Integer types capable of holding object pointers
The following type designates a signed integer type with the property that any valid pointer to void can be converted to this type, then converted back to pointer to void, and the result will compare equal to the original pointer:
intptr_t
[... similar text for uintptr_t follows ...]
These types are optional.
-- end of citation --
I conclude that we would need a configure test and an own typedef that would fall back to something like "long" if intptr_t is not defined, *even* if we agree that we can use at least the C99 standard.
Can we assume that we will find intptr_t on all current 64-bit systems? If yes, then we could use the above mentioned configure test and typedef and use it wherever applicable, but if not... I don't know.
Note: In c++0x these type are defined in the header <cstdint> with namespace std::, but I believe that c++0x is not yet released (is it?).
---
Drafts of standards:
[1] http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf
[2] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3126.pdf | |
| |
| #11 | greg.ercolano
19:30 Oct 03, 2010 |
| A few suggs:
In cases where sizeof(void*) >= sizeof(long), then I we can surely just use eg. memcpy() or whatever to save/retrieve longs from the void* 'data buffer' safely.
In cases where sizeof(void*) < sizeof(long), then I think the simple
solution is to have an #ifdef that simply adds a long variable called 'argument_' to Fl_Widget, and have argument() manipulate that. | |
| |
| #12 | yuri
07:51 Oct 04, 2010 |
| one of the possible way to solve the problem define fl_long type.
see patch.
from my point of view it's the simplest solution of this problem | |
| |
| #13 | AlbrechtS
07:57 Oct 04, 2010 |
| Whatever we do internally to store the value, please remember that the callback will always be called with only *one* calling convention: using
callback_(Fl_Widget *,void *)
Storing anything in a union (as suggested elsewhere) doesn't help much either, because we can use only one of the union's members for calling the callback. | |
| |
| #14 | greg.ercolano
12:51 Oct 04, 2010 |
| Possibly the sugg of using a union was for conversion from
long <-> void* only in the argument() method, eg:
#include <stdio.h>
class Fl_Widget {
void *userdata;
public:
Fl_Widget() { userdata = 0; }
void argument(long val) {
union { void *v; long l; } vl;
vl.l = val; // convert long..
userdata = vl.v; // ..to void*
}
long argument() const {
union { void *v; long l; } vl;
vl.v = userdata; // convert void*..
return(vl.l); // ..to long.
}
};
int main() {
Fl_Widget w;
w.argument(305419896L); // aka. 0x12345678
printf("Arg=%ld (%lx)\n", w.argument(), w.argument());
printf("sizeof(void*)=%lu\n", sizeof(void*));
printf(" sizeof(long)=%lu\n", sizeof(long));
return(0);
}
Even if sizeof(long) < sizeof(void*) that should work.
(I tried above technique with a char <-> void* instead of long
to simulate that condition, and seemed to compile/run OK on linux
and windows) | |
| |
| #15 | AlbrechtS
01:15 Oct 05, 2010 |
| @Greg: Yes, of course your code version works, because you are always accessing (reading) the union in the same way as you stored the value. But you're missing the point that I wrote above: the callback *always* uses the full user_data_ value and calls the callback function with this as the second argument. If sizeof(long) < sizeof(void*) in your code, then the extraneous part in the void* part of the union is undefined. Accessing it via the argument() method is okay, but if we would call the callback via callback_(Fl_Widget *, user_data_), then that undefined part would be used in calling the callback. This *might* be okay as long as the callback is defined as my_callback(Fl_Widget*, long), but what if it is defined as my_callback(Fl_Widget*,void *v), and the code casts the given argument like "long my_arg = (long)v" ? What if the same is done on a big-endian and on a little-endian machine?
Please understand me right: I don't want to say that it wouldn't work in some or all cases as expected, but we would enter the gray zone of undefined (maybe implementation-dependent) behaviour of the code. At least... | |
| |
| #16 | AlbrechtS
01:26 Oct 05, 2010 |
| @Yuri: the "fl_long" patch looks much better to me than the previous one. Of course, the windres change in test/Makefile can't be done this way, but most of the other code looks good to me.
However, this *does* change the API for Win64 targets, since then all callback-related functions that used and expected a long now would use an intptr_t type, which differs in size from a long on a Win64 system. This changes the documented interface.
This would only concern users who port to Win64, but OTOH these users might have portability issues if they also want to use the code on other platforms. Maybe...
If we would go this way, we'd at least have to document this change thoroughly, but then this _could_ be one way to go, IMHO. Maybe the cleanest.
Other opinions? | |
| |
| #17 | yuri
02:32 Oct 05, 2010 |
| > the windres change in test/Makefile can't be done this way
hm. it's only do for cross-compiling. i forgot remove it from this patch
> this *does* change the API for Win64 targets ...
this only need to rebuild projects.
but intptr_t have the same size as void* and porting to win64 from other platforms will require only replace long to fl_long (this need to be documented)
the similar way is used at others toolkits gtk/glib or qt introdusing its own types. | |
| |
| #18 | AlbrechtS
01:48 Nov 19, 2010 |
| I'm working on a patch now, based on Yuri's idea using an own typedef
(fl_long), but I'm also trying to avoid changing the API, so that
Fl_Widget::argument() still returns long. I have a working proof of
concept, but this still needs some cleanup. I'll provide a patch shortly.
I also changed the title (summary) to reflect FLTK 1.3 instead of FLTK 1.1.10, because this is not going to be implemented in FLTK 1.1. | |
| |
| #19 | yuri
09:26 Nov 19, 2010 |
| i think there is no reason leave return type of argument to be long.
it may cause value lost of value in some future codes.
if we decide use typedef fl_long we must use it everywhere to avoid future errors.
P.S. I think that on 'normal' systems this will only be a 'cosmetic' changes and we only must recomended use of fl_long in fltk manual (and use in in all examples of course). | |
| |
| #20 | matt
13:35 Nov 27, 2010 |
| Albrecht, you said: This *might* be okay as long as the callback is defined as my_callback(Fl_Widget*, long), but what if it is defined as my_callback(Fl_Widget*,void *v), and the code casts the given argument like "long my_arg = (long)v" ? What if the same is done on a big-endian and on a little-endian machine?
To answer your questions: nothing is going to happen. As long as I use "long" to read and write, and the "void*" is equal in size or bigger, the "junk" bits will be clipped by the casts, no matter if that will be the first bits or last bits (MSB, LSB). The "union" approach would also always work.
I feel like this is an overly protective default compiler option. The user should set this to a warning, if at all.
Oh, and there is a perfect solution, too: we can have two member variables, a pointer AND a long. But that needs more space and would be incompatible to existing code. | |
| |
| #21 | AlbrechtS
09:12 Nov 28, 2010 |
| Matt, you wrote:
> To answer your questions: nothing is going to happen. As long as I
> use "long" to read and write, and the "void*" is equal in size or
> bigger, the "junk" bits will be clipped by the casts, no matter if
> that will be the first bits or last bits (MSB, LSB). The "union"
> approach would also always work.
I'm afraid that I disagree here. First, the point was that we were talking about the case that sizeof(void*) > sizeof(long). The case "=" is trivial. Second, I was writing about a case where you use the "long" union member to write and the "void *" union member to read the userdata field. The former is possible (and we can't prevent it) by the user calling widget->argument(int), and the latter is the way a callback is called by FLTK. Then, on a big endian system, writing the long union member would write the first 4 bytes only (the following 4 bytes are undefined). When FLTK calls the callback, it uses the "void *" member variable, and this means that the "LSB" will be the last byte (big endian), and thus the last 4 bytes will become the low order 4 bytes of the void * pointer. Casting this to a long in the callback will result in 4 undefined bytes.
Strictly spoken: a union's value is undefined, if you use another member variable to read than you used to write, and we wouldn't be able to control this because of the different calls (argument() and userdata()) that are available to set the value.
Fortunately, this would probably be moot, since the only systems with sizeof(long) < sizeof(void *) are Windows systems, and AFAIK Windows doesn't work on big endian machines. ;-)
But anyway, I recommend not to use a union for the given reasons.
---
Matt, you also wrote:
> Oh, and there is a perfect solution, too: we can have two member
> variables, a pointer AND a long. But that needs more space and would
> be incompatible to existing code.
We could do this from an ABI point of view now, but we would pose a new restriction on setting the userdata (aka. argument()) value and we would probably need to differentiate the callback signature we call depending on which way was used to set the userdata value. At least we would also need a flag that tells us which of the variables was set in the last call. If we do that, then we can also use a union, because we can then use the same member for reading that was used for writing. Or something similar. ... | |
| |
| #22 | AlbrechtS
09:35 Nov 28, 2010 |
| Yuri, you wrote:
> i think there is no reason leave return type of argument to be long.
I think that we should keep the API (literally) so that we keep the 'long' return type. This way most user's won't need to change any code or other data types.
> it may cause value lost of value in some future codes.
The only problem is with Windows 64bit systems, and users compiling code for these systems need to handle that "long/pointer incompatibility" anyway.
> if we decide use typedef fl_long we must use it everywhere to avoid
> future errors.
My intention is to have the least impact on application developers for the transition from FLTK 1.1 to 1.3. We decided that FLTK 1.3 should be API compatible as much as possible, so that FLTK 1.1 source code compiles well with FLTK 1.3. Again, this will not be given for users that also switch from 32-bit Windows to 64-bit Windows systems, but this is its own problem. Taking this as a reason to change the API for all users is not what I think is appropriate now.
> P.S. I think that on 'normal' systems this will only be a 'cosmetic'
> changes and we only must recomended use of fl_long in fltk manual (and
> use in in all examples of course).
It might be true that most of the code will compile anyway (maybe with warnings), but why would we want to change the API w/o need, if there's a way that we can fix the issue w/o doing so?
Having said that: yes, I agree that we should think about changing the callback related API and semantics in a future version of FLTK. But IMO this should not be FLTK 1.3. And if we do, then we should do it "The Right Way".
P.S. I hope that I can post my version of a patch during the next few hours... | |
| |
| #23 | AlbrechtS
15:21 Nov 28, 2010 |
| Please see attached file fl_intptr_t-r7907.diff for my modified patch. This does not change the API and uses a similar typedef as Yuri's patch. I decided to call it 'fl_intptr_t' instead of 'fl_long', because it's similar to intptr_t, but that's maybe a matter of taste. I used argument() instead of a cast, where applicable. Since this doesn't change the API, the effective patch is also much smaller.
I tested it on Windows (native and Cygwin/mingw-w64) and Ubuntu (32 and 64 bit version) successfully. There are two "FIXME's" included, because this may be changed, if we can find a better place for the typedef than in Fl_Widget.H.
Comments and test results welcome. | |
| |
| #24 | yuri
12:30 Dec 03, 2010 |
| fl_intptr_t is the better name then fl_long.
but i think that we can safely change all long to fl_intptr_t in callback related function. Old fltk-1.1 code will be compiled without problen on all suppoted platforms exept mingw64. | |
| |
| #25 | matt
10:47 Dec 06, 2010 |
| The patch looks ok to me. | |
| |
| #26 | AlbrechtS
10:58 Dec 06, 2010 |
| I just posted a request for comments in fltk.development regarding whether we should change the API (use Yuri's patch) or not (use my patch) to solve this issue. Please read this post and give your votes. I'd like to finalize this STR and apply one of the patches ASAP. Thanks.
My RFC in fltk.development: <http://www.fltk.org/newsgroups.php?gfltk.development+v:11075>
Full thread (search): <http://www.fltk.org/newsgroups.php?gfltk.development+Q%22RFC+-+STR+%232308+-+To+change+or+not+to+change+the+callback+API%22> | |
| |
| #27 | AlbrechtS
03:39 Dec 07, 2010 |
| We have 4 votes against changing the API - thanks to all that took the time to read and vote. I'll go ahead and finalize my patch and commit it when time permits. | |
| |
| #28 | AlbrechtS
04:46 Dec 08, 2010 |
| Thanks to HenryN for the suggested fix for src/fl_open_uri.cxx - this is really a better solution.
I committed this one separately in svn r7976, since it is an independent improvement. More to follow... | |
| |
| #29 | AlbrechtS
06:19 Dec 08, 2010 |
| Fixed in Subversion repository.
I committed the changes in svn r 7978 and hope that this fixes all warnings and errors. I tested on Windows and Linux, both 32-bit and 64-bit, and found no problems compiling and running (some) tests.
Please test and report any remaining problems. Thanks.
If there are no reports during the next few days, this STR will be closed.
There's a small remaining issue: the new typedef's fl_intptr_t and fl_uintptr_t should be document for Win64 users so that they know how they can do the necessary casts in their own software. I'll do this later, independent of the status of this STR. | |
| |
| #30 | yuri
12:12 Dec 08, 2010 |
| in branches/branch-1.3/test/line_style.cxx
(long)(choice[0]->mvalue()->argument())
... etc
casting seams unnecessary. | |
| |
| #31 | AlbrechtS
15:52 Dec 08, 2010 |
| Yes, that (unnecessary casting) is correct, thanks for pointing this out. I'll fix this... | |
| |
| #32 | AlbrechtS
16:12 Dec 08, 2010 |
| Done: svn r7982. | |
[ Return to Bugs & Features ]
|
| |
