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

JIT in OTP 27-RC2 passes wrong value to NIF in aarch64 #8433

Closed
alexandrejbr opened this issue Apr 29, 2024 · 8 comments · Fixed by #8435
Closed

JIT in OTP 27-RC2 passes wrong value to NIF in aarch64 #8433

alexandrejbr opened this issue Apr 29, 2024 · 8 comments · Fixed by #8435
Assignees
Labels
bug Issue is reported as a bug team:VM Assigned to OTP team VM

Comments

@alexandrejbr
Copy link

alexandrejbr commented Apr 29, 2024

Describe the bug
We are observing consistently in our code what seems like the wrong value is being picked up to pass to a function. This is seen only with JIT and on aarch64 (both Linux and Darwin) in OTP 27. OTP 26.2.3 works with no problems.

The code has the following structure:

f(Atom, {ListOfAtoms, OtherAtom}, Record) ->
    (... Pure functions are called here) 
    YetAnotherAtom = (some value obtained from calling pure functions) 
    [OtherAtom |
     case Atom of
         atomA ->
             [OtherAtom, f2(OtherAtom, YetAnotherAtom)];
         atomB ->
             ['', 0]
     end].

f is being called with f(atomA, {[a1], c1}, #record{...}) and we have added an io:format in f2 and we see that OtherAtom is there atomA while YetAnotherAtom has the correct value.

If we rewrite the above code to be like the following the bug disappears, and also disappears if we add an io:format or put(something,something) immediately after the line that creates YetAnotherAtom:

f(Atom, {ListOfAtoms, OtherAtom}, Record) ->
    (... Pure functions are called here) 
    YetAnotherAtom = (some value obtained from calling pure functions) 
    F2 = f2(OtherAtom, YetAnotherAtom),
    [OtherAtom |
     case Atom of
         atomA ->
             [OtherAtom, F2];
         atomB ->
             ['', 0]
     end].

To Reproduce
Unfortunately, I didn't manage to isolate the issue so I can't really provide a reproduction. Even just by exporting the function and calling it from the shell doesn't trigger the bug.

Expected behavior
The first argument of the f2 function should be taken from OtherAtom and not Atom.

Affected versions
OTP 27, we tested with RC2 and RC3 and both behave the same.

Additional context
I understand that this is most not enough information for you to investigate the issue, but maybe you can give me ideas on what to get you so you can understand the issue better.

@alexandrejbr alexandrejbr added the bug Issue is reported as a bug label Apr 29, 2024
@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Apr 29, 2024
@jhogberg
Copy link
Contributor

Thanks for your report! If you can't share minimized code to reproduce the issue, can you try using git bisect to find the commit that introduced the error?

@jhogberg jhogberg self-assigned this Apr 29, 2024
@alexandrejbr
Copy link
Author

@jhogberg I think it's these: a16397d and 06d402a

I didn't do bisect since we have some patches on top and complicates things. I did instead some reverts of commits that look suspicious and it was when I reverted those 2 that the problem disappeared.

@jhogberg
Copy link
Contributor

Thanks, that's a humongous diff though 😅

Can you print out the result of beam_disasm:file("path/to/module.beam") to a file, and post the nearest handful instructions around the call to f2 (as many as you're comfortable sharing)?

@alexandrejbr
Copy link
Author

alexandrejbr commented Apr 29, 2024

Here you have the full function, I renamed things a bit, but all should be good.

This version has the bug.

            {function,f,3,1233,
                      [{line,857},
                       {label,1232},
                       {func_info,{atom,function_module},{atom,f},3},
                       {label,1233},
                       {test,is_tuple,{f,1232},[{x,1}]},
                       {test,test_arity,{f,1232},[{x,1},2]},
                       {get_tuple_element,{x,1},1,{x,3}},
                       {test,is_tagged_tuple,{f,1235},[{x,2},23,{atom,cs}]},
                       {allocate,6,4},
                       {init_yregs,{list,[{y,0}]}},
                       {move,{x,3},{y,2}},
                       {move,{x,2},{y,3}},
                       {move,{x,1},{y,4}},
                       {move,{x,0},{y,5}},
                       {get_tuple_element,{x,2},1,{x,0}},
                       {get_tuple_element,{x,2},3,{y,1}},
                       {move,{y,1},{x,1}},
                       {line,858},
                       {call_ext,2,{extfunc,cccs,somefunction,2}},
                       {get_tuple_element,{y,4},0,{y,4}},
                       {move,{x,0},{y,0}},
                       {move,{y,4},{x,0}},
                       {line,859},
                       {call,1,{function_module,cp,1}},
                       {move,{y,0},{x,1}},
                       {init_yregs,{list,[{y,0}]}},
                       {call_ext,2,{extfunc,cccs,someotherfunction,2}},
                       {swap,{y,4},{x,0}},
                       {line,860},
                       {call,1,{function_module,cp,1}},
                       {get_tuple_element,{y,3},1,{x,1}},
                       {init_yregs,{list,[{y,3}]}},
                       {call_ext,2,{extfunc,erlang,'++',2}},
                       {move,{y,1},{x,1}},
                       {init_yregs,{list,[{y,1}]}},
                       {call_ext,2,{extfunc,cccs,somefunction,2}},
                       {test,is_eq_exact,{f,1234},[{y,5},{atom,set_case}]},
                       {move,{x,0},{x,1}},
                       {move,{y,2},{y,5}},
                       {trim,4,2},
                       {move,{y,1},{x,0}},
                       {line,861},
                       {call,2,{function_module,f2,2}},
                       {test_heap,6,1},
                       {put_list,{x,0},nil,{x,0}},
                       {put_list,{y,1},{x,0},{x,0}},
                       {put_list,{y,0},{x,0},{x,0}},
                       {deallocate,2},
                       return,
                       {label,1234},
                       {test_heap,2,0},
                       {put_list,{y,4},{literal,['',0]},{x,0}},
                       {deallocate,6},
                       return,
                       {label,1235},
                       {line,862},
                       {badrecord,{x,2}}]},

This version doesn't cause the bug (it's the one with the rewrite I have above):

            {function,f,3,1233,
                      [{line,857},
                       {label,1232},
                       {func_info,{atom,function_module},{atom,f},3},
                       {label,1233},
                       {test,is_tuple,{f,1232},[{x,1}]},
                       {test,test_arity,{f,1232},[{x,1},2]},
                       {get_tuple_element,{x,1},1,{x,3}},
                       {test,is_tagged_tuple,{f,1235},[{x,2},23,{atom,cs}]},
                       {allocate,6,4},
                       {init_yregs,{list,[{y,0}]}},
                       {move,{x,3},{y,2}},
                       {move,{x,2},{y,3}},
                       {move,{x,1},{y,4}},
                       {move,{x,0},{y,5}},
                       {get_tuple_element,{x,2},1,{x,0}},
                       {get_tuple_element,{x,2},3,{y,1}},
                       {move,{y,1},{x,1}},
                       {line,858},
                       {call_ext,2,{extfunc,cccs,somefunction,2}},
                       {get_tuple_element,{y,4},0,{y,4}},
                       {move,{x,0},{y,0}},
                       {move,{y,4},{x,0}},
                       {line,859},
                       {call,1,{function_module,cp,1}},
                       {move,{y,0},{x,1}},
                       {init_yregs,{list,[{y,0}]}},
                       {call_ext,2,{extfunc,cccs,someotherfunction,2}},
                       {swap,{y,4},{x,0}},
                       {line,860},
                       {call,1,{function_module,cp,1}},
                       {get_tuple_element,{y,3},1,{x,1}},
                       {init_yregs,{list,[{y,3}]}},
                       {call_ext,2,{extfunc,erlang,'++',2}},
                       {move,{y,1},{x,1}},
                       {init_yregs,{list,[{y,1}]}},
                       {call_ext,2,{extfunc,cccs,somefunction,2}},
                       {move,{x,0},{x,1}},
                       {move,{y,2},{x,0}},
                       {line,861},
                       {call,2,{function_module,f2,2}},
                       {test,is_eq_exact,{f,1234},[{y,5},{atom,set_case}]},
                       {test_heap,6,1},
                       {put_list,{x,0},nil,{x,0}},
                       {put_list,{y,2},{x,0},{x,0}},
                       {put_list,{y,4},{x,0},{x,0}},
                       {deallocate,6},
                       return,
                       {label,1234},
                       {test_heap,2,0},
                       {put_list,{y,4},{literal,['',0]},{x,0}},
                       {deallocate,6},
                       return,
                       {label,1235},
                       {line,862},
                       {badrecord,{x,2}}]},

@jhogberg
Copy link
Contributor

Thanks, does atomA correspond to set_case?

@alexandrejbr
Copy link
Author

Yes

@jhogberg
Copy link
Contributor

Thanks, can you try out https://github.com/jhogberg/otp/tree/john/jit/fix-invalid-reg-cache/GH-8433 and see if that fixes the issue?

@alexandrejbr
Copy link
Author

Yes it does! Thank you for looking into this so fast!

jhogberg added a commit that referenced this issue May 2, 2024
…H-8433

arm: Add missing clobber of register cache when SUPER_TMP is used
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is reported as a bug team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants