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

taskfunc -> return and out params don't work correctly #217

Closed
Reid115 opened this issue Oct 25, 2022 · 3 comments · Fixed by #415
Closed

taskfunc -> return and out params don't work correctly #217

Reid115 opened this issue Oct 25, 2022 · 3 comments · Fixed by #415
Labels

Comments

@Reid115
Copy link

Reid115 commented Oct 25, 2022

The TCM manual states that, when calling taskfuncs that have a return value, assigning that value to something is optional. However, if you don't assign it to something, for some reason none of the parameters in the call will be read (and they will all be set to 0). Example:

on init
    message("")
    tcm.init(1000)
    declare ui_switch swch
    declare i
    declare val
end on

taskfunc tf(num) -> return
    message(num)
    return := 456
end taskfunc

on ui_control (swch)
    //i := tf(123)
    tf(123)
end on

When you click the switch, it will print out "0". If you replace the tf call line with the line above it that sets the return value to i, you will see the correct "123" printed out.

Another problem is with out parameters. I could be wrong, but I assume that out params are supposed to be initialized to zero. But they're actually remembering whatever the ending value was the last time the taskfunc was called. Example:

on init
    message("")
    tcm.init(1000)
    declare ui_switch swch
    declare val
end on

taskfunc tf(out return) // return does NOT reinitialize to 0
    inc(return)
end taskfunc

on ui_control (swch)
    val := 0 // not necessary, just whittling down the problem
    tf(val)
    message(val)
end on

If you press the switch again and again, the numbers being printed out will be 1, 2, 3, 4, 5, etc... whereas it should keep printing out 1. Perhaps out params are only supposed to be assigned to, I'm not sure.

@Reid115
Copy link
Author

Reid115 commented Oct 28, 2022

I dug into the return issue a little further and basically this is what the compiled code looks like:

function tf
%p[$sp-3] := $fp
$fp := $sp-3
$sp := $fp
message(%p[$fp+1])
%p[$fp+2] := 456
$sp := $fp
$fp := %p[$fp]
$sp := $sp+3
end function

// the CB that assigns the return
on ui_control($swch)
%p[$sp-2] := 123
call tf
$i := %p[$sp-1]
end on

// the CB that doesn't assign the return
on ui_control($swch)
%p[$sp-1] := 123 // should be sp - 2, not sp - 1
call tf
end on

The tf function is the same for both, it's just the caller that changes. Essentially what is happening is that 2 spots in the frame are being allocated -- first, one spot (sp - 1) for the return value, and second, one spot (sp - 2) for the argument. In the CB that doesn't assign the return value, it thinks "oh I don't need two slots because I don't care about the return value, I just need 1 slot for the argument", so it puts the argument in sp - 1 instead of sp - 2. But the function doesn't mimic this behavior, and still tries to read the argument from sp - 2, thus getting a 0 because nothing was put there. So the solution is to ensure that the structure of the frame doesn't change, and a spot for the return value is still allocated, regardless if it will end up being used. Not sure what would need to change in the repository for that to happen.

@mkruselj
Copy link
Collaborator

mkruselj commented Dec 2, 2023

@Reid115 I may have a fix for the first issue that you reported here (the return value not being used messing with stack pointer offset). However I am unsure what's the impact on existing scripts out there, so this would require a lot of testing.

The diff goes like this:

In ksp_compiler.py, navigate to getTaskFuncCallPrologueAndEpilogue() function, and replace these lines:

        for i, param in enumerate(parameters):
            idx = len(parameters)-i

with these:

        # "virtually" increase parameter count so that if taskfunc has a return value,
        # but upon calling is not assigned to a variable to store the return into,
        # it would still use the correct stack pointer offset.
        # See issue #217 on GitHub!
        if func.return_value is not None and not assign_stmt_lhs:
            num_params = num_params + 1

        for i, param in enumerate(parameters):
            idx = num_params - i

See what happens!

@mkruselj
Copy link
Collaborator

mkruselj commented Dec 2, 2023

As for the other issue you outlined, I am inclined to agree with your assumption:

Perhaps out params are only supposed to be assigned to, I'm not sure.

Yeah they are only supposed to be assigned to. out parameters are basically just a way to have multiple return values.

If you wanted to do what you're doing in that second example you posted, you're supposed to use the var qualifier, not out (TCM user guide mentions this specifically at the bottom of page 8: "However, when parameters are used both as input and output (values that are altered by the function), you need to use the var qualifier.").

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

Successfully merging a pull request may close this issue.

2 participants