Fix ASR verify pass error while using Interactive #2706
Conversation
src/libasr/asr_verify.cpp
Outdated
| ASR::FunctionType_t* v_func_type = ASR::down_cast<ASR::FunctionType_t>(x.m_function_signature); | ||
| if (v_func_type->m_abi == abiType::Interactive) { | ||
| // This function would have been verified in the previous interactive pass | ||
| return; | ||
| } |
There was a problem hiding this comment.
Instead of just returning here, let's verify things that we can verify. Like I think for interactive functions we expect the function bodies to be empty, we expect the dependencies to be empty and the other things in that direction.
|
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_varsLooks like we missed clearing the body of some of the functions. |
|
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. |
|
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. |
src/libasr/asr_verify.cpp
Outdated
| "The Function::n_body should be 0 if abi set to Interactive"); | ||
| require(x.m_body == nullptr, | ||
| "The Function::m_body should be null if abi set to Interactive"); | ||
| return; |
There was a problem hiding this comment.
I think this should be without the return:
| return; |
Then it would be good, it will make the ASR verify stronger.
There was a problem hiding this comment.
I think this should be without the return:
Yes, it should be removed. @Vipul-Cariappa since you have issues with the dependencies for interactive, how about we make just the dependency check to be optional? Or better we make the dependency check to be having no dependencies?
And we can also update the mark_all_variables_external() here https://github.com/lfortran/lfortran/blob/d0882b59a1002efccc8532ee235f859271ef658e/src/libasr/asr_scopes.cpp#L39-L45 to remove the dependencies (along with the body).
There was a problem hiding this comment.
You can remove the dependency check in a Draft PR to see if it fixes issues.
However, to merge into master, we need to keep all these checks, they are very important. Instead, we need to just remove all dependencies when we are removing the body. To be exact, we should keep dependencies if they are used in declarations, but removing all of them might get things working pretty well and we can refine it later. The ASR checks I think should all stay, with the addition of the above ones for interactive.
There was a problem hiding this comment.
To be exact, we should keep dependencies if they are used in declarations
Do we know of any case where we will require the dependencies of a function during its declaration, but it does not require the body of the function? Also a thing to note is that we would have changed the ABI to interactive.
I am not very familiar with dependency, can someone please explain the requirement of the dependency, and how it is used in later parts of the compiling stages? I assume we verify the dependencies by going through the function body and checking if it calls other functions and then checking if that function is in the dependency list.
There was a problem hiding this comment.
Yes, for example:
interface
subroutine s(n, x)
integer, intent(in) :: n
real, intent(inout) :: x(f(n))
end subroutine
interfaceHere s depends on the pure function f.
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. |
If you look at this #2706 (comment). |
|
Please have a look at the new changes. I have undone the modifications to 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. |
|
Let's keep the ASR verify changes, those were good. Just remove "return". |
|
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 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 maingfortran 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 functionI am new to fortran, if you could explain the meaning of these error messages and how to fix them, it would be helpful. |
|
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.
Any thoughts/suggestions? |
We cleared the body of the function, but as per the declaraction/prototype, the function 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 |
|
From here #2706 (comment), for the following
Some passes might be adding a new function or redefining the old function, they might need updates/fixes to support interactivity. |
|
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. |
|
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! |
|
I think the tests are failing for |
There seems a failure somewhere, we need to debug it and get the CI to pass. |
|
You can see that Things I tried to replicate:
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. |
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. |
|
@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? |
|
3 tests from 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. |
|
Explanation behind the update cmake to copy runtime python files to build dir commit Previous to this commit all tests passed except for |
|
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 |
| configure_file("${CMAKE_CURRENT_SOURCE_DIR}/src/runtime/random.py" "${CMAKE_CURRENT_BINARY_DIR}/src/runtime/random.py") | ||
| configure_file("${CMAKE_CURRENT_SOURCE_DIR}/src/runtime/statistics.py" "${CMAKE_CURRENT_BINARY_DIR}/src/runtime/statistics.py") | ||
| configure_file("${CMAKE_CURRENT_SOURCE_DIR}/src/runtime/sys.py" "${CMAKE_CURRENT_BINARY_DIR}/src/runtime/sys.py") | ||
| configure_file("${CMAKE_CURRENT_SOURCE_DIR}/src/runtime/time.py" "${CMAKE_CURRENT_BINARY_DIR}/src/runtime/time.py") |
There was a problem hiding this comment.
Not sure if its a good idea to copy these. As of now, where does lpython look for these? Can we update lpython to look into the correct location?
There was a problem hiding this comment.
Copying is the safest thing to do. Right now it uses relative path from the executable's location to figure out the runtime file's path.
The reason I say it is a safe option is because the user building the LPython may set the CMAKE_CURRENT_BINARY_DIR to a completely different folder. For example, in Windows, I may want to store the source files in D drive and the compiled executable in C drive.
Yes, it seems like a good alternative for now. |
|
I have managed to fix the Windows error. Please have a look at the latest changes. I have modifying |
src/runtime/lpython_builtin.py
Outdated
|
|
||
There was a problem hiding this comment.
Let's remove these spaces and instead fix our tokenizer/parser (#2725).
|
@Shaikh-Ubaid, this is ready to be merged. Please have a look at it. |
| return std::string(&bytes[0], filesize); | ||
| return replace(std::string(&bytes[0], filesize), "\r\n", "\n"); |
There was a problem hiding this comment.
Let's open an issue that our tokenizer/parser should be updated/fixed to produce the same tokens/parse-tree on windows and unix.
Code to replicate:
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_externalwhere we change the ABI type of all the previously defined/declared functions to Interactive. Therefore we can skip their verification.