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

implement sysclib_sprintf #19097

Merged
merged 2 commits into from
May 1, 2024
Merged

implement sysclib_sprintf #19097

merged 2 commits into from
May 1, 2024

Conversation

Kethen
Copy link
Contributor

@Kethen Kethen commented Apr 29, 2024

It gets through https://github.com/Kethen/psp-string-test/ with the following result

module started
test1 |1 2 3 4 5 6 7 8 9 10 11 12| test1
test2 |1 2 3 4 5 6 7 8 9 10 11 12| test2
test3 |1 2 3 4 5 6 7 8 9 10 11 12| test3
test4 |1 2 3 4 5 6 7 8 9 a b c| test4
test5 |123 45677654 321| test5
test6 |305419896 2427178479 1311768467294899695 305419896 -1867788817 1311768467294899695 12345678 90abcdef 1234567890abcdef| test6
test7 |12345678 90abcdef 1234567890abcdef 12345678 90abcdef 1234567890abcdef 12345678 90abcdef 1234567890abcdef| test7
test8 |123 456 1234567887654321 654 321| test8

int arg_idx = 0;
int fmt_len = 0;

for(const char *c = Memory::GetCharPointerUnchecked(fmt); *c != '\0'; c++){
Copy link
Owner

Choose a reason for hiding this comment

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

Please at least roughly follow PPSSPP's general code style throughout. You're close, but we write for( as for (, and ) and { are separated by a space (last part of the line).

// going by https://cplusplus.com/reference/cstdio/printf/#compatibility
// no idea what the kernel module really supports as of writing this

if(*c == '%'){
Copy link
Owner

Choose a reason for hiding this comment

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

This if chain would be better written as a switch.

Comment on lines 750 to 751
ERROR_LOG(SCEKERNEL, "Fmt: %s", Memory::GetCharPointerUnchecked(fmt));
ERROR_LOG(SCEKERNEL, "PSP arg reg dump: 0x%08x 0x%08x 0x%08x 0x%08x 0x%08x 0x%08x 0x%08x 0x%08x",
Copy link
Owner

Choose a reason for hiding this comment

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

Really still want to ERROR_LOG these? Doesn't seem like errors to me, more like the normal execution path.

if (Memory::IsValidAddress(dst) && Memory::IsValidAddress(fmt)) {
// TODO: Properly use the format string with more parameters.
return sprintf((char *)Memory::GetPointerUnchecked(dst), "%s", Memory::GetCharPointerUnchecked(fmt));

Copy link
Owner

Choose a reason for hiding this comment

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

Would be better to flip the logic of the if and early-out, instead of indenting the entire function.

Comment on lines 761 to 763
for(int i = 0;i < 24; i+=8){
u32 base = psp_stack_pointer + i * 4;
ERROR_LOG(SCEKERNEL, "PSP stack dump: 0x%08x 0x%08x 0x%08x 0x%08x 0x%08x 0x%08x 0x%08x 0x%08x",
Copy link
Owner

Choose a reason for hiding this comment

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

This stack dumping should surely not occur on every call. Comment out or something.

read_cnt++;
}
/*
// windows compiler don't like this
Copy link
Owner

Choose a reason for hiding this comment

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

For good reason, dynamic stack allocation is not a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, you're right, I supposed I should not be too trusty with snprintf's return value being always

@hrydgard
Copy link
Owner

hrydgard commented May 1, 2024

Much, much better! Hm, I think the debug logging should maybe be VERBOSE, but I'll go ahead and merge and possibly adjust after.

@hrydgard hrydgard merged commit 59be4b0 into hrydgard:master May 1, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants