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 #655

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Added various features #655

wants to merge 7 commits into from

Conversation

PQCraft
Copy link

@PQCraft PQCraft commented Jul 28, 2023

#651 died because I suck at using git

Changelog copied from #651:

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

This needs some more cleanup work:

  • Commit titles don't match our pattern (cxbe instead of tools/cxbe, Add instead of Added, missing prefix etc.)
  • Several fix-up commits that undo changes made in previous commits (8bc1c97) are not squashed into the commit that was faulty
  • fbd7746 does not need to be a separate commit
  • 2dd28ba has a rather non-descriptive commit title
  • 44069a5 commit title is too long
  • Unrelated changes in the same commit that should instead get split-up and squashed (303a499)
  • There still are Makefile changes in this PR
  • Commits that don't match the formatting style (46b8489), this has to be fixed in the faulty commits themselves, not with a separate commit at the end. Each commit should pass a check with clang-format.

We also usually don't use commit messages unless they're absolutely necessary to provide context.

- 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
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 - 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
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.
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
- Added XBE_TITLEID, XBE_REGION, and XBE_VERSION to take advantage of the new Cxbe features
Copy link
Member

@thrimbor thrimbor left a comment

Choose a reason for hiding this comment

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

Exams are over for now, so here's a more in-depth review.

There still are clang-format rule violations, formatting changes grouped into otherwise unrelated commits and even a commit that doesn't build.
The commit messages describe implementation details and imho are unnecessarily verbose, I'd recommend to just remove them all.
cxbe: Set proper flags on debug sections isn't a good commit title imho (what is "proper"?), I'd suggest something like cxbe: Don't preload .debug sections.
As previously said the Makefile changes won't get merged.

@@ -115,7 +116,46 @@ Exe::Exe(const char *x_szFilename)
goto cleanup;
}

printf("OK %d\n", v);
// interpret long section names
if(m_SectionHeader[v].m_name[0] == '/')
Copy link
Member

Choose a reason for hiding this comment

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

How was this commit tested?
In all my testing, I could not get lld-link to produce a section that exceeds 8 characters, even with trickery. This matches Microsoft's documentation, which says that executable images do not use a string table and do not support section names longer than 8 characters.

Copy link
Author

@PQCraft PQCraft Sep 30, 2023

Choose a reason for hiding this comment

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

I used objcopy to rename a section I made in some inline asm
XTIMAGE -> $$XTIMAGE

Copy link
Member

Choose a reason for hiding this comment

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

I have to say I'm not a fan, I'd rather stay a mile away from intentionally breaking the PE spec (or giving the impression that we support doing so).

Copy link
Author

@PQCraft PQCraft Sep 30, 2023

Choose a reason for hiding this comment

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

The thing with / is documented in the spec on microsoft's page. While it says that long section names are not supported with executables, objcopy can still produce executables with long section names and they load and run just fine.

Oh also debug sections use long section names

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is documented as not being valid for executable images. While it may be possible to hack around the linker for that, it is breaking the spec.

Copy link
Author

Choose a reason for hiding this comment

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

Also I added this because I actually need this to have a title image.
It would incorrectly add debug sections and the image sections as something like /43 which ofc would not be picked up by the dashboard as an icon.

Copy link
Member

Choose a reason for hiding this comment

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

  1. How does the XDK do it?
  2. Can't we tell cxbe to add or rename sections? (Not that I'd like the added complexity, but it's probably better than breaking the spec)

Comment on lines 30 to 41
{ szExeFilename, NULL, "exefile" }, { szXbeFilename, "OUT", "filename" },
{ szDumpFilename, "DUMPINFO", "filename" }, { szXbeTitle, "TITLE", "title" },
{ szMode, "MODE", "{debug|retail}" }, { szLogo, "LOGO", "filename" },
{ szDebugPath, "DEBUGPATH", "path" }, { NULL }
{ szExeFilename, NULL, "exefile" },
{ szXbeFilename, "OUT", "filename" },
{ szDumpFilename, "DUMPINFO", "filename" },
{ szXbeTitle, "TITLE", "title" },
{ szXbeTitleID, "TITLEID", "{%c%c-%u|%x}" },
{ szMode, "MODE", "{debug|retail}" },
{ szLogo, "LOGO", "filename" },
{ szDebugPath, "DEBUGPATH", "path" },
{ NULL }
Copy link
Member

Choose a reason for hiding this comment

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

Why the formatting change? This breaks the clang-format rules.

Copy link
Author

Choose a reason for hiding this comment

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

This was actually done by clang format I think
I remember just adding it to the end

Comment on lines +83 to +84
char titlechar[2];
unsigned titleno;
Copy link
Member

Choose a reason for hiding this comment

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

Using a not explicitly unsigned char can lead to incorrect results when shifting, better use the types defined in Cxbx.h or stdint.h for these variables.

Copy link
Author

Choose a reason for hiding this comment

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

Will adding unsigned do?
I prefer to use stdlib types with an stdlib function

{
char titlechar[2];
unsigned titleno;
if (sscanf(szXbeTitleID, "%c%c-%u", &titlechar[0], &titlechar[1], &titleno) != 3)
Copy link
Member

Choose a reason for hiding this comment

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

This line doesn't follow clang-format rules.

Copy link
Author

Choose a reason for hiding this comment

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

I'll see about doing a rebase and running clang-format on all the commits

Comment on lines -308 to +307
// TODO: allow configuration
m_Certificate.dwGameRegion = XBEIMAGE_GAME_REGION_MANUFACTURING |
XBEIMAGE_GAME_REGION_NA | XBEIMAGE_GAME_REGION_JAPAN |
XBEIMAGE_GAME_REGION_RESTOFWORLD;
m_Certificate.dwGameRegion = x_dwRegions;
Copy link
Member

Choose a reason for hiding this comment

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

This change probably belongs to a different commit and breaks building this one.

Copy link
Author

Choose a reason for hiding this comment

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

Probably
My rebase was kind of sketchy and one of my commits disappeared into thin air
This was probably a result of me trying to re-create the commit

Comment on lines +196 to +197
Xbe *XbeFile = new Xbe(
ExeFile, szXbeTitle, dwTitleId, dwRegions, dwVersion, bRetail, LogoPtr, szDebugPath);
Copy link
Member

Choose a reason for hiding this comment

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

This formatting change does not belong here.

Comment on lines 36 to 37
uint32 x_dwVersion, bool x_bRetail, const std::vector<uint08> *logo, const char *x_szDebugPath)
uint32 x_dwVersion, bool x_bRetail, const std::vector<uint08> *logo,
const char *x_szDebugPath)
Copy link
Member

Choose a reason for hiding this comment

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

This formatting change does not belong here.

? x_Exe->m_SectionHeader_longname[v].m_longname
: (char *)x_Exe->m_SectionHeader[v].m_name;
m_SectionHeader[v].dwFlags.bPreload =
(strcmp(name, ".debug") && strncmp(name, ".debug_", 7));
Copy link
Member

Choose a reason for hiding this comment

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

Why not just strncmp(name, ".debug", 6)?

Copy link
Author

Choose a reason for hiding this comment

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

.debugTest is not a real debug section name while .debug is and anything starting with .debug_ is

Copy link
Member

Choose a reason for hiding this comment

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

Which .debugTest? That is not a section I've ever seen nxdk produce (and nothing else should because the dot prefix is reserved for system sections)

Copy link
Author

Choose a reason for hiding this comment

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

It could be user generated. I only did it for completeness and it's not like it would hurt performance terribly. The more performance-oriented option would be to check if the first 6 matched .debug and then check if the 7th was \0 or _

Copy link
Member

Choose a reason for hiding this comment

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

It could be user generated

The dot prefix is reserved. I don't care about performance here as much as clean code.

tools/cxbe/Xbe.h Outdated
Comment on lines 30 to 31
uint32 x_dwVersion, bool x_bRetail, const std::vector<uint08> *logo = nullptr, const char *x_szDebugPath = nullptr);
uint32 x_dwVersion, bool x_bRetail, const std::vector<uint08> *logo = nullptr,
const char *x_szDebugPath = nullptr);
Copy link
Member

Choose a reason for hiding this comment

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

This formatting change does not belong here.

Comment on lines -101 to +102
$(VE)$(CXBE) -OUT:$@ -TITLE:$(XBE_TITLE) $< $(QUIET)
$(VE)$(CXBE) -OUT:$@ -TITLE:$(XBE_TITLE) -TITLEID:$(XBE_TITLEID) \
-REGION:$(XBE_REGION) -VERSION:$(XBE_VERSION) $< $(QUIET)
Copy link
Member

Choose a reason for hiding this comment

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

As I said before, using this Makefile to build third-party applications should be considered deprecated. It will get removed and we won't add any features to it.

Using your own Makefile not relying on this one is the way to go. If you absolutely have to rely on this Makefile for now and want to use custom cxbe parameters, override this rule in your Makefile which includes this one.

Copy link
Author

Choose a reason for hiding this comment

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

I really can't include your Makefile in mine. It has too many conflicts. It ended up causing a lot of issues with my Makefile especially with flags. Making another Makefile specifically for the NXDK would require maintaining 2 Makefiles and that would be an absolute pain thanks to the complexity of my Makefile. It turned out to be easier to just call $(MAKE) -C $(NXDK_DIR) and pass in stuff.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not suggesting you include the nxdk Makefile, I'm recommending the exact opposite. Don't rely on it, it's going to get removed.

I don't get why you would need two Makefiles either, you already seem to call cxbe yourself in your Makefile.

Copy link
Author

Choose a reason for hiding this comment

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

I currently use it to fetch all the includes from NXDK's lib dir and use it to build the tools.
I'll remove it since I'm assuming no one will use the new vars.

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