ghaerr / microwindows

The Nano-X Window System
Other
659 stars 90 forks source link

Make libraries compilable again #21

Closed roozbehid closed 5 years ago

roozbehid commented 5 years ago

This will make visual studio port compilable again. each commit description should tell the reason of why.

ghaerr commented 5 years ago

Thanks for your changes. I will merge your entire pull request, then make a few modifications and recommit. I would also like to mention a few thoughts:

1) I ported the system to Windows last weekend, using an old VS 2008 compiler (_MSC_VER==1500). I was able to get the Win32 API ported using a mechanism different that you're using, which manages to link both Microwindows Win32 API routines and the real Win32 API routines into the same program without having to use DLLs. The mechanism used is that the only drivers\scr/kbd/mou_win32.c use the real windows.h, and thus has WINAPI defined as __stdcall to call the real win32 routines. All the rest of Microwindows is compiled seperately using the internal windows.h WINAPI defined as blank, which defaults to _cdecl. Thus, the internal Win32 routines are name mangled as _Sleep, say, and the external real Win32 routines are called as Sleep$4 (parameter size mangled). The only restriction is that we can't have the same-named routines in the same source file.

2) Since you're building DLLs rather than linking statically, you require declspec(dllimport). This also happens to use stdcall, which means the above name-mangling distinction isn't being taken advantage of. That's ok, so I am going to use "#if _MSC_VER == 1500" in order to make the distinction between our dual use of the MS C compiler. FYI. You may want to consider not using the same names as the real Win32 routines for simplicity, later.

3) The reason why MwDelay() was removed from nanox/srvmain.c is for the reason above, which is that we can't have the real win32 Sleep() called from internal routine, so the entire routine was moved to drivers\scr_win32.c as discussed above. I'm currently thinking of some implementation ideas to get rid of system-dependent stuff in the engine/, nanox/ and mwin/ files, and move them into operating system-depending driver/ files, so you can expect more changes here in the future.

4) Lets have you use "#if _MSC_VER > 1500" for your future changes, to differentiate between my port (using "#if _MSC_VER == 1500" and your Win32 port for the time being, e.g. just like you did for winextra/gettimeofday.c.

5) I'm a little worried about whether the EPRINTF ##__VA_ARGS__ will work, but will add it for now. If it doesn't work, I will have to revert it and I suggest that you set HAVE_FPRINTF=0 like I did for my win32 port, which forces GdError() to be called directly instead. Far better, really. The only problem is that I had to move GdError() into drivers\scr_win32.c since it calls OutputDebugString() to print all debug messages directly to the VS output window. If you want to go this route, you can copy GdError over to winextra/error.c and include that in your compile. I also plan on fixing this problem and moving nanox/ and engine/ GdError() into an operating system dependent drivers/ file in the future as well.

6) Your directory.build.props file uses the define _CRT_SECURE_NO_WARNINGSl; do you really mean to have the "l" at the end of that?

7) Did you happen to notice that I added an include/mwconfig.h, which allows you to set all options in a single file, rather than in the VS project? You're welcome to add a section "#if WINEXTRA" at the top so that it is easier to track what defines you need set, rather than in your project file. I also listed all options and explain them to make porting easier. You can also see my Windows compile settings to compare with yours.

Thanks again for your interest and changes. Sorry I've had to drag you through this a couple times with my changes, your interest peaked my interest in getting Microwindows to compile and run natively on Windows, which now works.

roozbehid commented 5 years ago

Great, thanks.

So just to explain what I am doing -which maybe you are already fully aware of and what I kind of care for my own project is- I am using Visual Studio as I feel very comfortable with it. My goal is to create WebAssembly port of Microwindows for my use in C# .Net, but because I am doing development and debugging in Windows, and due to way things are now with toolings which would take around 2-3 minute to compile everything for WebAssembly, debuging/compiling is way faster on Windows -ie platform windows-. So :

This way I have similar things, and I am using Microwindows to fill all my Win32 APIs. For building them for WebAssembly I created another project Dotnet-Vcxproj which will use same visual studio projects but will use Emscripton (or any compiler) to do the job! So as you can see I am too much addicted to VisualStudio :D

Just telling you this that I don't use mou_win32 and similar, as that way my code will diverge -more- between WebAssembly and Windows which I don't want. So in the end I ship a nuget package to people using it, simply building their project in VS, they can run it in windows and interact with it and debug it, or they will get the artifacts for WebAssembly in their binary output folder and they can also run it in browser. Debugging is still being worked out for WebAssembly and C#, so this approach is easier for them.

  1. Makes sense. I think I can do the same thing but it needs some time to go through different projects and fix the calling convention.

  2. Ok

  3. Ok

  4. Ok

  5. From what I read and did a research ##VA_ARGS should be the correct way as it is in C99. The other way you used was GCC specific. This discussion for example says __VA_ARGS__ is correct way and also mentioned that g++ also seems to not support args... . Anyway it seems I hit another bug in MSVC with #ifdefs inside _VAARGS, so removing those ifdefs would make it compile fine.

  6. Ooops

  7. No,totally missed it. But because as I am going to use different project settings to compile for example for WebAssembly, StaticLinking or Win32, maybe I put everything in project files. I have to think which one makes it more clear. Not yet sure what do to here....

ghaerr commented 5 years ago

Thanks for your long explanation. Now I more fully understand that really the goal is for you to be able to use the Visual Studio/Microsoft development environment to port and compile Microwindows for WebAssembly. Cool.

Now that I have (an old) version of VS 2008 running, I hope to make this easier for you. And no problem that you won’t be using scr_win32.c, I’m glad you’ve got everything running on top of SDL 2 on Windows.

I’ll take a look at your Dotnet-Vcxproj, I’ve been wondering what that looks like.

So, here is what I think I’ll do:

  1. After looking hard at this whole __VA_ARGS__ thing, I think you’re right, it is the proper way to do things. Except that I want to be able to keep some

    ifdef’s in EPRINTF() expansions, which helps for font debugging. I think it

    will be better just to remove the whole varargs macro expansion of EPRINTF to fprintf, and have it always just call GdError, that is: #define EPRINTF GdError, and not mess with converting it to fprintf(stderr, …) [which doesn’t work on embedded systems anyway]. My whole approach is outdated and overly complicated, and there are tons of other fprintf() calls in demos that need to be revised.

I’ll take care of all this and keep your build working.

  1. I will create a new drivers\os_win32.c which contains Win32 operating specific things, including gettimeofday(), GdError (possibly renamed but working for your port in the case that you want to see internal debug messages in VS Output window when debugging your code), MwDelay, (so that we can remove the declspec(dllimport) stdcall SleepEx hack to get things to compile), and possibly other OS functions.

In this way, you would just add another file to your project, drivers/os_win32.c, and all the special Win32 functions will be in there. The only catch is that that file will have to include the “real” windows.h, so you may have to compile this file using a new project in VS, since your existing ones path microwindows/src/include, which will get the wrong windows.h. For ports to other systems, like EMSCRIPTEN, there will be a different os*.c file. I need to think about exactly how this will work without creating too much messiness.

Ultimately, you may have to re-implement just these functions in os_win32.c for your WebAssembly port that doesn’t use Windows, but at least they’ll only be in one file. Will this work?

For item 1), don’t worry about the WINAPI change at this point, I just wanted to let you know there may be an easier way.

For item 6), agreed it may be easier to keep WebAssembly, Staticlink and Win32 options seperately out of mwconfig.h, just wanted you to see that it is there. We don’t really want custom options in there, but I’m also interested in the kinds of settings you’re having to use for your compilations, so that I might make them easier to set. Another option would be to use #include MWCONFIG in device.h, where MWCONFIG is defined in your project settings to include a specific config file rather than hacking include/mwconfig.h.

Later this afternoon I’ll accept your pull request and start making other changes.

Thank you for your contributions!

On Feb 21, 2019, at 1:27 PM, roozbehid notifications@github.com wrote:

Great, thanks.

So just to explain what I am doing -which maybe you are already fully aware of and what I kind of care for my own project is- I am using Visual Studio as I feel very comfortable with it. My goal is to create WebAssembly port of Microwindows for my use in C# .Net, but because I am doing development and debugging in Windows, and due to way things are now with toolings which would take around 2-3 minute to compile everything for WebAssembly, debuging/compiling is way faster on Windows -ie platform windows-. So :

I am building everything with SDL2 to run on Windows. I am building everything with SDL2 to run on Webassembly. This way I have similar things, and I am using Microwindows to fill all my Win32 APIs. For building them for WebAssembly I created another project Dotnet-Vcxproj https://github.com/roozbehid/dotnet-vcxproj which will use same visual studio projects but will use Emscripton (or any compiler) to do the job! So as you can see I am too much addicted to VisualStudio :D

Just telling you this that I don't use mou_win32 and similar, as that way my code will diverge -more- between WebAssembly and Windows which I don't want. So in the end I ship a nuget package to people using it, simply building their project in VS, they can run it in windows and interact with it and debug it, or they will get the artifacts for WebAssembly in their binary output folder and they can also run it in browser. Debugging is still being worked out for WebAssembly and C#, so this approach is easier for them.

Makes sense. I think I can do the same thing but it needs some time to go through different projects and fix the calling convention.

Ok

Ok

Ok

From what I read and did a research ##VA_ARGS should be the correct way as it is in C99. The other way you used was GCC specific. This discussion https://stackoverflow.com/questions/679979/how-to-make-a-variadic-macro-variable-number-of-arguments for example says VA_ARGS is correct way and also mentioned that g++ also seems to not support args... . Anyway it seems I hit another bug in MSVC with #ifdefs inside VA_ARGS, so removing those ifdefs would make it compile fine.

Ooops

No,totally missed it. But because as I am going to use different project settings to compile for example for WebAssembly, StaticLinking or Win32, maybe I put everything in project files. I have to think which one makes it more clear. Not yet sure what do to here....

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ghaerr/microwindows/pull/21#issuecomment-466153634, or mute the thread https://github.com/notifications/unsubscribe-auth/ALbi5djGpyIGeqSTUploI6nCzabkTIxqks5vPwEpgaJpZM4bGuW1.

ghaerr commented 5 years ago

Ok, I removed the whole varargs macro expansion of EPRINTF to fprintf(stderr, ...), so we shouldn't have to #ifndef _MSC_VER in srvmain.c and devfont.c.

All OS dependent stuff was moved out of various files (srvmain.c, winmain.c, winextra/gettimeofday.c, error.c) into a single drivers/osdep.c. You will need to add this file back into your drivers project and things should compile. However, if you create a new project with just this file, and do NOT include auto directory search of microwindows/src/include (but instead let it include the REAL windows.h file), then it will link to Sleep/GetSystemTime/SystemTimeToFile/OutputDebugString without requiring dllimport hacks like we had to do in the old winextra/extra_kernel.h and gettimeofday.c. You may have to delete the code within the "#if _MSC_VER > 1500" in osdep.c when you do this. Take a look at the file and hopefully you'll see what I mean.

In the longer term, having this single file with the OS dependencies and having it include the real OS header files will work well if we have to add more functions in the future. I've tested it with my scr_win32.c build directly on windows and it works. You may have to push a few changes to osdep.c and your project files after this is done and we should be on our way to project compatibility.

Thanks!

roozbehid commented 5 years ago

So I was trying to build the most recent version and I understand your trick with stdcall and _ name mangling but it seems it wont work for me. I want to have WINAPI to be dllexports which are stdcall for MWin. Then I also have to import Sleep from Kernel. How should I use it now? I am getting redifintion of Sleep in osdep.c

Maybe change back to SleepEx ?

ghaerr commented 5 years ago

Ok on not being able to use the stdcall trick…

Not sure I understand why Sleep is getting redefined, since we’re not defining Sleep in osdep.c, but just calling it. But sure, try changing back to SleepEx in osdep.c. Hopefully that will work since I removed the extra_kernel.h definition.

On Feb 21, 2019, at 11:55 PM, roozbehid notifications@github.com wrote:

So I was trying to build the most recent version and I understand your trick with stdcall and _ name mangling but it seems it wont work for me. I want to have WINAPI to be dllexports which are stdcall for MWin. Then I also have to import Sleep from Kernel. How should I use it now? I am getting redifintion of Sleep in osdep.c

Maybe change back to SleepEx ?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/ghaerr/microwindows/pull/21#issuecomment-466294055, or mute the thread https://github.com/notifications/unsubscribe-auth/ALbi5cNX203OozoCD0w2-2I_n53Re_Ydks5vP5R7gaJpZM4bGuW1.

ghaerr commented 5 years ago

Remember that you would have to change include/windef.h and not #define WINAPI to be both dllexport and __stdcall, you would only want it to be dllexport and _cdecl. But I suppose if you are including the real Windows.h in your other compilations for .Net, then this won;’t work because they are expecting __stdcall imports and don’t know about the Microwindows trickery.

I still don't understand the Sleep redefinition, unless I typed it in incorrectly in osdep.c. Are you sure you are NOT searching the Microwindows src/include directory when compiling osdep,c? That would cause this error possibly.

On Feb 21, 2019, at 11:55 PM, roozbehid notifications@github.com wrote:

So I was trying to build the most recent version and I understand your trick with stdcall and _ name mangling but it seems it wont work for me. I want to have WINAPI to be dllexports which are stdcall for MWin. Then I also have to import Sleep from Kernel. How should I use it now? I am getting redifintion of Sleep in osdep.c

Maybe change back to SleepEx ?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/ghaerr/microwindows/pull/21#issuecomment-466294055, or mute the thread https://github.com/notifications/unsubscribe-auth/ALbi5cNX203OozoCD0w2-2I_n53Re_Ydks5vP5R7gaJpZM4bGuW1.

roozbehid commented 5 years ago

Ok so I am also confused! But it seems problem is now with VOID WINAPI Sleep(DWORD dwMilliseconds);

so this will create _Sleep@4 and also I am importing same _Sleep@4 from kernel32.lib

roozbehid commented 5 years ago

Seems to be solved by removing WINAPI from Sleep. Which kind of make sense to me. I mean if somebody calls Sleep externally probably want Sleep which is already defined for it. So I think it should not be exported anyway.

ghaerr commented 5 years ago

Glad you have it running. Not really sure what the deal is with Sleep, but I see in mwin/winmain,c it is also commented out for EMSCRIPTEN for a reason I can’t figure out either.

I don’t plan on making more major changes that will affect your work so much.

Hopefully this is about it, send me another pull requests for your mods and I’ll add it and you can go back to getting the rest of your project moving forward!

On Feb 22, 2019, at 12:14 AM, roozbehid notifications@github.com wrote:

Seems to be solved by removing WINAPI from Sleep. Which kind of make sense to me. I mean if somebody calls Sleep externally probably want Sleep which is already defined for it. So I think it should not be exported anyway.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/ghaerr/microwindows/pull/21#issuecomment-466297889, or mute the thread https://github.com/notifications/unsubscribe-auth/ALbi5dqOfEnKe2sqsbZOHHrvzKT8RP39ks5vP5jdgaJpZM4bGuW1.

ghaerr commented 5 years ago

There is one reason I can think of that this all might be happening: that is that we should NOT be using the dllimport definitions that are in osdep,c, including the one for OutputDebugString. These definitions are only needed for _MSC_VER > 1500 AND if you are NOT including the REAL windows.h and not searching src/include with your Visual Studio. This can only be fixed if you compile osdep.c using a separate project, not part of the regular drivers/*.c compile.

Since Sleep is defined internally as well as in the dllimport in osdep.c, it gets an error, whereas no other routines have this problem.

In any case, I’ll accept whichever fix you send me, but it would be nice to remove the #if _MSC_VER > 1500 code if possible. If not, then leave it in.

On Feb 22, 2019, at 12:19 AM, Greg Haerr greg.haerr@censoft.com wrote:

Glad you have it running. Not really sure what the deal is with Sleep, but I see in mwin/winmain,c it is also commented out for EMSCRIPTEN for a reason I can’t figure out either.

I don’t plan on making more major changes that will affect your work so much.

Hopefully this is about it, send me another pull requests for your mods and I’ll add it and you can go back to getting the rest of your project moving forward!

On Feb 22, 2019, at 12:14 AM, roozbehid <notifications@github.com mailto:notifications@github.com> wrote:

Seems to be solved by removing WINAPI from Sleep. Which kind of make sense to me. I mean if somebody calls Sleep externally probably want Sleep which is already defined for it. So I think it should not be exported anyway.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/ghaerr/microwindows/pull/21#issuecomment-466297889, or mute the thread https://github.com/notifications/unsubscribe-auth/ALbi5dqOfEnKe2sqsbZOHHrvzKT8RP39ks5vP5jdgaJpZM4bGuW1.

ghaerr commented 5 years ago

Ignore my last comment… I think the problem is what you originally described, and simply solved: we are declaring WINAPI Sleep in winmain.c and also importing it in osdep.c.

The fix should be that you leave WINAPI Sleep in winmain,c, and revert back to using SleepEx() in osdep.c. That should solve the problem completely, and leave Sleep() available in Microwindows or your DLL should a program need it.

You don’t need to compile osdep.c separately, but if you do, it would allow us to remove the dllimport definitions because you would be reading in the real windows.h.

On Feb 22, 2019, at 12:25 AM, Greg Haerr greg.haerr@censoft.com wrote:

There is one reason I can think of that this all might be happening: that is that we should NOT be using the dllimport definitions that are in osdep,c, including the one for OutputDebugString. These definitions are only needed for _MSC_VER > 1500 AND if you are NOT including the REAL windows.h and not searching src/include with your Visual Studio. This can only be fixed if you compile osdep.c using a separate project, not part of the regular drivers/*.c compile.

Since Sleep is defined internally as well as in the dllimport in osdep.c, it gets an error, whereas no other routines have this problem.

In any case, I’ll accept whichever fix you send me, but it would be nice to remove the #if _MSC_VER > 1500 code if possible. If not, then leave it in.

On Feb 22, 2019, at 12:19 AM, Greg Haerr <greg.haerr@censoft.com mailto:greg.haerr@censoft.com> wrote:

Glad you have it running. Not really sure what the deal is with Sleep, but I see in mwin/winmain,c it is also commented out for EMSCRIPTEN for a reason I can’t figure out either.

I don’t plan on making more major changes that will affect your work so much.

Hopefully this is about it, send me another pull requests for your mods and I’ll add it and you can go back to getting the rest of your project moving forward!

On Feb 22, 2019, at 12:14 AM, roozbehid <notifications@github.com mailto:notifications@github.com> wrote:

Seems to be solved by removing WINAPI from Sleep. Which kind of make sense to me. I mean if somebody calls Sleep externally probably want Sleep which is already defined for it. So I think it should not be exported anyway.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/ghaerr/microwindows/pull/21#issuecomment-466297889, or mute the thread https://github.com/notifications/unsubscribe-auth/ALbi5dqOfEnKe2sqsbZOHHrvzKT8RP39ks5vP5jdgaJpZM4bGuW1.