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

[BUG] [MOJO] Issue with string addition #3335

Closed
MVPavan opened this issue Jul 30, 2024 · 6 comments
Closed

[BUG] [MOJO] Issue with string addition #3335

MVPavan opened this issue Jul 30, 2024 · 6 comments
Labels
bug Something isn't working mojo-repo Tag all issues with this label

Comments

@MVPavan
Copy link

MVPavan commented Jul 30, 2024

Bug description

Observe image 1 and 2, as you can see before result = result "]" executed, result value was [0,1,2. But after it's executed I was expecting [0,1,2,] but got [0,1, [0].

Image 1:
image 1

Image 2:
image 2

Steps to reproduce

Code to replicate

from memory.unsafe_pointer import (
    UnsafePointer,
    move_pointee,
    move_from_pointee,
    initialize_pointee_copy,
    initialize_pointee_move,
    destroy_pointee
)
from testing import assert_true


trait QType(CollectionElement, EqualityComparable, RepresentableCollectionElement):
    pass

# enqueue, dequeue, peek, is_empty, size
struct MyQueueArr[T:QType]:
    var data:UnsafePointer[T]
    var size:Int
    var capacity:Int

    fn __init__(inout self,):
        self.data = UnsafePointer[T]()
        self.size = 0
        self.capacity = 0
    
    @always_inline
    fn __del__(owned self):
        for i in range(self.size):
            destroy_pointee(self.data   i)
        if self.data:
            self.data.free()

    fn enqueue(inout self, owned value:T):
        if self.size>=self.capacity:
            self._realloc(max(1,self.capacity*2))
        initialize_pointee_move(self.data self.size, value^)
        self.size  = 1

    fn dequeue(inout self) raises-> T:
        debug_assert(self.size>0, "Queue cannot be empty for dequeuing") 
        var value = move_from_pointee(self.data)
        if self.data:
            self.data.free()
        self.data = self.data 1
        self.size -= 1
        return value
    
    fn peek(self) -> T:
        return self.data[]
    
    fn len(self) -> Int:
        return self.size
    
    fn is_empty(self) -> Bool:
        return not self.size>0

    fn __copyinit__(inout self, existing: Self):
        self = Self()
        for i in range(existing.size):
            self.enqueue((existing.data i)[])

    fn __moveinit__(inout self, owned existing: Self):
        self.data = existing.data
        self.size = existing.size
        self.capacity = existing.capacity

    fn __len__(self) -> Int:
        return self.size
    
    fn __str__(self) -> String:
        var result:String = "[ "
        for i in range(self.size):
            result  = repr((self.data i)[])   ", "
        result = result   " ]"
        return result
    
    fn __repr__(self)->String:
        return self.__str__()
    
    fn _realloc(inout self, new_capcity:Int):
        var new_data = UnsafePointer[T].alloc(new_capcity)
        for i in range(self.size):
            move_pointee(src=self.data i, dst=new_data i)
        
        if self.data:
            self.data.free()
        self.data = new_data
        self.capacity = new_capcity

System information

OS: Ubuntu 22
Mojo Version: mojo 24.4.0 (2cb57382)
@MVPavan MVPavan added bug Something isn't working mojo-repo Tag all issues with this label labels Jul 30, 2024
@soraros
Copy link
Contributor

soraros commented Jul 30, 2024

I think it's UB triggered by this function:

    fn dequeue(inout self) raises -> T:
        debug_assert(self.size > 0, "Queue cannot be empty for dequeuing") 
        var value = move_from_pointee(self.data)
        if self.data:
            self.data.free()  # the whole block is freed here, why?
        self.data = self.data   1
        self.size -= 1
        return value

@MVPavan
Copy link
Author

MVPavan commented Jul 31, 2024

Thanks for pointing it out, yeah it's a mistake, but that was not the issue here. And before calling str I haven't called dequeue.

Surprisingly today I was not facing this issue, the system restart cleared it. I am not sure what the reason was, just before executing the last second line it was fine, and then it messed up as shown in the picture, and dequeue was not called in the entire script.

@MVPavan MVPavan closed this as completed Jul 31, 2024
@soraros
Copy link
Contributor

soraros commented Jul 31, 2024

I bisected your test file to dequeue, and it does look like UB. Anyways, please include the whole repro script next time.

@MVPavan
Copy link
Author

MVPavan commented Jul 31, 2024

Okay, so it was happening for 'test_queue_expansion' which doesn't include dequeue.

@soraros
Copy link
Contributor

soraros commented Jul 31, 2024

Okay, so it was happening for 'test_queue_expansion' which doesn't include dequeue.

That particular function might not include dequeue. However, if dequeue is called in any part of your script (maybe in another test), UB can happen in its typical "spooky action at a distance" fashion. The same can also be observed in my repro.

Of course, if you have a different repro without the said memory safety problems, my analysis would have been wrong. In that case, please reopen.

Repro

NB: I removed some null check on the pointer since free is no-op on them.

from memory.unsafe_pointer import (
    UnsafePointer,
    move_pointee,
    move_from_pointee,
    initialize_pointee_copy,
    initialize_pointee_move,
    destroy_pointee,
)
from testing import assert_true, assert_false


trait QType(
    CollectionElement,
    EqualityComparable,
    RepresentableCollectionElement,
):
    pass


struct Queue[T: QType]:
    var data: UnsafePointer[T]
    var size: Int
    var capacity: Int

    fn __init__(inout self):
        self.data = UnsafePointer[T]()
        self.size = 0
        self.capacity = 0

    fn __copyinit__(inout self, existing: Self):
        self = Self()
        for i in range(len(existing)):
            self.enqueue(existing.data[i])

    fn __moveinit__(inout self, owned existing: Self):
        self.data = existing.data
        self.size = existing.size
        self.capacity = existing.capacity

    fn __del__(owned self):
        for i in range(len(self)):
            destroy_pointee(self.data   i)
        self.data.free()  # free will be called on an advanced pointer

    fn enqueue(inout self, owned value: T):
        if self.size >= self.capacity:
            self._realloc(max(1, self.capacity * 2))
        initialize_pointee_move(self.data   self.size, value^)
        self.size  = 1

    fn dequeue(inout self) raises -> T:
        if not self:
            raise "Dequeuing empty queue, aborting..."

        var value = move_from_pointee(self.data)
        if self.data:
            self.data.free()
        self.data = self.data   1  # this is also troublesome
        self.size -= 1
        return value^

    fn __len__(self) -> Int:
        return self.size

    fn __bool__(self) -> Bool:
        return len(self) > 0

    fn __str__(self) -> String:
        var result: String = "[ "
        for i in range(len(self)):
            result  = repr(self.data[i])   ", "
        result  = "]"
        return result

    fn __repr__(self) -> String:
        return str(self)

    fn _realloc(inout self, new_capcity: Int):
        var new_data = UnsafePointer[T].alloc(new_capcity)
        for i in range(self.size):
            move_pointee(src=self.data   i, dst=new_data   i)
        self.data.free()
        self.data = new_data
        self.capacity = new_capcity


def main():
    var p = Queue[Int]()
    p.enqueue(0)
    p.enqueue(1)
    _ = p.dequeue()

    var q = Queue[Int]()
    for i in range(10):
        q.enqueue(i)
    print(q)  # prints [ 0, 1, [ 03, 4, 5, 6, 7, 8, 9, ]

@MVPavan
Copy link
Author

MVPavan commented Aug 1, 2024

True, dequeu called in different test function is affecting it. Thanks for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working mojo-repo Tag all issues with this label
Projects
None yet
Development

No branches or pull requests

2 participants