odin-lang / Odin

Odin Programming Language
https://odin-lang.org
BSD 3-Clause "New" or "Revised" License
7.04k stars 628 forks source link

Clarification of Odin's return by value calling convention behavior #2657

Open JesseRMeyer opened 1 year ago

JesseRMeyer commented 1 year ago

What should the value of game.bob^ be?

Game is larger than a register size. According to Odin's calling convention, the callee should reserve the stack space, and assignments in this case should be made directly ("copy elision"). But that does not happen. Apparently the callee and the caller reserve space, and at the function end, there is a copy, which causes the pointer to refer to stale stack memory.

package main

import "core:fmt"

Game :: struct {
    bob: ^u64,
    alice: u64,
}

gameInit :: proc() -> (game: Game) {//(game: ^Game) {
    game.bob = &game.alice

    game.alice = 5;

    return
}

main :: proc() {
    //game: Game
    //gameInit(&game)
    game := gameInit()

    fmt.println(game.bob^)
}
Sanian-Creations commented 1 year ago

I'm not sure where you get your understanding from.

Copy elison is an optimization, not expected behaviour. Named returns are treated like regular local variables, only with optimizations turned on will the compiler even consider copy elison. But even with optimized builds, it may or may not apply it. Don't go writing code that expects this to happen.

Here's a little snippet that returns a struct and it checks whether its address is the same in the caller as in the callee.

package main
import "core:fmt"

Thing :: struct {
    self_ptr : ^Thing,
    padding  : [16]u8 // make bigger to increase chance of optimization
}

main :: proc() {
    fmt.println("size of Thing:", size_of(Thing))
    caller()
}

caller :: proc() {
    t := callee()
    fmt.printf("addr in callee: %#p\n", t.self_ptr)
    fmt.printf("addr in caller: %#p\n", &t)
    fmt.println(t.self_ptr == &t ? "used" : "did not use", "copy elison")
}

callee :: proc() -> (retval: Thing) {
    retval.self_ptr = &retval
    // printing retval here may also trigger the optimization, even if the struct is not that big.
    return
}

Run with odin run example.odin -file -o:speed (without -o:speed it never uses copy elison)

What you'll find is that optimization is unpredictable. The code in your own example, for instance, doesn't prints 0 if that is the whole file, but when I renamed the main function and called it from some other procs I was messing with, it printed 5 like you expected it to. There are effectively infinite factors that could influence how the compiler optimizes, you cannot rely on it.

I don't think this is an issue with the language.

JesseRMeyer commented 1 year ago

Odin's calling convention for return values greater than register size is (essentially):

Callee reserve stack memory for return value Call func() Func() writes directly to reserve stack

This is a form of copy elision.

But that is not what I think is happening here. Instead: Callee reserve stack memory for return value Call func() Func() reserves stack memory for return value Func() writes to its local stack Func() copies local reserve stack to callee's reserve.

So, if I'm right, the current implementation does not actually honor Odin's calling convention, and it's causing bugs.

Sanian-Creations commented 1 year ago

You're getting your callee vs caller messed up. Callee is the one getting called, not the one doing the calling.

foo() {     // caller (does the calling)
    bar()   // callee (gets called)
}

Regardless, where does it say that Odin works like this for values greater than register size? If you have a source I'd like to see it, I can't find any such information myself.

All I know is what the Overview page on the website says, which is this:

It [the odin calling convention] is the same as cdecl but passes an implicit context pointer on each call.

And as far as I know cdecl doesn't make any such promises, it is compiler dependent.

JesseRMeyer commented 1 year ago

Thanks.

The source is @gingerBill, but the specifics remain in question, which is why this issue is asking for clarification.