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

Added various features #651

Closed
wants to merge 17 commits into from
Closed

Added various features #651

wants to merge 17 commits into from

Conversation

PQCraft
Copy link

@PQCraft PQCraft commented Jul 20, 2023

tools/cxbe:

  • Added the ability to handle long section names (moved the limit from 8 chars to 255)
  • Added an option to set the XBE's title ID to either a hex code (like FFFF0002) or a human-readable code (like CX-9999)
  • Added an option to set the XBE's region flags
  • Added an option to set the XBE's version (can be dec, hex, or oct)

Makefile:

  • Added XBE_TITLEID to define a title ID (goes to cxbe as -TITLEID:$(XBE_TITLEID))
  • Added XBE_REGION to define a title ID (goes to cxbe as -REGION:$(XBE_REGION))
  • Added XBE_VERSION to define a title ID (goes to cxbe as -VERSION:$(XBE_VERSION))

@thrimbor
Copy link
Member

I haven't given this an in-depth review yet, but here's some preliminary feedback:

tools/cxbe:

This commit contains multiple separate changes and should be split up, with each commit introducing one feature and a short but descriptive commit title following the style of previous commits.

lib/pbkit:

The printing functionality in pbkit is a bit of a legacy feature - pbkit is currently a wild mix of high- and low-level code, and the printing code is most likely going to be removed soon-ish, so imho it doesn't make much sense to add features to it. It's only still here because it can help with debugging.
If you need a way to print text, I'd suggest having your own printing code in your app (potentially using the current pbkit code as a starting point).

Makefile:

The makefile is a remnant from the early days of nxdk, I'd actually recommend against using it for anything other than building nxdk itself. We've been working on removing the need for it by adding wrapper scripts for the compiler and CMake support, and we'll eventually remove it.
For the moment, if you rely on the features you added, I'd recommend adding them as overrides to your own makefile and including the nxdk makefile like the samples do, but the most future proof solution is supplying your own independent makefile.

* Added an `objcopy` line after the linking stage to redirect `XTIMAGE` to `$$XTIMAGE` and `XSIMAGE` to `$$XSIMAGE` (it is impossible to define these sections otherwise)

Hmmm, interesting. I'll try to find some time to experiment with this, but if there really is no other way we should probably put that info into the nxdk wiki.

This review might sound a bit negative, but be assured that I really do appreciate people upstreaming fixes and features 👍

@PQCraft
Copy link
Author

PQCraft commented Jul 20, 2023

Splitting up the commits for cxbe might take a bit since I will need to go into the code and manually undo the changes in reverse order then redo them

@PQCraft
Copy link
Author

PQCraft commented Jul 20, 2023

For pbkit, I'll probably steal pbkit's text functions directly and paste them into my rendering code lol

For the Makefile, I am relying on calling it from my actual Makefile using this little snippet:

    ...
    else ifeq ($(CROSS),xbox)
        ifndef NXDK_DIR
            .PHONY: error
            error:
	            @echo Please define the NXDK_DIR environment variable
	            @exit 1
        endif
        ifneq ($(MODULE),engine)
            .PHONY: error
            error:
	            @echo Invalid module: $(MODULE)
	            @exit 1
        endif

        PLATFORM := Xbox

        export XBE_TITLE := PlatinumSrc
        export XBE_TITLEID := PQ-001
        export XBE_VERSION := $(shell grep '#define PSRC_BUILD ' src/version.h | sed 's/#define .* //')
        export GEN_XISO := $(XBE_TITLE).xiso.iso
        export NXDK_SDL := y
        SRCS := $(wildcard $(SRCDIR)/psrc_aux/*.c)
        SRCS := $(SRCS) $(wildcard $(SRCDIR)/psrc_engine/*.c)
        SRCS := $(SRCS) $(wildcard $(SRCDIR)/psrc_enginemain/*.c)
        SRCS := $(SRCS) $(wildcard $(SRCDIR)/psrc_server/*.c)
        SRCS := $(SRCS) $(wildcard $(SRCDIR)/stb/*.c)
        export SRCS
        export NXDK_SDL = y

        MKENV := ${MKENV} OUTPUT_DIR=xbox
        CPPFLAGS := $(CPPFLAGS) -DSTBI_NO_SIMD -DPB_HAL_FONT
        ifdef DEBUG
            CFLAGS := $(CFLAGS) -Og -g
            CPPFLAGS := $(CPPFLAGS) -DDBGLVL=$(DEBUG)
            MKENV := ${MKENV} DEBUG=y
        else
            ifndef O
                O = 2
            endif
            CFLAGS := $(CFLAGS) -O$(O)
            ifndef NOLTO
                MKENV := ${MKENV} LTO=y
            endif
        endif

        export CFLAGS := $(CFLAGS) $(CPPFLAGS)
        export LDFLAGS

        xbox-build:
	        @$(MAKE) --no-print-directory -f $(NXDK_DIR)/Makefile ${MKENV}

        xbox-clean:
	        @$(MAKE) --no-print-directory -f $(NXDK_DIR)/Makefile ${MKENV} clean

        xbox-distclean:
	        @$(MAKE) --no-print-directory -f $(NXDK_DIR)/Makefile ${MKENV} distclean

        .PHONY: xbox-build xbox-clean xbox-distclean
    else
    ...

@PQCraft
Copy link
Author

PQCraft commented Jul 20, 2023

For the $$XTIMAGE and $$XSIMAGE sections, putting a $ in the section name causes the linker to remove everything after the $ and the $ itself (my$sect -> my). I think it's for some special Windows stuff with shared libraries but the linker does it regardless of the output. Also the linker will truncate any section longer than 8 chars and these sections are both 9 chars. It isn't possible to tell the linker to stop modifying section names or to allow long section names so you have to edit the binary after linking using an external tool (like objcopy).

- Increased the buffer sizes from 9 to 256 which now makes the max section name length 255 chars
- Added code in Exe.cpp to parse long section names (they begin with /)
- Modified code in Xbe.cpp to write out sections names longer than 8 chars
Caused by strcasecmp on line 1413 comparing "name" which may not have a null terminator
The Title ID is interpreted in Main.cpp and passed as x_dwTitleID when creating the XBE. Title IDs can be in human-readable form (like CX-9999) or a hex code (like 4358270F). Strings without - in them are tried as hex codes.
Region flags can be 'a' for all regions, '-' for no regions, or any combo of 'n' for North America, 'j' for Japan, 'w' for world, and/or 'm' for manufacturing
The version is interpreted by strtoul() which can do decimal, hex, and octal
- Added XBE_TITLEID, XBE_REGION, and XBE_VERSION to take advantage of the new Cxbe features
- Added an objcopy line (with || exit 0 to make sure it doesn't fail the build) to rename XTIMAGE to $$XTIMAGE and XSIMAGE to $$XSIMAGE
- The default for XBE_TITLEID is FFFF0002 for compatibility reasons. The new default in Cxbe is 4358270F to match CX-9999.
@PQCraft
Copy link
Author

PQCraft commented Jul 20, 2023

Ok @thrimbor I split up the cxbe commits and also removed the changes to pbkit 👍

tools/cxbe/Xbe.h Outdated Show resolved Hide resolved
tools/cxbe/Exe.cpp Outdated Show resolved Hide resolved
tools/cxbe/Exe.cpp Outdated Show resolved Hide resolved
// Old value was 8 which caused problems
// For example, $$XTIMAGE and $$XSIMAGE are 9 bytes long not including null
tools/cxbe/Main.cpp Outdated Show resolved Hide resolved
tools/cxbe/Main.cpp Outdated Show resolved Hide resolved
tools/cxbe/Main.cpp Outdated Show resolved Hide resolved
tools/cxbe/Main.cpp Outdated Show resolved Hide resolved
and changed the long section interpreter to assume not a long section if there is something other than numbers after the /
Makefile Outdated Show resolved Hide resolved
@PQCraft
Copy link
Author

PQCraft commented Jul 20, 2023

@JayFoxRox I fixed all of the issues I think

@PQCraft PQCraft requested a review from JayFoxRox July 20, 2023 23:43
…$XSIMAGE

bInsertedFile, bHeadPageRO, and bTailPageRO should be set and bPreload should be cleared otherwise it causes memory/stack corruption when running the XBE (my theory is that it overwrites some program data with the image data unless you clear bPreload). A more permanent solution would probably be to add an option set the flags for specific sections.
I no longer use NXDK's Makefile to build the XBE so I reverted this change. I now only use the NXDK Makefile to build the tools and libs.
Another little "hack" to clear the preload flag on sections named .debug or starting with .debug_ so they won't be loaded and waste memory
@PQCraft
Copy link
Author

PQCraft commented Jul 28, 2023

IDK how to make the "This branch cannot be rebased due to conflicts" message go away so I'm going to open a new PR and see if that works.

@PQCraft PQCraft closed this Jul 28, 2023
@PQCraft PQCraft deleted the features branch July 28, 2023 18:29
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