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.

Sign up for GitHub

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

Jump to bottom

Added various features #655

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

Added various features #655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bd30f4b
cxbe: Add the ability to handle long section names
PQCraft Jul 20, 2023
2f0deb5
cxbe: Add -TITLEID:{%c%c-%u|%x}
PQCraft Jul 20, 2023
c0a40d6
cxbe: Add -REGION:{-|[n][j][w][m]}
PQCraft Sep 19, 2023
b62a0ae
cxbe: Add -VERSION
PQCraft Jul 20, 2023
fc892e7
cxbe: Set the proper flags for *IMAGE sections
PQCraft Jul 21, 2023
25c32e4
cxbe: Set proper flags on debug sections
PQCraft Jul 22, 2023
e13a346
Add features to Makefile
PQCraft Jul 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,8 @@ DEPS += $(filter %.cpp.d, $(SRCS:.cpp=.cpp.d))

$(OUTPUT_DIR)/default.xbe: main.exe $(OUTPUT_DIR) $(CXBE)
@echo "[ CXBE ] $@"
$(VE)$(CXBE) -OUT:$@ -TITLE:$(XBE_TITLE) $< $(QUIET)
$(VE)$(CXBE) -OUT:$@ -TITLE:$(XBE_TITLE) -TITLEID:$(XBE_TITLEID) \
-REGION:$(XBE_REGION) -VERSION:$(XBE_VERSION) $< $(QUIET)
Comment on lines -101 to +102
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.


$(OUTPUT_DIR):
@mkdir -p $(OUTPUT_DIR);
Expand Down
47 changes: 46 additions & 1 deletion tools/cxbe/Exe.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ Exe::Exe(const char *x_szFilename)
// read section headers
{
m_SectionHeader = new SectionHeader[m_Header.m_sections];
m_SectionHeader_longname = new SectionHeader_longname[m_Header.m_sections];

printf("Exe::Exe: Reading Section Headers...\n");

Expand All @@ -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 •
edited
Loading

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 •
edited
Loading

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)

{
m_SectionHeader_longname[v].m_offset = 0;
for(uint32 i = 1; i < 8; ++i)
{
char c = m_SectionHeader[v].m_name[i];
if(!c)
break;
if(c < '0' || c > '9') // not a long section after all?
goto notlong;
m_SectionHeader_longname[v].m_offset *= 10;
m_SectionHeader_longname[v].m_offset += c - '0';
}
m_SectionHeader_longname[v].m_longname = new char[256]();

long tmppos = ftell(ExeFile);
fseek(ExeFile, m_Header.m_symbol_table_addr + m_SectionHeader_longname[v].m_offset,
SEEK_SET);

uint32 i;
for(i = 0; i < 255; ++i)
{
int c = fgetc(ExeFile);
if(!c || c == EOF)
break;
m_SectionHeader_longname[v].m_longname[i] = c;
}
m_SectionHeader_longname[v].m_longname[i] = 0;

fseek(ExeFile, tmppos, SEEK_SET);
printf("OK %d (long)\n", v, m_SectionHeader_longname[v].m_offset,
m_SectionHeader_longname[v].m_longname);
}
else
{
notlong:;
m_SectionHeader_longname[v].m_longname = NULL;
printf("OK %d\n", v);
}
}
}

Expand Down Expand Up @@ -184,6 +224,7 @@ Exe::Exe(const char *x_szFilename)
void Exe::ConstructorInit()
{
m_SectionHeader = NULL;
m_SectionHeader_longname = NULL;
m_bzSection = NULL;
}

Expand All @@ -193,12 +234,16 @@ Exe::~Exe()
if(m_bzSection != 0)
{
for(uint32 v = 0; v < m_Header.m_sections; v++)
{
delete[] m_SectionHeader_longname[v].m_longname;
delete[] m_bzSection[v];
}

delete[] m_bzSection;
}

delete[] m_SectionHeader;
delete[] m_SectionHeader_longname;
}

// export to Exe file
Expand Down
7 changes: 7 additions & 0 deletions tools/cxbe/Exe.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,13 @@ class Exe : public Error
uint32 m_characteristics; // characteristics for this segment
} __attribute((packed)) * m_SectionHeader;

// array to store long section names
struct SectionHeader_longname
{
char *m_longname;
uint32 m_offset;
} __attribute((packed)) * m_SectionHeader_longname;

// array of section data
uint08 **m_bzSection;

Expand Down
114 changes: 109 additions & 5 deletions tools/cxbe/Main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "Exe.h"
#include "Xbe.h"

#include <stdlib.h>
#include <string.h>

// program entry point
Expand All @@ -19,18 +20,35 @@ int main(int argc, char *argv[])
char szXbeFilename[OPTION_LEN + 1] = { 0 };
char szDumpFilename[OPTION_LEN + 1] = { 0 };
char szXbeTitle[OPTION_LEN + 1] = "Untitled";
char szXbeTitleID[OPTION_LEN + 1] = "";
char szXbeRegions[OPTION_LEN + 1] = "";
char szXbeVersion[OPTION_LEN + 1] = "";
char szMode[OPTION_LEN + 1] = "retail";
char szLogo[OPTION_LEN + 1] = "";
char szDebugPath[OPTION_LEN + 1] = "";

bool bRetail;
uint32 dwTitleId = 0xFFFF0002;
uint32 dwRegions = XBEIMAGE_GAME_REGION_NA | XBEIMAGE_GAME_REGION_JAPAN |
XBEIMAGE_GAME_REGION_RESTOFWORLD | XBEIMAGE_GAME_REGION_MANUFACTURING;
uint32 dwVersion;

const char *program = argv[0];
const char *program_desc = "CXBE EXE to XBE (win32 to Xbox) Relinker (Version: " VERSION ")";
Option options[] = {
{ 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}" },
{ szXbeRegions, "REGION",
"{-|[n][j][w][m]}\n"
" -=none, n=North America, j=Japan, w=world, m=manufacturing" },
{ szXbeVersion, "VERSION", "version" },
{ szMode, "MODE", "{debug|retail}" },
{ szLogo, "LOGO", "filename" },
{ szDebugPath, "DEBUGPATH", "path" },
{ NULL }
};

if(ParseOptions(argv, argc, options, szErrorMessage))
Expand All @@ -54,6 +72,91 @@ int main(int argc, char *argv[])
szXbeTitle[40] = '\0';
}

// interpret title id
if(szXbeTitleID[0])
{
bool hex = true;
for(int i = 0; szXbeTitleID[i]; ++i)
{
if(szXbeTitleID[i] == '-')
{
hex = false;
break;
}
}
if(hex)
{
sscanf(szXbeTitleID, "%x", &dwTitleId);
}
else
{
char titlechar[2];
unsigned titleno;
Comment on lines +93 to +94
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

if(sscanf(szXbeTitleID, "%c%c-%u", &titlechar[0], &titlechar[1], &titleno) != 3)
{
strncpy(szErrorMessage, "invalid TITLEID", ERROR_LEN);
goto cleanup;
}
if(titleno > 0xFFFF)
{
printf("WARNING: Title ID number too high (max is 65535)\n");
titleno = 0xFFFF;
}
dwTitleId = (titlechar[0] << 24) | (titlechar[1] << 16) | titleno;
}
}

// interpret region flags
if(szXbeRegions[0])
{
char c;
for(int i = 0; (c = szXbeRegions[i]); ++i)
{
switch(c)
{
case '-':;
dwRegions = 0;
goto breakfor;
case 'a':;
dwRegions = XBEIMAGE_GAME_REGION_NA | XBEIMAGE_GAME_REGION_JAPAN |
XBEIMAGE_GAME_REGION_RESTOFWORLD |
XBEIMAGE_GAME_REGION_MANUFACTURING;
goto breakfor;
case 'n':;
dwRegions |= XBEIMAGE_GAME_REGION_NA;
break;
case 'j':;
dwRegions |= XBEIMAGE_GAME_REGION_JAPAN;
break;
case 'w':;
dwRegions |= XBEIMAGE_GAME_REGION_RESTOFWORLD;
break;
case 'm':;
dwRegions |= XBEIMAGE_GAME_REGION_MANUFACTURING;
break;
default:;
printf("WARNING: Invalid region char: %c\n", c);
break;
}
}
breakfor:;
}
else
{
dwRegions = XBEIMAGE_GAME_REGION_NA | XBEIMAGE_GAME_REGION_JAPAN |
XBEIMAGE_GAME_REGION_RESTOFWORLD | XBEIMAGE_GAME_REGION_MANUFACTURING;
}

// interpret version
if(szXbeVersion[0])
{
dwVersion = strtoul(szXbeVersion, NULL, 0);
}
else
{
dwVersion = 0;
}

// verify we received the required parameters
if(szExeFilename[0] == '\0')
{
Expand Down Expand Up @@ -90,7 +193,8 @@ int main(int argc, char *argv[])
LogoPtr = &logo;
}

Xbe *XbeFile = new Xbe(ExeFile, szXbeTitle, bRetail, LogoPtr, szDebugPath);
Xbe *XbeFile = new Xbe(
ExeFile, szXbeTitle, dwTitleId, dwRegions, dwVersion, bRetail, LogoPtr, szDebugPath);
Comment on lines +196 to +197
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.


if(XbeFile->GetError() != 0)
{
Expand Down
76 changes: 54 additions & 22 deletions tools/cxbe/Xbe.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ static size_t BasenameOffset(const std::string &path)
}

// construct via Exe file object
Xbe::Xbe(class Exe *x_Exe, const char *x_szTitle, bool x_bRetail, const std::vector<uint08> *logo,
Xbe::Xbe(class Exe *x_Exe, const char *x_szTitle, uint32 x_dwTitleID, uint32 x_dwRegions,
uint32 x_dwVersion, bool x_bRetail, const std::vector<uint08> *logo,
const char *x_szDebugPath)
{
ConstructorInit();
Expand Down Expand Up @@ -125,8 +126,16 @@ Xbe::Xbe(class Exe *x_Exe, const char *x_szTitle, bool x_bRetail, const std::vec
{
uint32 s = 0;

while(s < 8 && x_Exe->m_SectionHeader[v].m_name[s] != '\0')
s++;
if(x_Exe->m_SectionHeader_longname[v].m_longname)
{
while(s < 255 && x_Exe->m_SectionHeader_longname[v].m_longname[s] != '\0')
s++;
}
else
{
while(s < 8 && x_Exe->m_SectionHeader[v].m_name[s] != '\0')
s++;
}

mrc += s + 1;
}
Expand Down Expand Up @@ -271,8 +280,7 @@ Xbe::Xbe(class Exe *x_Exe, const char *x_szTitle, bool x_bRetail, const std::vec

m_Certificate.dwTimeDate = CurrentTime;

// TODO: generate in the form CX-9999
m_Certificate.dwTitleId = 0xFFFF0002;
m_Certificate.dwTitleId = x_dwTitleID;

// title name
memset(m_Certificate.wszTitleName, 0, sizeof(m_Certificate.wszTitleName));
Expand All @@ -297,19 +305,15 @@ Xbe::Xbe(class Exe *x_Exe, const char *x_szTitle, bool x_bRetail, const std::vec
XBEIMAGE_MEDIA_TYPE_MEDIA_BOARD | XBEIMAGE_MEDIA_TYPE_NONSECURE_HARD_DISK |
XBEIMAGE_MEDIA_TYPE_NONSECURE_MODE;

// 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;
Comment on lines -300 to +308
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


// TODO: allow configuration
m_Certificate.dwGameRatings = 0xFFFFFFFF;

// always disk 0, AFAIK
m_Certificate.dwDiskNumber = 0;

// TODO: allow configuration
m_Certificate.dwVersion = 0;
m_Certificate.dwVersion = x_dwVersion;

// generate blank LAN, signature, and alternate signature keys
{
Expand Down Expand Up @@ -345,7 +349,7 @@ Xbe::Xbe(class Exe *x_Exe, const char *x_szTitle, bool x_bRetail, const std::vec

// write section headers / section names
{
m_szSectionName = new char[m_Header.dwSections][9];
m_szSectionName = new char[m_Header.dwSections][256];

m_SectionHeader = new SectionHeader[m_Header.dwSections];

Expand Down Expand Up @@ -373,14 +377,31 @@ Xbe::Xbe(class Exe *x_Exe, const char *x_szTitle, bool x_bRetail, const std::vec

memset(&m_SectionHeader[v].dwFlags, 0, sizeof(m_SectionHeader->dwFlags));

if(characteristics & IMAGE_SCN_MEM_WRITE)
m_SectionHeader[v].dwFlags.bWritable = true;

if((characteristics & IMAGE_SCN_MEM_EXECUTE) ||
(characteristics & IMAGE_SCN_CNT_CODE))
m_SectionHeader[v].dwFlags.bExecutable = true;
// check for $$XTIMAGE or $$XSIMAGE and set the correct flags
if(x_Exe->m_SectionHeader_longname[v].m_longname &&
(!strcmp(x_Exe->m_SectionHeader_longname[v].m_longname, "$$XTIMAGE") ||
!strcmp(x_Exe->m_SectionHeader_longname[v].m_longname, "$$XSIMAGE")))
Comment on lines +382 to +383
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 a fan of this hack. I'd rather like to see proper file insertion support in cxbe and something like -TITLEIMAGE: / -SAVEIMAGE: or similar.

Copy link
Author

Choose a reason for hiding this comment

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

There is currently no code to do section insertion as far as i'm aware, and that could probably be removed later once there is code made to add sections.

Copy link
Member

Choose a reason for hiding this comment

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

There isn't, as I said I'd rather see it implemented than a name-based hack on top of violating the PE spec. nxdk does not require the usage of these sections nor does it implement the APIs relying on them yet, so we might as well take the time to do things properly.

Copy link
Author

Choose a reason for hiding this comment

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

I don't have the time to write something like that and I need the feature now. Loading the image sections takes up ram to do nothing. This code can be removed later when the better code is added.

Copy link
Member

@thrimbor thrimbor Sep 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This code can be removed later when the better code is added.

I'm not really interested in nxdk acquiring even more technical debt. We haven't even gotten rid of the one inherited from OpenXDK yet.

{
m_SectionHeader[v].dwFlags.bInsertedFile = true;
m_SectionHeader[v].dwFlags.bHeadPageRO = true;
m_SectionHeader[v].dwFlags.bTailPageRO = true;
Comment on lines +385 to +387
Copy link
Member

Choose a reason for hiding this comment

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

How were these flag settings determined? They don't match Halo 2.

Copy link
Author

Choose a reason for hiding this comment

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

I used an XBE header dumper on a few XBEs. I think it was Doom 3 and a Quake homebrew XBE I had laying around

}
else
{
if(characteristics & IMAGE_SCN_MEM_WRITE)
m_SectionHeader[v].dwFlags.bWritable = true;

if((characteristics & IMAGE_SCN_MEM_EXECUTE) ||
(characteristics & IMAGE_SCN_CNT_CODE))
m_SectionHeader[v].dwFlags.bExecutable = true;

char *name = (x_Exe->m_SectionHeader_longname[v].m_longname)
? 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.

}

m_SectionHeader[v].dwFlags.bPreload = true;
m_SectionHeader[v].dwVirtualAddr =
x_Exe->m_SectionHeader[v].m_virtual_addr + m_Header.dwPeBaseAddr;

Expand Down Expand Up @@ -431,10 +452,21 @@ Xbe::Xbe(class Exe *x_Exe, const char *x_szTitle, bool x_bRetail, const std::vec
memset(secn, 0, 8);

m_SectionHeader[v].dwSectionNameAddr = hwc_secn;
while(s < 8 && x_Exe->m_SectionHeader[v].m_name[s] != '\0')
if(x_Exe->m_SectionHeader_longname[v].m_longname)
{
m_szSectionName[v][s] = secn[s] = x_Exe->m_SectionHeader[v].m_name[s];
s++;
while(s < 255 && x_Exe->m_SectionHeader_longname[v].m_longname[s] != '\0')
{
m_szSectionName[v][s] = secn[s] = x_Exe->m_SectionHeader_longname[v].m_longname[s];
s++;
}
}
else
{
while(s < 8 && x_Exe->m_SectionHeader[v].m_name[s] != '\0')
{
m_szSectionName[v][s] = secn[s] = x_Exe->m_SectionHeader[v].m_name[s];
s++;
}
}

m_szSectionName[v][s] = '\0';
Expand Down
Loading