It would be nice to be able to see the return value from the patched calls, though that will be a bit more complex as you need to report twice, once before the call and once after.
There are some functions that return 64-bit values too, if you get to the point of reporting on return codes.
But i can just call original function before the log, so to have result as well at the same time, i mean something like:
// Wrapper function for patching Open
static BPTR Patched_Open(struct DOSIFace *Self, CONST_STRPTR name, LONG accessMode) {
BPTR result = 0;
// Call the original function
if (Original_Open) {
result = Original_Open(Self, name, accessMode);
}
// Log arguments and return value
LogPatchedFunction(RETURN_BPTR, result, "IDOS->Open(name='%s', accessMode=%ld)", name ? name : "NULL", accessMode, result);
return result;
}
or for int64 return:
// Wrapper function for patching GetFilePosition
static int64 Patched_GetFilePosition(struct DOSIFace *Self, BPTR fh) {
int64 result = 0;
// Call the original function
if (Original_GetFilePosition) {
result = Original_GetFilePosition(Self, fh);
}
// Log arguments and return value
LogPatchedFunction(RETURN_INT64, result, "IDOS->GetFilePosition(Self=0x%lx, fh=0x%lx)", (uint32)Self, fh);
return result;
}
And log it like:
uint32 VARARGS68K LogPatchedFunction(uint32 returnType, int64 returnValue, const char *format, ...) {
char buffer[512];
va_list args;
va_startlinear(args, format);
APTR data = va_getlinearva(args, APTR);
IExec->RawDoFmt(format, data, NULL, buffer);
if (returnType != RETURN_NONE) {
char returnBuffer[64];
switch (returnType) {
case RETURN_BPTR: {
// Use lower 32 bits for BPTR
int32 value = (int32)(returnValue & 0xFFFFFFFF);
IExec->RawDoFmt(" return=0x%lx\n", &value, NULL, returnBuffer);
break;
}
case RETURN_INT32: {
// Use lower 32 bits for int32
int32 value = (int32)(returnValue & 0xFFFFFFFF);
IExec->RawDoFmt(" return=%ld\n", &value, NULL, returnBuffer);
break;
}
case RETURN_LONG: {
// Use lower 32 bits for LONG
int32 value = (int32)(returnValue & 0xFFFFFFFF);
IExec->RawDoFmt(" return=%ld\n", &value, NULL, returnBuffer);
break;
}
case RETURN_INT64: {
// Use full int64
IExec->RawDoFmt(" return=%lld\n", &returnValue, NULL, returnBuffer);
break;
}
case RETURN_BOOL: {
// Use lower 32 bits for BOOL (normalized to 0 or 1)
int32 value = (int32)(returnValue & 0xFFFFFFFF);
IExec->RawDoFmt(" return=%ld\n", &value, NULL, returnBuffer);
break;
}
default:
returnBuffer[0] = '\0'; // No return value if type is unknown
break;
}
// Append return value to the log
IExec->DebugPrintF("%s%s", buffer, returnBuffer);
} else {
IExec->DebugPrintF("%s", buffer);
}
va_end(args);
return 0;
}
But i can just call original function before the log, so to have result as well at the same time...
I wrote my original comment back before you switched to IPC. Now that you're copying all the arguments anyway, you're right, it's no big deal to wait until the call returns so you can report the return code as well. Snork is getting better and better!
I wrote my original comment back before you switched to IPC. Now that you're copying all the arguments anyway, you're right, it's no big deal to wait until the call returns so you can report the return code as well. Snork is getting better and better!
In small tests even without IPC taking result and then print it with arguments works, but yea, after calling original function arguments can easy die, so copy need it..
@msteed Design question: Should we allow any format specifier for any argument, or restrict them to a specific set? If we allow any, users could easily use %s for a decimal value and create a mess (though we can already detect invalid strings and limit their size). Allowing all format specifiers feels logical: users can choose what they want—pointers, hex, decimal, strings, etc.—but it requires caution. Alternatively, we could limit specifiers to match the types expected by the patched function. But then, what's the point of setting them if they're fixed to the function's expected types? In that case, we wouldn't need scripting—just a list of functions with the ability to mark unneeded args with "-".
IMHO, we should support the same range of printf-style format specifiers and let users decide what they want to see. What do you think?
Also, I'm unsure whether we should provide an option to enable/disable "task/process" info and "return type," or just always show the task name/address and return type by default and no worry about disabling. What do you think?
Should we allow any format specifier for any argument, or restrict them to a specific set?
One argument -- the one you're leaning towards -- is that it's a programmer's tool, and programmers are supposed to know what they're doing, so you should let them do what they want with it, even if it doesn't seem to make sense.
Another option -- one I'm kind of leaning towards -- would be to restrict the format specifiers to ones that make sense for the argument type. A string pointer, for instance, might be displayed as a hex (or even decimal) value or as a string, but not as a tag list. A decimal value might be displayed as decimal or as hex, but not as a string or a tag list. A tag list pointer could also be displayed as a hex value, but not as a string.
The downside of this approach is that the code would have to know what the type of every argument of every function is, so it would know which specifiers to allow. That's more work and larger code size, especially as more libraries are added.
Quote:
Also, I'm unsure whether we should provide an option to enable/disable "task/process" info and "return type," or just always show the task name/address and return type by default and no worry about disabling.
If you're using the option to snoop on one specific program then there's no real need to show the program's name, as you already know what program it is. But if you're not using that option, it seems like you'd want to see what program was making the call, otherwise you wouldn't know whether it was the program you're trying to debug, or some other program.
As far as the return value, it would be consistent to let you disable it with '-' if you're not interested, just as you can any of the arguments.