Reference counting issue with GetElementPtr

[JukkaL]

Consider this code:

from typing import List

def f() -> int:
    a: List[int] = []
    return len(a)

It gets compiled into this (incorrect) code:

def f():
    r0, a :: list
    r1 :: ptr
    r2 :: int64
    r3 :: short_int
    r4 :: int
L0:
    r0 = []
    if is_error(r0) goto L2 (error at f:4) else goto L1
L1:
    a = r0
    r1 = get_element_ptr a ob_size :: PyVarObject
    dec_ref a  # <<---- Free here
    r2 = load_mem r1 :: int64*   # <<---- Use after free
    r3 = r2 << 1
    return r3
L2:
    r4 = <error> :: int
    return r4

This results in segfaults in some cases, but simple examples may work correctly most of the time.

[JukkaL]

The cleanest way to fix might be to somehow keep track of the fact that r1 contains a reference to a, so a is still alive until after load_mem (aliasing). However, this might be tricky to implement in full generality (and potentially slow). What if a gets assigned a new value while we hold a reference to an item, for example? Maybe we can make some simplifying assumptions here (these are before reference counting transform):

• We can't assign to original register (e.g. a) during the lifetime of an element pointer (e.g. r1).
• We can't reassign to the element pointer register.

Now the lifetime of a would simply include the lifetimes of all element pointer registers.

We'll also need to consider any pointer derived from r1 via pointer arithmetic to be an alias to a (once we have uses of pointer arithmetic).

Another option would be to merge get_element_ptr and load_mem into a macro op that also reads an element, potentially translating these to lower-level ops after the reference counting transform. This seems kind of ugly and also restricts the expressive power of the IR.

[msullivan]

Oh, this is nasty. If it's a release blocker, probably revert if it isn't too disruptive?

One quick-and-dirty approach would be to add a no-op keep-alive op, and generate it after the load_mem to "pin" a.

So we'd generate

    r1 = get_element_ptr a ob_size :: PyVarObject
    r2 = load_mem r1 :: int64*
    unpin a  # Keeps a alive until this point

[JukkaL]

If it's a release blocker, probably revert if it isn't too disruptive?

If we can't get fix out today, I'm probably going to create a branch form an earlier commit and cherry pick some of the following commits, as there aren't many of them.

One quick-and-dirty approach would be to add a no-op keep-alive op

That's a clever idea! It feels a bit hacky, so I'm still trying to update liveness analysis to somehow do the right thing here, but this seems better than merging the ops.

[JukkaL]

Here's my current idea about how to fix liveness analysis:

• Create a liveness dependency map via a simple pass over the IR (e.g. r1 -> a in the original example).
• Each op that accesses one of the map keys also implicitly accesses the all the values that can be reached from the key via a graph search, thus keeping them alive.

[JukkaL]

My latest idea is to use a variant of the unpin approach, for simplicity. Since this only affects LoadMem, I'm planning to add an attribute to LoadMem that contains the base object from which we are reading memory. This is only needed for liveness analysis.