[flang] Align -x language modes with gfortran
#130268
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-driver Author: Iñaki Amatria Barral (inaki-amatria) ChangesThis PR addresses some of the issues described in #127617. Key changes:
Full diff: https://github.com/llvm/llvm-project/pull/130268.diff 5 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/Flang.cpp b/clang/lib/Driver/ToolChains/Flang.cpp
index d4fea633d0edf..f6b23cec9c2ee 100644
--- a/clang/lib/Driver/ToolChains/Flang.cpp
+++ b/clang/lib/Driver/ToolChains/Flang.cpp
@@ -817,8 +817,12 @@ void Flang::ConstructJob(Compilation &C, const JobAction &JA,
// 'flang -E' always produces output that is suitable for use as fixed form
// Fortran. However it is only valid free form source if the original is also
- // free form.
- if (InputType == types::TY_PP_Fortran &&
+ // free form. Ensure this logic does not incorrectly assume fixed-form for
+ // cases where it shouldn't, such as `flang -x f95 foo.f90`.
+ bool isAtemporaryPreprocessedFile =
+ llvm::sys::path::extension(Input.getFilename())
+ .ends_with(types::getTypeTempSuffix(InputType, /*CLStyle=*/false));
+ if (InputType == types::TY_PP_Fortran && isAtemporaryPreprocessedFile &&
!Args.getLastArg(options::OPT_ffixed_form, options::OPT_ffree_form))
CmdArgs.push_back("-ffixed-form");
diff --git a/flang/lib/Frontend/CompilerInvocation.cpp b/flang/lib/Frontend/CompilerInvocation.cpp
index 8b07a50824899..1537122ddced5 100644
--- a/flang/lib/Frontend/CompilerInvocation.cpp
+++ b/flang/lib/Frontend/CompilerInvocation.cpp
@@ -863,6 +863,12 @@ static void parsePreprocessorArgs(Fortran::frontend::PreprocessorOptions &opts,
(currentArg->getOption().matches(clang::driver::options::OPT_cpp))
? PPMacrosFlag::Include
: PPMacrosFlag::Exclude;
+ // Enable -cpp based on -x unless explicitly disabled with -nocpp
+ if (opts.macrosFlag != PPMacrosFlag::Exclude)
+ if (const auto *dashX = args.getLastArg(clang::driver::options::OPT_x))
+ opts.macrosFlag = llvm::StringSwitch<PPMacrosFlag>(dashX->getValue())
+ .Case("f95-cpp-input", PPMacrosFlag::Include)
+ .Default(opts.macrosFlag);
opts.noReformat = args.hasArg(clang::driver::options::OPT_fno_reformat);
opts.preprocessIncludeLines =
diff --git a/flang/test/Driver/dash-x-f95-cpp-input.f b/flang/test/Driver/dash-x-f95-cpp-input.f
new file mode 100644
index 0000000000000..2f42dc9342842
--- /dev/null
+++ b/flang/test/Driver/dash-x-f95-cpp-input.f
@@ -0,0 +1,35 @@
+program main
+ print *, __FILE__, __LINE__
+end
+
+! This test verifies that `flang`'s `-x` options behave like `gfortran`'s.
+! Specifically:
+! - `-x f95` should process the file based on its extension unless overridden.
+! - `-x f95-cpp-input` should behave like `-x f95` but with preprocessing
+! (`-cpp`) enabled unless overridden.
+
+! ---
+! Ensure the file is treated as fixed-form unless explicitly set otherwise
+! ---
+! RUN: not %flang -Werror -x f95 -cpp %s 2>&1 | FileCheck --check-prefix=SCAN-ERROR %s
+! RUN: not %flang -Werror -x f95-cpp-input %s 2>&1 | FileCheck --check-prefix=SCAN-ERROR %s
+
+! SCAN-ERROR: error
+
+! RUN: %flang -Werror -x f95 -cpp -ffree-form %s 2>&1 | FileCheck --check-prefix=NO-SCAN-ERROR --allow-empty %s
+! RUN: %flang -Werror -x f95-cpp-input -ffree-form %s 2>&1 | FileCheck --check-prefix=NO-SCAN-ERROR --allow-empty %s
+
+! NO-SCAN-ERROR-NOT: error
+
+! ---
+! Ensure `-cpp` is not enabled by default unless explicitly requested
+! ---
+! RUN: not %flang -Werror -x f95 -ffree-form %s 2>&1 | FileCheck --check-prefix=SEMA-ERROR %s
+! RUN: not %flang -Werror -x f95-cpp-input -nocpp -ffree-form %s 2>&1 | FileCheck --check-prefix=SEMA-ERROR %s
+
+! SEMA-ERROR: error
+
+! RUN: %flang -Werror -x f95 -cpp -ffree-form %s 2>&1 | FileCheck --check-prefix=NO-SEMA-ERROR --allow-empty %s
+! RUN: %flang -Werror -x f95-cpp-input -ffree-form %s 2>&1 | FileCheck --check-prefix=NO-SEMA-ERROR --allow-empty %s
+
+! NO-SEMA-ERROR-NOT: error
diff --git a/flang/test/Driver/dash-x-f95-do-not-assume-fixed-form.f90 b/flang/test/Driver/dash-x-f95-do-not-assume-fixed-form.f90
new file mode 100644
index 0000000000000..549286d7f2f3d
--- /dev/null
+++ b/flang/test/Driver/dash-x-f95-do-not-assume-fixed-form.f90
@@ -0,0 +1,12 @@
+! This test verifies that using `-x f95` does not cause the driver to assume
+! this file is in fixed-form.
+
+program main
+ print *, "Hello, World!"
+end
+
+! RUN: %flang -### -x f95 %s 2>&1 | FileCheck --check-prefix=PRINT-PHASES %s
+! PRINT-PHASES-NOT: -ffixed-form
+
+! RUN: %flang -Werror -x f95 %s 2>&1 | FileCheck --check-prefix=COMPILE --allow-empty %s
+! COMPILE-NOT: error
diff --git a/flang/test/Driver/input-from-stdin/input-from-stdin.f90 b/flang/test/Driver/input-from-stdin/input-from-stdin.f90
index 285f0751b35d8..1fcc0340a64ba 100644
--- a/flang/test/Driver/input-from-stdin/input-from-stdin.f90
+++ b/flang/test/Driver/input-from-stdin/input-from-stdin.f90
@@ -6,7 +6,7 @@
! Input type is implicit
! RUN: cat %s | %flang -E -cpp - | FileCheck %s --check-prefix=PP-NOT-DEFINED
! RUN: cat %s | %flang -DNEW -E -cpp - | FileCheck %s --check-prefix=PP-DEFINED
-! RUN: cat %s | %flang -DNEW -E - | FileCheck %s --check-prefix=PP-NOT-DEFINED
+! RUN: cat %s | %flang -DNEW -E - | FileCheck %s --check-prefix=PP-DEFINED
! RUN: cat %s | %flang -DNEW -E -nocpp - | FileCheck %s --check-prefix=PP-NOT-DEFINED
! Input type is explicit
|
| ! RUN: %flang -### -x f95 %s 2>&1 | FileCheck --check-prefix=PRINT-PHASES %s | ||
| ! PRINT-PHASES-NOT: -ffixed-form | ||
|
|
||
| ! RUN: %flang -Werror -x f95 %s 2>&1 | FileCheck --check-prefix=COMPILE --allow-empty %s |
There was a problem hiding this comment.
With this test as written, a linker and the runtime libraries are both required. Is this necessary? Is the intention to check that the free-form code here is parsed correctly? If so, is -emit-llvm or -emit-mlir sufficient?
There was a problem hiding this comment.
Fixed. I added -fsyntax-only to ensure this executes flang only
| ! RUN: not %flang -Werror -x f95 -ffree-form %s 2>&1 | FileCheck --check-prefix=SEMA-ERROR %s | ||
| ! RUN: not %flang -Werror -x f95-cpp-input -nocpp -ffree-form %s 2>&1 | FileCheck --check-prefix=SEMA-ERROR %s | ||
|
|
||
| ! SEMA-ERROR: error |
There was a problem hiding this comment.
Will this test pass if a different error occurs? Is it feasible to search for something more precise than just "error" - perhaps "semantics error? If so, we should do that for the other checks as well.
There was a problem hiding this comment.
I assumed the broader the check, the better. But if you believe checking for "Semantic errors" or "Scan errors" is a better alternative I can update the test. TYSM for taking the time to review this. I appreciate it :-)
There was a problem hiding this comment.
It's always a balancing act.
In this case, you are checking that the compiler parses the file in a specific way. In that case, you expect to see a specific category of error, so you might as well check for that. That way, if a change elsewhere in the compiler results in a different error, the test failure will alert the developer. It may be that this "new" error is expected. Or, it could indicate that the change had an effect on a seemingly unrelated part of the code. In either case, it forces the developer to take a closer look.
That's just my opinion, though.
There was a problem hiding this comment.
Fixed. I replaced the ! CHECK: error pattern with specific checks!
inaki-amatria
force-pushed
the
fix-dash-x-language-modes
branch
from
6e304d0 to
eceaa34
Compare
There was a problem hiding this comment.
This LGTM now when @tarunprabhu is happy, thanks for seeing this through despite my lengthy comments!
This isn't to hold up this patch, but as a separate thought: I wonder if there's even any reason to have types::TY_PP_Fortran and types::TY_Fortran? Do we ever actually treat them differently..? It seems we always decide whether to preprocess or not based on other criteria
inaki-amatria
force-pushed
the
fix-dash-x-language-modes
branch
from
eceaa34 to
91f1dee
Compare
Thank you for the quick review, David! I also appreciate your patience with my lengthy comments. I was so caught up in the fixed-form assumption that I failed to realize that reverting the patch I had intended to undo in the previous PR would introduce a regression on your side. I truly appreciate your time and insights!
As for your separate thought: I agree that this part of the code might benefit from a second look. |
This change ensures that flang no longer assumes fixed-form when using `-x f95`. The only case where fixed-form is still assumed is when `-x f95` is used with `.i` files, unless explicitly overridden by the user.
This change ensures that specifying `-x f95-cpp-input` automatically enables `-cpp`, making `flang`'s behavior consistent with `gfortran`. `flang/test/Driver/input-from-stdin/input-from-stdin.f90` changes because the driver assumes `-x f95-cpp-input` for `<stdin>`. Therefore, after this patch, Fortran source that comes from `<stdin>` will now be preprocessed.
inaki-amatria
force-pushed
the
fix-dash-x-language-modes
branch
from
91f1dee to
35ebfdf
Compare
This PR addresses some of the issues described in llvm#127617. Key changes: - Stop assuming fixed-form for `-x f95` unless the input is a `.i` file. This change ensures compatibility with `-save-temps` workflows while preventing unintended fixed-form assumptions. - Ensure `-x f95-cpp-input` enables `-cpp` by default, aligning Flang's behavior with `gfortran`.
| Input.isFilename() && | ||
| llvm::sys::path::extension(Input.getFilename()) | ||
| .ends_with(types::getTypeTempSuffix(InputType, /*CLStyle=*/false)); | ||
| if (InputType == types::TY_PP_Fortran && isAtemporaryPreprocessedFile && |
There was a problem hiding this comment.
I've only just spotted this, but this requirement that isAtemporaryPreprocessedFile appears to have changed the default for all files to -cpp instead of -nocpp. I don't think this patch intended that change?
E.g. files with the .f90 suffix are now treated as if -cpp were passed, which I don't think is the intended behaviour.
There was a problem hiding this comment.
Sorry I thought it was this part but I actually think it is the other change 😓
| if (opts.macrosFlag != PPMacrosFlag::Exclude) | ||
| if (const auto *dashX = args.getLastArg(clang::driver::options::OPT_x)) | ||
| opts.macrosFlag = llvm::StringSwitch<PPMacrosFlag>(dashX->getValue()) | ||
| .Case("f95-cpp-input", PPMacrosFlag::Include) |
There was a problem hiding this comment.
I believe we concluded that f95-cpp-input should mean that it has already been preprocessed, and so does not need preprocessing again, and fixed that elsewhere. So this should be PPMacrosFlag::Exclude since -x f95-cpp-input is what we pass for .f90 files, and that's where the issue is coming from.
Edit: Ah, no, we agreed the opposite 😅 , and we are passing the wrong -x for .f90 files. Sorry, I find this all a bit confusing....
|
I've created a PR at #133775 cleaning up the meanings of the |
This PR addresses some of the issues described in #127617. Key changes:
-x f95unless the input is a.ifile. This change ensures compatibility with-save-tempsworkflows while preventing unintended fixed-form assumptions.-x f95-cpp-inputenables-cppby default, aligning Flang's behavior withgfortran.