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

Use DbgPrint in place of internal _PDCLIB_print #56

Closed
GXTX opened this issue Oct 10, 2023 · 11 comments
Closed

Use DbgPrint in place of internal _PDCLIB_print #56

GXTX opened this issue Oct 10, 2023 · 11 comments

Comments

@GXTX
Copy link

GXTX commented Oct 10, 2023

Attempting to minimize executable size, PDClib' print function uses a fair amount of space and is frankly unneeded, also as far as I can tell these aren't actually printed anywhere to the user(?), either the screen or via serial port. So it should be replaced with a call to DbgPrint.

int main(void)
{
    for(;;) {
        DbgPrint("Hello\n");
    }
    return 0;
}
LTO = y
CFLAGS += -Oz -DNDEBUG
CXXFLAGS += -Oz -DNDEBUG
NXDK_CFLAGS += -Oz -DNDEBUG
NXDK_CXXFLAGS += -Oz -DNDEBUG

As is, this results in a 32KB binary. Nuking the while loops in vfprintf and vsnprintf we end up with 28KB.

@thrimbor
Copy link
Member

It should be replaced where? The above code is not part of pdclib.

@GXTX
Copy link
Author

GXTX commented Oct 10, 2023

@GXTX
Copy link
Author

GXTX commented Oct 10, 2023

The fprintf and fputs in raise.c should also be replaced with DbgPrint.

https://github.com/XboxDev/nxdk-pdclib/blob/nxdk/platform/xbox/functions/signal/raise.c#L52

@thrimbor
Copy link
Member

vfprintf and vsnprintf are implementations of printing functions, so replacing anything there is not an option.

While we could replace the fprintf and fputs calls in this instance it comes with drawbacks and the benefit of doing so is small. A much better solution would be to move forward with XboxDev/nxdk#172 and allow redirection of stdout and stderr, which is much closer to what a user can expect from a C standard library.

If you want to minimize your executable size it's the same procedure for the Xbox as for all other platforms: Avoid the functions that have big dependencies. I doubt you require raise() for your program.

@GXTX
Copy link
Author

GXTX commented Oct 10, 2023

I doubt you require raise() for your program

I don't, however platform initialization calls it, as well as the printf's mentioned.

@thrimbor
Copy link
Member

crt0 does not call any of these functions.

@GXTX
Copy link
Author

GXTX commented Oct 10, 2023

I don't know what to tell you. Compile the sample I gave in the OP and check the disassembly.

@thrimbor
Copy link
Member

I'm not interested in doing your size optimization for you, but here's a free hint: Disable D: automounting.

@GXTX
Copy link
Author

GXTX commented Oct 10, 2023

DVD auto mounting adds essentially nothing due to alignment.

I'm not asking you to size optimize for me, I'm reporting an issue where things are being included and compiled that aren't being used, and you admit yourself that the platform init isn't using these functions, as well as functions that produce no user viewable output being redirected to DbgPrint.

@JayFoxRox
Copy link
Member

JayFoxRox commented Oct 10, 2023

I think this is a bit of a niche.

For nxdk, it should be higher priority to actually work / be functional, rather than optimizing for size.
I assume that nxdk is far better at size optimization than the XDK, simply by being based on modern tooling.

DVD auto mounting adds essentially nothing due to alignment.

His point was that some code must depend on those functions so they end up in the binary.
So some code - like the DVD automount - is probably responsible for those functions being linked.
As @thrimbor mentioned: Your code (or CRT0 which runs your code) doesn't seem to depend on it, so your linker probably links some files which do depend on them.

I'm reporting an issue where things are being included and compiled that aren't being used

That shouldn't happen (if you instruct your compiler / linker to remove unused stuff) and would potentially be out of scope for nxdk (and more of a clang/lld thing)

I think we can look into fixing the size issue if you can actually figure out which functions depend on vfprintf or vsnprintf. Right now, this issue is too vague.

Even if there's a bug (/ misconfiguration) in nxdk, you could work around it.
If you are confident that these functions aren't used, you can probably also replace those functions in the object files if you are desperately size optimizing.. however, in that case there are probably better ways to compress your binary (or its memory footprint).
Eitherway you'll have to dive into object files and compression tooling probably.


Also note that _PDCLIB_print is doing something different from DbgPrint; so I'm not sure how you want to replace one with the other? In some cases you can probably use DbgPrint, but it's not generic or functional enough for libc purposes - it's clearly meant for a specific niche (kernel debugging).

@GXTX
Copy link
Author

GXTX commented Oct 10, 2023

I think we can look into fixing the size issue if you can actually figure out which functions depend on vfprintf or vsnprintf. Right now, this issue is too vague.

Removing the forced stack protection and associated bits brings the majority of savings. Perhaps we can add define, or somehow check when no-stack-protector is set. We can set DEBUG_CONSOLE (which is only used in check_stack.c), however that only removes the ancillary printf's.

#pragma comment(linker, "/include:__xlibc_check_stack")

https://github.com/XboxDev/nxdk/blob/ed21e83da43b8b8da14a9a78cf2528d9b5df86a7/lib/xboxrt/c_runtime/check_stack.c#L22

And signal being included is because of malloc, setting #define PROCEED_ON_ERROR 1 removes the abort calls.

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

No branches or pull requests

3 participants