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

Runtime restructuring #155

Merged
merged 3 commits into from
Sep 30, 2019
Merged

Conversation

thrimbor
Copy link
Member

@thrimbor thrimbor commented Aug 25, 2019

This restructures our runtime code to live in subdirectories in lib/xboxrt. My goal was to group all runtime functionality, and I plan to build on this in the near future by adding a mechanism to PDCLib that allows us to enhance standard headers, so that we have a place where we can cleanly and tidily keep Windows/Xbox specific C standard enhancements (like strdup, or rand_s). It will also get a new directory called vcruntime, where the C++ runtime functions will live (required by libc++ for stuff like RTTI, exceptions etc.).

There's a good chance that even more code will be moved here in the future (like TLS handling when we move to Win32 threads).

The included SDL2 update only includes a fix for the modified path of debug.h.

/edit:
Include guards only adjusted for debug.h, imho the remaining ones still fit.

@thrimbor thrimbor force-pushed the runtime_improvements branch from 4ccffd5 to 792ee00 Compare August 28, 2019 22:14
Copy link
Contributor

@dracc dracc left a comment

Choose a reason for hiding this comment

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

Looks good to me. 👍

Makefile Outdated Show resolved Hide resolved
@thrimbor thrimbor force-pushed the runtime_improvements branch from 792ee00 to 3749fa8 Compare September 7, 2019 03:15
lib/xboxrt/Makefile Outdated Show resolved Hide resolved
@@ -0,0 +1,17 @@
SRCS += \
$(NXDK_DIR)/lib/xboxrt/libc_extensions/debug.c \
$(NXDK_DIR)/lib/xboxrt/libc_extensions/stat.c \
Copy link
Member

Choose a reason for hiding this comment

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

Sidenote: This file is currently entirely dead code (disabled by #if 0). Do we intend to revive it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really sure. Afaik Win32 has it, so it's probably a good idea to implement this properly, and some libc++ filesystem code wants to have that, too (although that code in libc++ is broken anyway atm). Should I eventually do it, I'd to it from scratch though and on top of winapi functions.
Afaik we only keep stat.c around because it forms a pair with stat.h and that one is required by our usb code.

Copy link
Member

@JayFoxRox JayFoxRox Sep 7, 2019

Choose a reason for hiding this comment

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

The MS extensions (aiming for POSIX compatibility probably) are usually prefixed, so it would be _stat. The UNIX and POSIX variants aren't usually prefixed: stat. Win32 would probably use GetFileAttributes instead (also see #161).

I think it's more interesting to fix on winapi support (which has been going quite well), and we could probably use old mingw / cygwin wrappers in the future (the new ones seem to use _stat32 from the MS CRT, but I believe they used to use Winapi directly).

stat.h can definitely stay for now (at least it declares 1 type). I'm still not sure what to do with stat.c - for this PR, I'm fine with just keeping it around.

@thrimbor thrimbor force-pushed the runtime_improvements branch from 3749fa8 to 7471731 Compare September 7, 2019 04:05
@thrimbor thrimbor force-pushed the runtime_improvements branch 2 times, most recently from 833b8ea to 13757c8 Compare September 7, 2019 06:12
@JayFoxRox
Copy link
Member

This was discussed in length on XboxDev Discord (by @mborgerson and me), starting at: https://discordapp.com/channels/428359196719972353/428360618102226946/620187257302679558
Main topics were: where to put debug.h and "what is a runtime"

debug.h

We agreed that the current debug library is bad overall, and it's neither fitting into a libcrt, libc, pdclib or hal.
Ideally we'll replace it with a dedicated debugging lib, so this is hopefully a temporary issue; see #172

For now, we agreed that we can tolerate the legacy-framebuffer debug.h in hal/debug.h.

"what is a runtime"

We concluded that runtime is the platform specific part of the C compiler that's not already compiled into the program; also see https://en.m.wikipedia.org/wiki/Runtime_library. It could potentially include the C standard library (= the parts which are necessary to make the full C language).

@mborgerson suggested (Editors note: libcxx line is by me):

split into:

  • libc (Editors note: Probably pdclib + libc_extensions)
  • libcxx for C++ (Editors note: Planned by thrimbor)
  • libcrt (Editors note: Probably xlibc-rt) for the C/compiler runtime.

I then realized that this is quite close to what this PR was already trying to do (see notes), except that libc_extensions / libcxx / libcrt are in the xboxrt subdirectory to isolate them from user-libraries.

However, I'm not sure why pdclib somehow did not get that xboxrt treatment if we include libc_extensions in the runtime definition.
If we exclude libc from the runtime definition, then xboxrt is a bad name as it still contains libc_extensions.
Eitherway, this feels inconsistent.


To not further delay this PR, we agreed that this could be merged as-is without changes.
We rarely touch these files anyway.

However, a more suitable name for xboxrt (which, I think, is an OpenXDK relict) could still be suggested or an explanation as to why pdclib is not in xboxrt.

@thrimbor
Copy link
Member Author

@mborgerson suggested (Editors note: libcxx line is by me):

split into:

  • libc (Editors note: Probably pdclib + libc_extensions)
  • libcxx for C++ (Editors note: Planned by thrimbor)
  • libcrt (Editors note: Probably xlibc-rt) for the C/compiler runtime.

I then realized that this is quite close to what this PR was already trying to do (see notes), except that libc_extensions / libcxx / libcrt are in the xboxrt subdirectory to isolate them from user-libraries.

However, I'm not sure why pdclib somehow did not get that xboxrt treatment if we include libc_extensions in the runtime definition.
If we exclude libc from the runtime definition, then xboxrt is a bad name as it still contains libc_extensions.
Eitherway, this feels inconsistent.
However, a more suitable name for xboxrt (which, I think, is an OpenXDK relict) could still be suggested or an explanation as to why pdclib is not in xboxrt.

I'd be absolutely fine with someone giving xboxrt a better name or moving PDCLib into that folder (in fact we might want to merge all of that into a single library file). I really don't consider naming or organizing a skill of mine, I only made these changes to avoid making an even greater mess in the lib directory when adding more stuff required by libc++ (like vcruntime, which contains stuff required for RTTI and exceptions, or non-standard PDCLib extensions).

@JayFoxRox
Copy link
Member

This has a conflict now @thrimbor . I'm also still not sure how to go forward, however, considering nobody veto'd it, I'd be willing to merge it regardless (as stated in my last post), just to avoid any further pile-up of PRs.

@thrimbor thrimbor force-pushed the runtime_improvements branch from 13757c8 to 4b7548e Compare September 26, 2019 02:41
@thrimbor
Copy link
Member Author

thrimbor commented Sep 26, 2019

Rebase pushed. macOS CI will fail due to the issue described in #173.

@thrimbor thrimbor force-pushed the runtime_improvements branch from 4b7548e to 3e1230e Compare September 26, 2019 18:16
@thrimbor thrimbor force-pushed the runtime_improvements branch from 3e1230e to b233cd3 Compare September 30, 2019 01:16
@@ -48,7 +48,8 @@ EXTRACT_XISO = $(NXDK_DIR)/tools/extract-xiso/extract-xiso
TOOLS = cxbe vp20compiler fp20compiler extract-xiso
NXDK_CFLAGS = -target i386-pc-win32 -march=pentium3 \
-ffreestanding -nostdlib -fno-builtin -fno-exceptions \
-I$(NXDK_DIR)/lib -I$(NXDK_DIR)/lib/xboxrt \
-I$(NXDK_DIR)/lib -I$(NXDK_DIR)/lib/xboxrt/libc_extensions \
-I$(NXDK_DIR)/lib/hal \
Copy link
Member

Choose a reason for hiding this comment

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

This include path shouldn't be added in my opinion.

We should force people to explicitly use hal/<...>.h so they know which APIs belong to this legacy API.
This is also helpful when we do libraryization as people can explicitly see which libs they use directly.

Unfortunately, at this point, it's still necessary because:

/lib/pdclib/platform/xbox/functions/assert/assert.c:2:10: fatal error: 
      'debug.h' file not found
#include <debug.h>

We should fix this in the future.

@JayFoxRox JayFoxRox merged commit 2dbf7ba into XboxDev:master Sep 30, 2019
@thrimbor thrimbor deleted the runtime_improvements branch October 2, 2019 01:00
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