-
Notifications
You must be signed in to change notification settings - Fork 162
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
Sync libasr
from LFortran
#2148
Conversation
1e55b08
to
895b929
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me. Thank you so much for this!
std::cerr << diagnostics.render2(); | ||
throw LCompilersException("Verify failed"); | ||
}; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has to be there, this is an important check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is reason why I removed it. I will explain in detail tomorrow morning. In short, this is removed because ExternalSymbol aren’t resolved at this stage. They are resolved later and then ASR Verify is called after that. Full explanation tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So here you go. Found the example which fails if we keep this check. The scene is that, the fix_external_symbols(*tu, *symtab);
isn't called yet. So ExternalSymbols::m_external
is nullptr
when this check is called. It is called inside, load_module
(see below lines),
The example which fails if we keep this check,
debug1.f90
module fpm_command_line
implicit none
type, abstract :: fpm_cmd_settings
character(len=:), allocatable :: working_dir
logical :: verbose=.true.
end type
type, extends(fpm_cmd_settings) :: fpm_build_settings
logical :: list=.false.
logical :: show_model=.false.
logical :: build_tests=.false.
logical :: prune=.true.
character(len=:), allocatable :: compiler
character(len=:), allocatable :: c_compiler
character(len=:), allocatable :: cxx_compiler
character(len=:), allocatable :: archiver
character(len=:), allocatable :: profile
character(len=:), allocatable :: flag
character(len=:), allocatable :: cflag
character(len=:), allocatable :: cxxflag
character(len=:), allocatable :: ldflag
end type
type, extends(fpm_build_settings) :: fpm_run_settings
character(len=:), allocatable :: name(:)
character(len=:), allocatable :: args
character(len=:), allocatable :: runner
logical :: example
end type
end module
debug2.f90
module fpm_model
implicit none
integer, parameter :: FPM_SCOPE_APP = 3
integer, parameter :: FPM_SCOPE_TEST = 4
integer, parameter :: FPM_SCOPE_EXAMPLE = 5
end module
debug3.f90
module fpm
use fpm_command_line, only: fpm_run_settings
use fpm_model, only: FPM_SCOPE_APP, FPM_SCOPE_EXAMPLE, FPM_SCOPE_TEST
implicit none
public :: cmd_run
contains
subroutine cmd_run(settings, test)
class(fpm_run_settings), intent(in) :: settings
logical, intent(in) :: test
integer :: run_scope
if (test) then
run_scope = FPM_SCOPE_TEST
else
run_scope = merge(FPM_SCOPE_EXAMPLE, FPM_SCOPE_APP, settings%example)
end if
contains
subroutine compact_list_all()
end subroutine compact_list_all
subroutine compact_list()
end subroutine compact_list
end subroutine cmd_run
end module fpm
debug.f90
module fpm_cmd_install
use fpm
end module fpm_cmd_install
(lf) 0:41:49:~/lfortran_project/lfortran % lfortran -c /Users/czgdp1807/lfortran_project/debug1.f90
(lf) 0:41:54:~/lfortran_project/lfortran % lfortran -c /Users/czgdp1807/lfortran_project/debug2.f90
(lf) 0:41:56:~/lfortran_project/lfortran % lfortran -c /Users/czgdp1807/lfortran_project/debug3.f90
(lf) 0:41:58:~/lfortran_project/lfortran % lfortran -c /Users/czgdp1807/lfortran_project/debug.f90
Internal Compiler Error: Unhandled exception
Traceback (most recent call last):
File "/Users/czgdp1807/lfortran_project/lfortran/src/bin/lfortran.cpp", line 2081
return compile_to_object_file(arg_file, outfile, false,
File "/Users/czgdp1807/lfortran_project/lfortran/src/bin/lfortran.cpp", line 888
result = fe.get_asr2(input, lm, diagnostics);
File "/Users/czgdp1807/lfortran_project/lfortran/src/lfortran/fortran_evaluator.cpp", line 251
Result<ASR::TranslationUnit_t*> res2 = get_asr3(*ast, diagnostics);
File "/Users/czgdp1807/lfortran_project/lfortran/src/lfortran/fortran_evaluator.cpp", line 273
compiler_options.symtab_only, compiler_options);
File "/Users/czgdp1807/lfortran_project/lfortran/src/lfortran/semantics/ast_to_asr.cpp", line 93
if (res.ok) {
File "/Users/czgdp1807/lfortran_project/lfortran/src/lfortran/semantics/ast_symboltable_visitor.cpp", line 2747
v.visit_TranslationUnit(ast);
File "/Users/czgdp1807/lfortran_project/lfortran/src/lfortran/semantics/ast_symboltable_visitor.cpp", line 126
visit_ast(*x.m_items[i]);
File "/Users/czgdp1807/lfortran_project/lfortran/src/lfortran/ast.h", line 4768
void visit_ast(const ast_t &b) { visit_ast_t(b, self()); }
File "/Users/czgdp1807/lfortran_project/lfortran/src/lfortran/ast.h", line 4725
case astType::unit: { v.visit_unit((const unit_t &)x); return; }
File "/Users/czgdp1807/lfortran_project/lfortran/src/lfortran/ast.h", line 4771
void visit_mod(const mod_t &b) { visit_mod_t(b, self()); }
File "/Users/czgdp1807/lfortran_project/lfortran/src/lfortran/ast.h", line 4366
case modType::Module: { v.visit_Module((const Module_t &)x); return; }
File "/Users/czgdp1807/lfortran_project/lfortran/src/lfortran/semantics/ast_symboltable_visitor.cpp", line 471
visit_ModuleSubmoduleCommon<AST::Module_t, ASR::Module_t>(x);
File "/Users/czgdp1807/lfortran_project/lfortran/src/lfortran/semantics/ast_symboltable_visitor.cpp", line 424
visit_unit_decl1(*x.m_use[i]);
File "/Users/czgdp1807/lfortran_project/lfortran/src/lfortran/ast.h", line 4780
void visit_unit_decl1(const unit_decl1_t &b) { visit_unit_decl1_t(b, self()); }
File "/Users/czgdp1807/lfortran_project/lfortran/src/lfortran/ast.h", line 4387
case unit_decl1Type::Use: { v.visit_Use((const Use_t &)x); return; }
File "/Users/czgdp1807/lfortran_project/lfortran/src/lfortran/semantics/ast_symboltable_visitor.cpp", line 2000
t = (ASR::symbol_t*)(ASRUtils::load_module(al, tu_symtab,
File "/Users/czgdp1807/lfortran_project/lfortran/src/libasr/asr_utils.cpp", line 247
*symtab, intrinsic, pass_options);
File "/Users/czgdp1807/lfortran_project/lfortran/src/libasr/asr_utils.cpp", line 395
ASR::TranslationUnit_t *asr = load_modfile(al, modfile, false, symtab);
File "/Users/czgdp1807/lfortran_project/lfortran/src/libasr/modfile.cpp", line 94
ASR::asr_t *asr = deserialize_asr(al, asr_binary, load_symtab_id, symtab);
File "/Users/czgdp1807/lfortran_project/lfortran/src/lfortran/ast_serialization.cpp", line 388
Line not found
File "/Users/czgdp1807/lfortran_project/lfortran/src/lfortran/ast_serialization.cpp", line 405
Line not found
File "/Users/czgdp1807/lfortran_project/lfortran/src/libasr/asr_verify.cpp", line 1102
v.visit_TranslationUnit(unit);
File "/Users/czgdp1807/lfortran_project/lfortran/src/libasr/asr_verify.cpp", line 108
for (auto &a : x.m_global_scope->get_scope()) {
File "../libasr/asr.h", line 5025
File "../libasr/asr.h", line 4741
File "/Users/czgdp1807/lfortran_project/lfortran/src/libasr/asr_verify.cpp", line 316
for (auto &a : x.m_symtab->get_scope()) {
File "../libasr/asr.h", line 5025
File "../libasr/asr.h", line 4742
File "/Users/czgdp1807/lfortran_project/lfortran/src/libasr/asr_verify.cpp", line 434
visit_stmt(*x.m_body[i]);
File "../libasr/asr.h", line 5042
File "../libasr/asr.h", line 4777
File "../libasr/asr.h", line 5425
File "../libasr/asr.h", line 5042
File "../libasr/asr.h", line 4765
File "/Users/czgdp1807/lfortran_project/lfortran/src/libasr/asr_verify.cpp", line 361
}
File "../libasr/asr.h", line 5369
File "../libasr/asr.h", line 5088
File "../libasr/asr.h", line 4819
File "/Users/czgdp1807/lfortran_project/lfortran/src/libasr/asr_verify.cpp", line 980
verify_(x, diagnostics);
File "../libasr/pass/intrinsic_function_registry.h", line 2539
File "../libasr/asr_utils.h", line 184
File "../libasr/asr.h", line 49239
AssertFailed: e->m_external
A natural question why didn't we observe this failure before? Well before this, we didn't have merge implemented properly. When we implemented it as an IntrinsicFunction
, then its verify_args
is called where it tries to verify the type of each of its argument. However since all the ExternalSymbol::m_external
are nullptr
at this point of time, so ASRUtils::expr_type
gives the above assertion error.
Bottomline - During de-serialisation It only make sense to call ASR verify pass when all the ExternalSymbol::m_external
are filled correctly with the help of fix_external_symbols(*tu, *symtab)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ASR verify check has a mode to run and allow the m_external
in ExternalSymbol to be nullptr, which is what you need to run after you deserialize. So we should run it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check_external
is indeed false
and even then the verify fails. The failure is not in,
lpython/src/libasr/asr_verify.cpp
Lines 669 to 671 in 851f878
if (check_external) { | |
require(x.m_external != nullptr, | |
"ExternalSymbol::m_external cannot be nullptr"); |
but in the following code,
lpython/src/libasr/pass/intrinsic_function_registry.h
Lines 2537 to 2540 in 851f878
static inline void verify_args(const ASR::IntrinsicFunction_t& x, diag::Diagnostics& diagnostics) { | |
const Location& loc = x.base.base.loc; | |
ASR::expr_t *tsource = x.m_args[0], *fsource = x.m_args[1], *mask = x.m_args[2]; | |
ASR::ttype_t *tsource_type = ASRUtils::expr_type(tsource); |
As I explained in my above comment (quoting again),
A natural question why didn't we observe this failure before? Well before this, we didn't have merge implemented properly. When we implemented it as an
IntrinsicFunction
, then itsverify_args
is called where it tries to verify the type of each of its argument. However since all theExternalSymbol::m_external
arenullptr
at this point of time, soASRUtils::expr_type
gives the above assertion error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I am sure it's involved why it fails. But that's irrelevant. When we deserialize, we need to be able to verify that the ASR is actually correct. So if things became tricky with regards to external symbol in IntrinsicFunction, then we might need to add an exception when check_external
is false. But we should check everything that we can check, for robustness. The deserialization can be a fragile process, one simple mistake in serialization and the whole ASR becomes incorrect. So we need to check this, one way or the other.
Corresponding LFortran PR - lfortran/lfortran#1965