Skip to content

Conversation

Kethen
Copy link
Collaborator

@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
Collaborator 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

@Kethen Kethen force-pushed the sysclib_sprintf branch from 135e307 to fa0eaeb Compare May 1, 2024 09:32
@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.

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