XboxDev / nxdk-pdclib

The Public Domain C Library (adapted for original Xbox / nxdk toolchain)
http://pdclib.rootdirectory.de
Creative Commons Zero v1.0 Universal
19 stars 9 forks source link

Use DbgPrint in place of internal `_PDCLIB_print` #56

Closed GXTX closed 1 year ago

GXTX commented 1 year ago

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 commented 1 year ago

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

GXTX commented 1 year ago

https://github.com/XboxDev/nxdk-pdclib/blob/nxdk/functions/stdio/vfprintf.c#L47

https://github.com/XboxDev/nxdk-pdclib/blob/nxdk/functions/stdio/vsnprintf.c#L30

Called in both of these places. I haven't found where or why these are being called in such a binary yet.

GXTX commented 1 year ago

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 commented 1 year ago

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 https://github.com/XboxDev/nxdk/issues/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 commented 1 year ago

I doubt you require raise() for your program

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

thrimbor commented 1 year ago

crt0 does not call any of these functions.

GXTX commented 1 year ago

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

thrimbor commented 1 year ago

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

GXTX commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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. https://github.com/XboxDev/nxdk-pdclib/blob/5a3a4c6170f4d544230a43b1575812eeb709514c/platform/xbox/crt0.c#L10

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.