Skip to content

Run the executable if no error #1302

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 21, 2022
Merged

Run the executable if no error #1302

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 28 additions & 32 deletions src/bin/lpython.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -927,16 +927,6 @@ int link_executable(const std::vector<std::string> &infiles,
std::cout << "The command '" + cmd + "' failed." << std::endl;
return 10;
}
if (compiler_options.arg_o == "") {
err = system(outfile.c_str());
if (err != 0) {
if (0 < err && err < 256) {
return err;
} else {
return 1;
}
}
}
} else {
std::string CC = "cc";
char *env_CC = std::getenv("LFORTRAN_CC");
Expand All @@ -962,16 +952,6 @@ int link_executable(const std::vector<std::string> &infiles,
std::cout << "The command '" + cmd + "' failed." << std::endl;
return 10;
}
if (compiler_options.arg_o == "") {
err = system(("./" + outfile).c_str());
if (err != 0) {
if (0 < err && err < 256) {
return err;
} else {
return 1;
}
}
}
}
return 0;
} else if (backend == Backend::cpp) {
Expand Down Expand Up @@ -1527,32 +1507,48 @@ int main(int argc, char *argv[])

if (endswith(arg_file, ".py"))
{
int err = 0;
if (backend == Backend::x86) {
return compile_to_binary_x86(arg_file, outfile,
err = compile_to_binary_x86(arg_file, outfile,
runtime_library_dir, compiler_options, time_report);
} else if (backend == Backend::wasm) {
return compile_to_binary_wasm(arg_file, outfile,
err = compile_to_binary_wasm(arg_file, outfile,
runtime_library_dir, compiler_options, time_report);
} else if (backend == Backend::wasm_x86) {
return compile_to_binary_wasm_to_x86(arg_file, outfile,
err = compile_to_binary_wasm_to_x86(arg_file, outfile,
runtime_library_dir, compiler_options, time_report);
}

std::string tmp_o = outfile + ".tmp.o";
int err;
if (backend == Backend::llvm) {
} else if (backend == Backend::llvm) {
#ifdef HAVE_LFORTRAN_LLVM
err = compile_python_to_object_file(arg_file, tmp_o, runtime_library_dir, lpython_pass_manager, compiler_options, time_report);
std::string tmp_o = outfile + ".tmp.o";
err = compile_python_to_object_file(arg_file, tmp_o, runtime_library_dir,
lpython_pass_manager, compiler_options, time_report);
if (err != 0) return err;
err = link_executable({tmp_o}, outfile, runtime_library_dir,
backend, static_link, true, compiler_options);
if (err != 0) return err;
#else
std::cerr << "Compiling Python files to object files requires the LLVM backend to be enabled. Recompile with `WITH_LLVM=yes`." << std::endl;
return 1;
#endif
} else {
throw LFortran::LCompilersException("Unsupported backend.");
}
if (err) return err;
return link_executable({tmp_o}, outfile, runtime_library_dir,
backend, static_link, true, compiler_options);

if (compiler_options.arg_o == "") {
if (backend == Backend::wasm) {
err = system(("js " + outfile +".js").c_str());
} else {
Comment on lines +1538 to +1540
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Shaikh-Ubaid, the Wasm Backend executable can be executed using js filename.out.js, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be executed using node a.out.js (where a.out and a.out.js are the output files generated).

I am unsure if we need auto-execution for wasm backend (or other backends excluding llvm). It seems that when a user specifies a particular backend (apart from llvm, for example, let's say --backend=wasm), the (primary) intention of the user is to generate target backend code from the source code. Thus, if a user executes lpython examples/expr2.py --backend wasm, he's trying to generate wasm code from the source code. He could later choose to use the generated wasm code for his own purposes or choose to execute it.

I think it would be better if we leave the choice of what should be done with the output code (wasm code in our example) onto the user. One added advantage of leaving the choice with the user is that the user would get to know what all files are generated using a particular backend (wasm backend generates two files a.out and a.out.js). The user also gets the opportunity to explore how the generated target (wasm) code could be executed.

I am just sharing my thoughts here. Anything is-fine/works for me.

Copy link
Collaborator Author

@Thirumalai-Shaktivel Thirumalai-Shaktivel Nov 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@czgdp1807 @certik Can you please share your thoughts as well?
I made these changes because I felt like the behaviour should be consistent on all the backends.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that when a user specifies a particular backend (apart from llvm, for example, let's say --backend=wasm), the (primary) intention of the user is to generate target backend code from the source code.

Not sure. I guess --show-wasm should generate the code?

Overall I guess at least the executable shouldn't be deleted after automatic execution. For now for LLVM backend it gets deleted.

Copy link
Collaborator Author

@Thirumalai-Shaktivel Thirumalai-Shaktivel Nov 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this PR changes got merged. LPython doesn't generate a.out anymore. Instead, it generates filename.out. These changes were made because of some problems in testing the runtime outputs, see here for more details. Also, to stop the automatic execution one can use the -o option to specify the executable to be written into.
I apologize for not sharing this updated information.

Copy link
Contributor

@certik certik Nov 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion either way on this. I feel calling "node" from LPython seems fragile.

I think for now, why don't we do the following:

  • lpython a.py compiles a.py with the default backend (currently LLVM, perhaps in the future it will be WASM + x86/arm) and runs it
  • If any other option is passed to lpython, then it will not run it by default.

Although I can see how this is not super consistent. But it gets us started. I think the most common use case is python a.py, so we now already have lpython a.py. Everything else seems lower priority.

We could also consider lpython run a.py or lpython --run a.py, and then you can supply other options as needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean lpython --run a.py --backend x86 to run by default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think lpython a.py --backend x86 && ./a.out is much familiar than using --run. So, Let's only support lpython a.py to run by default.
Also, I have a doubt: Currently, we write the executable to filename.out. Is this fine for other backend as well?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean lpython --run a.py --backend x86 to run by default?

Makes sense to me.

err = system(("./" + outfile).c_str());
}
if (err != 0) {
if (0 < err && err < 256) {
return err;
} else {
return 1;
}
}
Comment on lines +1543 to +1549
Copy link
Collaborator Author

@Thirumalai-Shaktivel Thirumalai-Shaktivel Nov 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a separate topic:
I recently came across something like this:

def test():
	quit(10)

test()
$ lpython a.py
err = 2560
STOP
$ echo $?
1

So, I thought to replace this with the following change:

Suggested change
if (err != 0) {
if (0 < err && err < 256) {
return err;
} else {
return 1;
}
}
if (err != 0) {
if (0 < err && err < 256) {
return err;
} if else (err % 256 == 0) {
return err / 256;
} else {
return 1;
}
}

@certik

}
return 0;
} else {
return link_executable(arg_files, outfile, runtime_library_dir,
backend, static_link, true, compiler_options);
Expand Down