Fix ASR verify pass error while using Interactive

[Vipul-Cariappa]

Code to replicate:

❯ lpython  
>>> print(3 % 2)
1
>>> print(0)
ASR verify pass error: Function __lpython_overloaded_0___lpython_str_center doesn't depend on __lpython_overloaded_2___mod but is found in its dependency list.
Internal Compiler Error: Unhandled exception
Traceback (most recent call last):
  Binary file "/home/vipul/Workspace/python/lpython/src/bin/lpython", in _start()
  Binary file "/lib64/libc.so.6", in __libc_start_main_alias_2()
  Binary file "/lib64/libc.so.6", in __libc_start_call_main()
  File "/home/vipul/Workspace/python/lpython/src/bin/lpython.cpp", line 1990, in main()
    return interactive_python_repl(lpython_pass_manager, compiler_options, arg_v);
  File "/home/vipul/Workspace/python/lpython/src/bin/lpython.cpp", line 852, in interactive_python_repl()
    res = fe.evaluate(code_string, verbose, lm, pass_manager, diagnostics);
  File "/home/vipul/Workspace/python/lpython/src/lpython/python_evaluator.cpp", line 74, in LCompilers::PythonCompiler::evaluate(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool, LCompilers::LocationManager&, LCompilers::PassManager&, LCompilers::diag::Diagnostics&)
    Result<ASR::TranslationUnit_t*> res2 = get_asr3(*ast, diagnostics, lm, true);
  File "/home/vipul/Workspace/python/lpython/src/lpython/python_evaluator.cpp", line 153, in LCompilers::PythonCompiler::get_asr3(LCompilers::LPython::AST::ast_t&, LCompilers::diag::Diagnostics&, LCompilers::LocationManager&, bool)
    compiler_options, true, "__main__", "", false, is_interactive ? eval_count : 0);
LCompilersException: Verify failed

---

I am skipping the verification of function with Interactive ABI. In the above example when line 1 is executed all the functions are verified. While executing the second line we call mark_all_variables_external where we change the ABI type of all the previously defined/declared functions to Interactive. Therefore we can skip their verification.

[Vipul-Cariappa]

cc @Shaikh-Ubaid

[Vipul-Cariappa]

I found something interesting. Trying this out in interactive:

❯ lpython
>>> print(3 % 2)
1
>>> 
ASR verify pass error: The Function::m_body should be null if abi set to Interactive
Internal Compiler Error: Unhandled exception
Traceback (most recent call last):
  Binary file "/home/vipul/Workspace/c/lpython/src/bin/lpython", in _start()
  Binary file "/lib64/libc.so.6", in __libc_start_main_alias_2()
  Binary file "/lib64/libc.so.6", in __libc_start_call_main()
  File "/home/vipul/Workspace/c/lpython/src/bin/lpython.cpp", line 1990, in main()
    return interactive_python_repl(lpython_pass_manager, compiler_options, arg_v);
  File "/home/vipul/Workspace/c/lpython/src/bin/lpython.cpp", line 852, in interactive_python_repl()
    res = fe.evaluate(code_string, verbose, lm, pass_manager, diagnostics);
  File "/home/vipul/Workspace/c/lpython/src/lpython/python_evaluator.cpp", line 92, in LCompilers::PythonCompiler::evaluate(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool, LCompilers::LocationManager&, LCompilers::PassManager&, LCompilers::diag::Diagnostics&)
    pass_manager, diagnostics, lm.files.back().in_filename);
  File "/home/vipul/Workspace/c/lpython/src/lpython/python_evaluator.cpp", line 194, in LCompilers::PythonCompiler::get_llvm3(LCompilers::ASR::TranslationUnit_t&, LCompilers::PassManager&, LCompilers::diag::Diagnostics&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)
    run_fn, infile);
  File "/home/vipul/Workspace/c/lpython/src/libasr/codegen/asr_to_llvm.cpp", line 9783, in LCompilers::asr_to_llvm(LCompilers::ASR::TranslationUnit_t&, LCompilers::diag::Diagnostics&, llvm::LLVMContext&, Allocator&, LCompilers::PassManager&, LCompilers::CompilerOptions&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)
    pass_manager.apply_passes(al, &asr, co.po, diagnostics);
  File "/home/vipul/Workspace/c/lpython/src/libasr/pass/pass_manager.h", line 311, in LCompilers::PassManager::apply_passes(Allocator&, LCompilers::ASR::TranslationUnit_t*, LCompilers::PassOptions&, LCompilers::diag::Diagnostics&)
    apply_passes(al, asr, _passes, pass_options, diagnostics);
LCompilersException: Verify failed in the pass: nested_vars

Looks like we missed clearing the body of some of the functions.

[certik]

I think the code that sets all functions to interactive is incorrect ---- it leaves dependencies in there which should also be removed. The functions just become an interface?

So that code has to be fixed, not removing ASR verify, which now hide the bugs in there.

The ASR is incorrect, because the dependencies of these functions are incorrect, and consequently other parts of our compiler will fail that assume the dependencies are correct.

In general, we don't change ASR verify(), unless we are absolutely sure the ASR verification is incorrect.

[certik]

P.S. is "Interactive" function always without a body --- an interface? If so, then this check should be added in addition to all the other checks.

[Shaikh-Ubaid]

P.S. is "Interactive" function always without a body --- an interface?

Yes, we do that here https://github.com/lfortran/lfortran/blob/d0882b59a1002efccc8532ee235f859271ef658e/src/libasr/asr_scopes.cpp#L39-L45

In the first run for each function, its abi is source. For the next run, previously declared functions are marked as abi interactive and their bodies are removed.

[Vipul-Cariappa]

In the first run for each function, its abi is source. For the next run, previously declared functions are marked as abi interactive and their bodies are removed.

If you look at this https://github.com/lcompilers/lpython/pull/2706#issuecomment-2120601841. To me, it looks like there is some kind of edge case where the body is not removed. Any thoughts?

[Vipul-Cariappa]

Please have a look at the new changes. I have undone the modifications to asr_verify and just updated the method in which we clear the body and dependencies of a function. For some reason, we treat intrinsic functions differently, and need to preserve their body and dependencies for asr_verify.

If these changes are acceptable, I can also update lfortran/lfortran#4006 with the same changes. Testing it locally, LLVM tests did pass.

---

EDITED: Please do not merge it yet. I was tinkering around and found some problems.

[certik]

Let's keep the ASR verify changes, those were good. Just remove "return".

[Vipul-Cariappa]

The reason I asked not to merge this yet is that this fix gives an error for the example @certik gave above in the interactive mode.

Code:

>>> pure function id(x) result(r)                                                                                         ┐
...     integer, intent(in) :: x                                                                                          │
...     integer :: r                                                                                                      │
...     r = x                                                                                                             │
... end function id                                                                                                 5,16  ┘
>>> subroutine sa(n, x)                                                                                                   ┐
...     integer, intent(in) :: n                                                                                          │
...     integer, intent(inout) :: x(id(n))                                                                                │
...                                                                                                                       │
...     integer :: i                                                                                                      │
...     do i = 1, size(x)                                                                                                 │
...         x(i) = x(i) + 1                                                                                               │
...     end do                                                                                                            │
... end subroutine sa                                                                                               9,18  ┘
>>> integer :: i                                                                                                    1,13  ]
ASR verify pass error: Function sa depends on id but isn't found in its dependency list.
 --> input:1:1 - 2:167
  |
1 |    integer :: i
  |    ^^^^^^^^^^^^...
...
  |
2 |    
  | ...^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ failed here

Note: Please report unclear, confusing or incorrect messages as bugs at
https://github.com/lfortran/lfortran/issues.

We get ASR verify pass error: Function sa depends on id but isn't found in its dependency list., ideally that should not be the behaviour.

---

I was trying the above code out in gfortran, and get the following errors.

program main
    implicit none
    integer :: l
    integer :: arr(10)
    integer :: x
    l = 10
    print *, "Before: "
    print *, arr
    call sa(l, arr)
    print *, "After: "
    print *, arr
contains

pure function id(x) result(r)
    integer, intent(in) :: x
    integer :: r
    r = x
end function id

subroutine sa(n, x)
    integer, intent(in) :: n
    integer, intent(inout) :: x(id(n))

    integer :: i
    do i = 1, size(x)
        x(i) = x(i) + 1
    end do
end subroutine sa

end program main

gfortran errors:

❯ gfortran ./e.f90 
./e.f90:22:32:

   22 |     integer, intent(inout) :: x(id(n))
      |                                1
Error: Specification function ‘id’ at (1) cannot be an internal function
./e.f90:22:32:

   22 |     integer, intent(inout) :: x(id(n))
      |                                1
Error: Specification function ‘id’ at (1) cannot be an internal function
./e.f90:22:32:

   22 |     integer, intent(inout) :: x(id(n))
      |                                1
Error: Specification function ‘id’ at (1) cannot be an internal function

I am new to fortran, if you could explain the meaning of these error messages and how to fix them, it would be helpful.

[Vipul-Cariappa]

The following change looks like the best option to me, if we want to verify the ASR of interactive ABI that has been verified in the previous interactive evaluation.

index 4fae6739e..759269885 100644
--- a/src/libasr/asr_scopes.cpp
+++ b/src/libasr/asr_scopes.cpp
@@ -40,8 +40,6 @@ void SymbolTable::mark_all_variables_external(Allocator &al) {
                 ASR::Function_t *v = ASR::down_cast<ASR::Function_t>(a.second);
                 ASR::FunctionType_t* v_func_type = ASR::down_cast<ASR::FunctionType_t>(v->m_function_signature);
                 v_func_type->m_abi = ASR::abiType::Interactive;
-                v->m_body = nullptr;
-                v->n_body = 0;
                 break;
             }
             case (ASR::symbolType::Module) : {

i.e. Do not remove the body nor the dependencies, only change the ABI to interactive.

Note: The change I have specified in this comment passes all test cases and the edge cases I have encountered so far (like the above one I mentioned).

Any thoughts/suggestions?

[Shaikh-Ubaid]

We get ASR verify pass error: Function sa depends on id but isn't found in its dependency list., ideally that should not be the behaviour.

We cleared the body of the function, but as per the declaraction/prototype, the functionsa depends on id. If we simply clear out the dependencies, then this error is expected.

To fix this, I think we would need to update the dependencies after we clear the body of the function. For example, @Vipul-Cariappa can you try using PassUtils::UpdateDependenciesVisitor after we clear the function body?

[Shaikh-Ubaid]

From here https://github.com/lcompilers/lpython/pull/2706#issuecomment-2120601841, for the following

LCompilersException: Verify failed in the pass: nested_vars Looks like we missed clearing the body of some of the functions.

Some passes might be adding a new function or redefining the old function, they might need updates/fixes to support interactivity.

[Vipul-Cariappa]

Please look at the new changes. If these changes look good, then I can implement the same changes in the LFortran repo and add in Fortran specific tests.

[Vipul-Cariappa]

Not sure why the tests fail. Locally they pass.

❯ ctest
Test project /home/vipul/Workspace/python/lpython
    Start 1: test_stacktrace
1/2 Test #1: test_stacktrace ..................   Passed    0.12 sec
    Start 2: test_lpython
2/2 Test #2: test_lpython .....................   Passed    0.15 sec

100% tests passed, 0 tests failed out of 2

Total Test time (real) =   0.27 sec

❯ ./src/lpython/tests/test_lpython
[doctest] doctest version is "2.4.8"
[doctest] run with "--help" for options
===============================================================================
[doctest] test cases:  38 |  38 passed | 0 failed | 0 skipped
[doctest] assertions: 192 | 192 passed | 0 failed |
[doctest] Status: SUCCESS!

[nikabot]

I think the tests are failing for test_llvm, can you check that?

[Shaikh-Ubaid]

Not sure why the tests fail. Locally they pass.

There seems a failure somewhere, we need to debug it and get the CI to pass.

[Vipul-Cariappa]

You can see that ./src/lpython/tests/test_lpython is failing (reference). But in this https://github.com/lcompilers/lpython/pull/2706#issuecomment-2126808126, I have already specified that it is running in my local computer.

Things I tried to replicate:

• Run under gdb and lldb. Could not replicate the error.
• Run valgrind, there were some memory leaks, but I was searching for the use of uninitialized values. There were 2 cases where an uninitialized value was used, but to me, it did not look like it could be the source of the problem.
• Also tested release build. But still could not replicate the error.

I am not of ideas and clues.

One more thing I could try is to open a new PR that only adds the test without any other changes.

[Shaikh-Ubaid]

One more thing I could try is to open a new PR that only adds the test without any other changes.

Go ahead and try this. Let's get the test to work first.

[Vipul-Cariappa]

Go ahead and try this. Let's get the test to work first.

Please look at #2713. Once that has been merged I will try rebasing the main branch and debug the problem.

[Vipul-Cariappa]

@Shaikh-Ubaid, should I make these changes in the LFortran repo and add in test cases specific to Fortran and check if the tests pass there?

[Shaikh-Ubaid]

@Shaikh-Ubaid, should I make these changes in the LFortran repo and add in test cases specific to Fortran and check if the tests pass there?

Yes, we should test it. It seems you made a PR here https://github.com/lfortran/lfortran/pull/4086.

[Shaikh-Ubaid]

We also need the tests here to pass.

[Shaikh-Ubaid]

Please mark as "Ready for review" when ready.

[Vipul-Cariappa]

As I mentioned earlier, I am not able to replicate this bug locally. Could someone please clone this branch and try to replicate this bug. Running ./src/lpython/tests/test_lpython should error out. Look at https://github.com/lcompilers/lpython/pull/2706#issuecomment-2128482959 for more details.

[Vipul-Cariappa]

3 tests from test_llvm.cpp fail on Windows. All the 3 tests that fail, use the modulo operator %. This causes LPython to import the lpython_builtin.py.

Running this locally on Windows, you will see the following error message:

(lp16) C:\Users\vipul\Documents\Workspace\lpython\build16>.\src\bin\lpython.exe
>>> 3 % 2
warning: The module 'lpython_builtin' located in C:\Users\vipul\Documents\Workspace\lpython\build16\src\bin/../runtime/lpython_builtin.py cannot be loaded
 --> input:1:1
  |
1 | 3 % 2
  | ^^^^^ imported here

syntax error: Token '' (of type 'dedent') is unexpected here
   --> C:\Users\vipul\Documents\Workspace\lpython\build16\src\bin/../runtime/lpython_builtin.py:153:4
    |
153 |     return sum
    |    ^

semantic error: The file 'C:\Users\vipul\Documents\Workspace\lpython\build16\src\bin/../runtime/lpython_builtin.py' failed to parse
 --> input:1:1
  |
1 | 3 % 2
  | ^^^^^

Note: Please report unclear or confusing messages as bugs at
https://github.com/lcompilers/lpython/issues.

This bug is related to #2725

---

EDITED

commit fix indentation for windows fixes the above mentioned bug.

[Vipul-Cariappa]

Explanation behind the update cmake to copy runtime python files to build dir commit

Previous to this commit all tests passed except for ubuntu-latest, macos-latest, and windows-2019. One of the differences between these 3 tests and other tests is that these 3 tests are out-of-source built/compiled. At runtime, they were not able to locate the required runtime Python/LPython files. The commit copies the required runtime files to the build folder from the src folder.

[Vipul-Cariappa]

With the latest commits/changes, tests still fail on Windows. But passes in all other environments.

Error (reference):

JIT session error: Failed to materialize symbols: { (Main, { __module_lpython_builtin___lpython_overloaded_1__abs, _lcompilers_optimization_floordiv_u64, __module_lpython_builtin___lpython_overloaded_13__complex, _lcompilers_optimization_floordiv_i8, __module_lpython_builtin___lpython_overloaded_7___mod, __module_lpython_builtin___lpython_overloaded_2__round, __module_lpython_builtin___lpython_overloaded_0___mod, __module_lpython_builtin___lpython_overloaded_0__max, __module_lpython_builtin___lpython_overloaded_9___mod, __module_lpython_builtin___lpython_overloaded_3__min, _lcompilers_optimization_floordiv_u8, __module_lpython_builtin___lpython_overloaded_0___floor, __module_lpython_builtin___lpython_overloaded_8__abs, __module_lpython_builtin___lpython_overloaded_1__round, __module_lpython_builtin___lpython_overloaded_0___lpython_str_lstrip, _lcompilers_optimization_floordiv_i328, __module_lpython_builtin___lpython_overloaded_2__sum, __module_lpython_builtin___lpython_overloaded_1___lpython_str_center, __mod
JIT session error: Failed to materialize symbols: { (Main, { __real@3fe0000000000000, __real@40000000, __xmm@80000000000000008000000000000000, __real@3eb0c6f7a0b5ed8d, __real@3f000000, __real@80000000, __real@4000000000000000 }) }

The bug is similar to another bug I came across while using REPL on Windows.

(base) PS C:\Users\vipul\Documents\Workspace\lpython\build16> .\src\bin\lpython.exe
>>> def f(x: f64) -> f64:
...   return x
...
>>> f(2.0)
2
>>> def g(x: f64) -> f64:
...   return f(x)
...
>>> g(2.0)
2
>>> def h(x: f64) -> f64:
...   return g(-x)
...
>>> h(2.0)
JIT session error: Failed to materialize symbols: { (Main, { __module___main___h }) }
JIT session error: Failed to materialize symbols: { (Main, { __xmm@80000000000000008000000000000000 }) }

---

I request that we wrap the failing tests with #ifndef _WIN32, and skip these tests on Windows. I will also open an issue regarding it. We can fix the problem of failing tests on Windows in a separate PR.

[Shaikh-Ubaid]

I request that we wrap the failing tests with #ifndef _WIN32, and skip these tests on Windows. I will also open an issue regarding it. We can fix the problem of failing tests on Windows in a separate PR.

Yes, it seems like a good alternative for now.

[Vipul-Cariappa]

I have managed to fix the Windows error. Please have a look at the latest changes. I have modifying KaleidoscopeJIT.h according to the LLVM's KaleidoscopeJIT.h from the 16.0.6 version.

[Vipul-Cariappa]

@Shaikh-Ubaid, this is ready to be merged. Please have a look at it.