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 variadics don't work with non-trivial register-only types. #1941

Closed
elhigu opened this issue Mar 13, 2024 · 4 comments
Closed

[BUG]: Mojo variadics don't work with non-trivial register-only types. #1941

elhigu opened this issue Mar 13, 2024 · 4 comments
Labels
bug Something isn't working mojo Issues that are related to mojo mojo-python-interop mojo-repo Tag all issues with this label

Comments

@elhigu
Copy link

elhigu commented Mar 13, 2024

Bug description

Application crashes as soon as I try to do a.values()[0]

I would expect either working code or compile time error for using brackets in this case. Actually now that I think I might be confused with bracket getitem and bracket compile time parameters... so feel free to close if was complete user error.

Steps to reproduce

Pretty minimal example code included:

        (venv) mlepisto@stevetop:~/projects/learn-machine$ cat test.mojo 
        from python import Python 
        
        def main():
          var json = Python.import_module("json")
          a = json.loads('{"a": 1}')
          print(a.values())
          print(a.values()[0]) 
        (venv) mlepisto@stevetop:~/projects/learn-machine$ mojo run test.mojo 
        dict_values([1])
        [15228:15228:20240313,030428.656443:ERROR elf_dynamic_array_reader.h:64] tag not found
        [15228:15228:20240313,030428.658258:ERROR file_io_posix.cc:144] open /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq: No such file or directory (2)
        [15228:15228:20240313,030428.658285:ERROR file_io_posix.cc:144] open /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq: No such file or directory (2)
        Please submit a bug report to https://github.com/modularml/mojo/issues and include the crash backtrace along with all the relevant source codes.
        Stack dump:
        0.      Program arguments: mojo run test.mojo
        Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
        0  mojo                0x000055b953a9fe47
        1  mojo                0x000055b953a9da1e
        2  mojo                0x000055b953aa051f
        3  libc.so.6           0x00007fafbf6f5520
        4  libpython3.9.so.1.0 0x00007fafa0333e7a _PyObject_Call + 26
        5  libpython3.9.so.1.0 0x00007faf2004b34a _PyObject_Call + 18446744071559017706
        Segmentation fault
        (venv) mlepisto@stevetop:~/projects/learn-machine$ mojo --version
        mojo 24.1.0 (55ec12d6)
        (venv) mlepisto@stevetop:~/projects/learn-machine$

System information

Running in WSL2 (ubuntu22)
mojo 24.1.0 (55ec12d6)
modular 0.5.1 (1b608e3d)
@elhigu elhigu added bug Something isn't working mojo Issues that are related to mojo labels Mar 13, 2024
@elhigu
Copy link
Author

elhigu commented Mar 13, 2024

Tried also iterating over that, which fails to iterator not being mutable or something like that.

(venv) mlepisto@stevetop:~/projects/learn-machine$ cat test.mojo 
from python import Python

fn main() raises:
    var json = Python.import_module("json")
    var a = json.loads('{"a": 1, "b": 2}')
    print(a.values())
    for v in a.values():
        print(v)
(venv) mlepisto@stevetop:~/projects/learn-machine$ mojo run test.mojo 
/home/mlepisto/projects/learn-machine/test.mojo:8:22: error: invalid call to '__iter__': argument #0 must be mutable in order to pass as a by-ref argument
    for v in a.values():
             ~~~~~~~~^~
/home/mlepisto/projects/learn-machine/test.mojo:1:1: note: function declared here
from python import Python
^
mojo: error: failed to parse the provided Mojo

@JoeLoser
Copy link
Collaborator

Your second example works fine on top-of-tree and will work in 24.2:

from python import Python

fn main() raises:
    var json = Python.import_module("json")
    var a = json.loads('{"a": 1, "b": 2}')
    print(a.values())
    for v in a.values():
        print(v)

It prints:

dict_values([1, 2])
1
2

Your first example:

from python import Python 
        
def main():
    var json = Python.import_module("json")
    a = json.loads('{"a": 1}')
    print(a.values())
    print(a.values()[0]) 

crashes on the subscript operator as you mentioned. We should fix that to give a better error message. The print(a.values()) prints:

dict_values([1])

as expected

@ematejska ematejska added mojo-python-interop mojo-lang Tag for all issues related to language. and removed mojo-python-interop mojo-lang Tag for all issues related to language. labels Mar 21, 2024
@lattner lattner changed the title [BUG]: Runtime crash when trying to access dict_values() element [BUG]: Mojo variadics don't work with non-trivial register-only types. Apr 14, 2024
@lattner
Copy link
Collaborator

lattner commented Apr 14, 2024

@akirchhoff-modular helped track this down - this is caused by register-only types being handled by VariadicList instead of VariadicListMem, so their copy constructors don't get run correctly. There is a big migration to move the variadic list representation over to this new form, which can hopefully help, but I'm not exactly sure when it will converge

@lattner
Copy link
Collaborator

lattner commented Apr 14, 2024

This is fixed in the next update, thank you for reporting this!

patrickdoc pushed a commit that referenced this issue May 2, 2024
VariadicList is implemented with AnyRegType so it can't invoke
copy constructors.  This means we can't use it for non-trivial types
like PythonObject.  Switch variadics of non-trivial register type to
use VariadicListMem instead: we'd eventually like to remove
VariadicList but are not quite ready for that.

This fixes variadics with things like PythonObject and
Fixes #37362 and Fixes #1941

MODULAR_ORIG_COMMIT_REV_ID: fd3cdc841773c009ccce94110e722961fa2a1b82
@ematejska ematejska added the mojo-repo Tag all issues with this label label May 6, 2024
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 Issues that are related to mojo mojo-python-interop mojo-repo Tag all issues with this label
Projects
None yet
Development

No branches or pull requests

4 participants