Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xbox: Add includes/alloca.h #44

Open
wants to merge 1 commit into
base: nxdk
Choose a base branch
from

Conversation

Teufelchen1
Copy link

@Teufelchen1 Teufelchen1 commented Apr 14, 2021

This PR adds the file alloca.h which provides an implementation of alloca(size) according to the manpage.
Implementation might be a bit far fetched as it is just a #define to refer to the builtin.
Having this file simplifies porting of certain programs and libraries. i.e. Micropython. I currently have no way of running xbox executable but at least it compiles fine :p

For my understanding:

  1. The program includes <alloca.h> expecting to find an implementation of alloca(size)
  2. The program uses the function accordingly
  3. The (preprocessor/)compiler replaces all references to alloca(size) with its own builtin
  4. The compiler uses its builtin to compile the program.

This is done because alloca operates/allocates on the stack frame - which is handled by the compiler, not the OS(as given when using malloc). Additionally, it is therefore compiler(and machine) depended wich builtin/implementation is to be used.
Right? 😅

I'm not sure if the PR belongs here or if it is okay at all. So don't feel bad when giving hard backlash in the review.
Edit:
Maybe I should have done something similar to this and place it in nxdk/libs/xboxrt/alloca.h

Additionally, this is according to the manpage of alloca commonly found on modern *nix systems.

@@ -0,0 +1 @@
#define alloca(size) __builtin_alloca (size)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misses "header" with information about the purpose and License of this file.

@thrimbor
Copy link
Member

alloca is not part of the C standard, so if we want to add this, it should be in the nxdk repo instead.
I'm not sure whether we should add the header at all - it's a POSIX thing, and the Xbox isn't a POSIX platform. Micropython seems to already have Window support, so I expect it to already handle those platform differences.

@Teufelchen1
Copy link
Author

alloca is not part of the C standard, so if we want to add this, it should be in the nxdk repo instead.

Meh, I feared that could be the case.

I'm not sure whether we should add the header at all - it's a POSIX thing, and the Xbox isn't a POSIX platform.

It's not a POSIX thing, at least according to the C POSIX library.
The manpages states that as well, hence my "alloca commonly found on modern *nix systems.". I should have been more verbose there, sorry.

Micropython seems to already have Window support, so I expect it to already handle those platform differences.

In the Windows port, they include <malloc.h> - Which would also have been valid solution in this case[micropython -> xbox], as is defines the macro as well. As seen here. Beside, micropython was just an example.


Why not support some POSIX on the xbox as well?

[..] and the Xbox isn't a POSIX platform

Thats the current situation, however that shouldn't stop us from discussing about changing that 🙂.

@JayFoxRox
Copy link
Member

Full disclosure: I originally asked for alloca.h; but the existing review comments reminded me of why I didn't do it and why it might not be a good idea.

alloca is not part of the C standard, so if we want to add this, it should be in the nxdk repo instead.

Agreed; GNU libc ships it, but also a lot of other POSIX platforms - but it's not part of C.

I'm not sure whether we should add the header at all - it's a POSIX thing, and the Xbox isn't a POSIX platform. Micropython seems to already have Window support, so I expect it to already handle those platform differences.

Agreed, micropython is a bad example, because it doesn't work out of the box anyway (it is meant to require porting).
The nxdk winapi should be improved so the Windows version works out of the box, or upstream should be modified to be more modular, so a subset of supported functions can be used.

However, there are still other libs which probably need "alloca.h". I ran into it a couple of times.

Instead of creating "alloca.h", it probably does make more sense to change those projects to make them compiler aware, so they can use the compilers alloca.
General philosophy should be:

  • Porting to Xbox = bad
  • Porting to Clang / Windows = good.

On the other hand, you might also conclude that "alloca.h" has become some defacto-standard for exposing this feature of compilers. Does Clang maybe even expose "alloca.h" on Windows as part of it's GNU C support?

Why not support some POSIX on the xbox as well?

[..] and the Xbox isn't a POSIX platform

Thats the current situation, however that shouldn't stop us from discussing about changing that slightly_smiling_face.

I also share that sentiment. nxdk is low level first (custom drivers + NT kernel), winapi very close second, SDL2 third, but GNU C / POSIX soon after that.

So, because nxdk is fundamentally heading towards becoming a winapi platform (which has some benefits but also some issues) in userspace-ish sections, I think that it makes most sense to implement POSIX standards on top of existing winapi (like cygwin / mingw).

So just creating a alloca-win32 (alloca.h which uses win32 _alloca) or ensure-alloca / portable-alloca (provide alloca.h which detects compiler or uses #include_next <alloca.h> fallback) library might be better.

Ideally that shouldn't be Xbox specific and modular, so we have more stakeholders. We should then try to get people to port their projects with those libraries while they don't have explicit Windows support.

So either a port will boil down to adding a library inclusion upstream (add Windows support to their POSIX code using alloca-win32), or a wrapper around our build-system which includes those wrappers (add POSIX support on top of nxdk winapi to the build-environment).

Some projects (and requests) already exist to implement some POSIX stuff on Windows.
Also some POSIX libraries are almost part of Windows, like https://docs.microsoft.com/en-us/cpp/c-runtime-library/low-level-i-o?view=msvc-160.

The most critical POSIX libraries (some with bad license):

The main issue is usually signals, filesystems or interactions between stuff.

Creating an organization for modular and hookable CC0 variants of this could be very helpful.
Aim should be on lightness, not completeness (unlike mingw / cygwin).

Anyhow, this probably deseves its own issue on nxdk or elsewhere.

@thrimbor
Copy link
Member

[..] and the Xbox isn't a POSIX platform

Thats the current situation, however that shouldn't stop us from discussing about changing that 🙂.

We can't, really - POSIX specifies lots of things that aren't possible with the Xbox's kernel, such as cli, forking etc. What we can do is provide the parts that can be implemented as wrappers, but implementing and maintaining them takes time and effort, and it might not be worth it.

* file i/o: functions linked above, but can easily be done for most parts (might have to be part of mmap)

* dirent: https://github.com/tronkko/dirent

* pthreads: https://sourceware.org/pthreads-win32/

* mmap: https://github.com/alitrack/mman-win32

I did start implementing POSIX-stlye file I/O a while ago for SDLPoP, but I abandoned it. You have to keep a mapping table to convert between the POSIX fd and the winapi handle, and in the end it's probably far easier to just convert whatever you're porting to C file I/O (like afaik Ryzee did with SDPoP).
Dirent should be relatively simple, pthreads might be a headache, if I remember correctly it's a quite large API. I looked a tiny bit into how that mmap implementation works, and if ReactOS is to be trusted, it relies on a kernel function to create the mapping that is not available on the Xbox.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants