This is an archive of the discontinued LLVM Phabricator instance.

[flang][driver] Fix output filename generation in `flang`
ClosedPublic

Authored by awarzynski on Jul 13 2021, 8:31 AM.

Details

Summary

In the flang bash script, we need to be careful _when_ to use <output>
from flang -c -o <output> <input> when generating the relocatable
output file name.

In particular, we should use it in this case:

compilation only
flang -c -o <output> <input>

but leave it for the final executable here:

compile, assemble and link
flang  -o <output> <input>

This change is implemented in get_relocatable_name.

I've also taken the liberty to fix how errors from sub-commands are
reported (without this change, flang always returns 0 on failure).
This is implemented in main.

Diff Detail

Event Timeline

awarzynski created this revision.Jul 13 2021, 8:31 AM
awarzynski requested review of this revision.Jul 13 2021, 8:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2021, 8:31 AM

The specifics of the script are a bit beyond me but:

(without this change, flang always returns 0 on failure)

Definitely will help the test suite be clearer about what's failed where.

flang/tools/f18/flang.in
343–349

I'm surprised that this works with:

set -euo pipefail

(but then again bash is always surprising)

awarzynski added inline comments.Jul 15 2021, 1:00 AM
flang/tools/f18/flang.in
343–349

That's a great catch, thank you!

I don't see a better way to work around it other than surrounding such statements with set +e and set -e. That's also the suggested solution here (it's the best intro into set -euo pipefail that I am aware of). This is not great, but we don't consider this script a long-term solution, so perhaps that's fine?

As for if ! <command>, that sets $? to 0 when <command> fails. That's because there is ! in ! <command>. This is not what we want here. This is documented in help if. Hence the need to update this.

Work around set -e and commands that might fail

The failure handling looks good. I'm still confused about the output naming. Can you clarify what you mean by:

In particular, we should use it in this case:

compilation only
flang -c -o <output> <input>
but leave it for the final executable here:

compile, assemble and link
flang  -o <output> <input>

Do you mean "but leave it for the final executable" as in, only use <output> to name the final executable. Instead of using it to name the intermediate sources as well?

flang/tools/f18/flang.in
343–349

This is not great

My thoughts on most bash but this looks good. Pretty obvious what's going on.

The failure handling looks good. I'm still confused about the output naming. Can you clarify what you mean by:

In particular, we should use it in this case:

compilation only
flang -c -o <output> <input>
but leave it for the final executable here:

compile, assemble and link
flang  -o <output> <input>

Do you mean "but leave it for the final executable" as in, only use <output> to name the final executable. Instead of using it to name the intermediate sources as well?

Let's consider 2 examples. This is basically what the script does.
CASE 1: compilation only

flang -c hello.f90 -o hello

This script will translate this into:

# Genreate temporary unaprsed Fortran file
flang-new -fdebug-unparse hello.f90 -o flang_unparsed_source_file_0.f90
# Genreate the output object file - do use `-o hello`
gfortran -c -o hello flang_unparsed_source_file_0.f90

CASE 2: compilation + assembly + linking

flang  hello.f90 -o hello

This script will translate this into:

# Genreate temporary unaprsed Fortran file
flang-new -fdebug-unparse hello.f90 -o flang_unparsed_source_file_0.f90
# Generate a temporary object file - don't use `-o hello`
gfortran -c  flang_unparsed_source_file_0.f90  -o flang_unparsed_source_file_0.o
# Generate the final outupt - do use `-o hello`
gfortran  flang_unparsed_source_file_0.o -o hello

BOTTOM LINE
The script needs to be careful _when_ to use -o <output> and _what_ to use as <output>. Sadly, it's tricky to convey that with comments or code :/

DavidSpickett accepted this revision.Jul 15 2021, 4:12 AM

I don't think I can word it well either but it makes sense seeing the examples.

This revision is now accepted and ready to land.Jul 15 2021, 4:12 AM

Rebase on top of main