[PATCH] D92854: [flang][driver] Add support for `-fsyntax-only`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski marked 4 inline comments as done.
awarzynski added a comment.

Thank you for your reviews! I'll submit an updated patch shortly.



================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:43-44
   } else if (isa<CompileJobAction>(JA) || isa<BackendJobAction>(JA)) {
-    CmdArgs.push_back("-triple");
-    CmdArgs.push_back(Args.MakeArgString(TripleStr));
     if (JA.getType() == types::TY_Nothing) {
----------------
kiranchandramohan wrote:
> Why?
These 2 lines mean that `flang-new -fsyntax-only` is translated to `flang-new -fc1 -fsyntax-only -triple <triple>`.  However,
```
$ bin/flang-new -fc1 -triple
error: unknown argument: '-triple'
```
i.e. the compiler frontend driver, `flang-new -fc1`, doesn't support this flag.

I extracted this change here: https://reviews.llvm.org/D93027.  Also updated the tests that rely on this. Thanks for pointing those failures @sameeranjoshi!


================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:103-107
+  /// Replace the current stream for verbose output.
+  void set_semaOutputStream(llvm::raw_ostream &Value);
+
+  /// Replace the current stream for verbose output.
+  void set_semaOutputStream(std::unique_ptr<llvm::raw_ostream> Value);
----------------
kiranchandramohan wrote:
> What is the coding style of this file?
https://github.com/llvm/llvm-project/blob/main/flang/docs/C%2B%2Bstyle.md


================
Comment at: flang/include/flang/Frontend/FrontendActions.h:29
 
+class SyntaxOnlyAction : public FrontendAction {
+  void ExecuteAction() override;
----------------
sameeranjoshi wrote:
> Do you think following a pattern here for mapping with `enum ActionKind` would be better?
> something like `ParseSyntaxOnlyAction` ?
Yes, that would be better, thanks!


================
Comment at: flang/lib/Frontend/CMakeLists.txt:16
   FortranParser
+  FortranSemantics
+  FortranCommon
----------------
kiranchandramohan wrote:
> Is Evaluate needed?
Don't think so. I built with `-DBUILD_SHARED_LIBS=On`, which usually is pretty good at catching missing CMake dependencies.


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:81
+  // Parse
+  ci.parsing().Parse(llvm::outs());
+  auto &parseTree{*ci.parsing().parseTree()};
----------------
kiranchandramohan wrote:
> What is the use of output stream here? 
In `set_debugOutput`: https://github.com/llvm/llvm-project/blob/1365718778b4ce05587afa22835282c5d3f835b7/flang/lib/Parser/parsing.cpp#L113. Ideally this should  go via some diagnostics engine.


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:82
+  ci.parsing().Parse(llvm::outs());
+  auto &parseTree{*ci.parsing().parseTree()};
+
----------------
kiranchandramohan wrote:
> Will the Prescan step have happened before?
`Prescan` happens here: https://github.com/llvm/llvm-project/blob/1365718778b4ce05587afa22835282c5d3f835b7/flang/lib/Frontend/FrontendAction.cpp#L53. `ExecuteAction` (i.e. this method) is the next step.


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:96
+  semantics.EmitMessages(ci.semaOutputStream());
+}
----------------
sameeranjoshi wrote:
> What happens in case one of the stages in the pipeline fails?
> Where is the error recovery done for e.g if parsing fails ?
That's a great catch, thank you!


================
Comment at: flang/test/Flang-Driver/syntax-only.f90:1
+! RUN: %flang-new -fc1 -fsyntax-only %s 2>&1 | FileCheck %s
+
----------------
kiranchandramohan wrote:
> should there be a test without fc1?
This is a tricky question :) The answer depends on what we want to test here:
* fronted driver (`flang-new -fc1`),
* compiler driver (`flang-new`), 
* Flang frontend, or 
* libclangDriver?

In general `flang-new -fsyntax-only` != `flang-new -fc1 -fsyntax-only`. Admittedly,  currently these are actually equal. However,  if you compare `clang -### -fsyntax-only` and `clang -cc1 -fsyntax-only` then you will see a rather huge difference. `flang-new` is also heading in that direction.

To me `-fsyntax-only` is actually a frontend driver flag. If we also test `flang-new -fsyntax-only` then that triggers quite a few code-paths within `libclangDriver`. But this patch doesn't really touch that logic. Instead, it adds some logic in the frontend driver and hence IMO it should be testing the frontend driver only.

But one can argue that from user's perspective:
* `flang-new -fc1 -fsyntax-only input.f`, and 
* `flang-new input.f`
should be identical. But then again, are we verifying that the user gets what they expect or the internal implementation?

Separately, the fact that `libclangDriver` _forwards_ `-fsyntax-only` to `flang-new -fc1` is tested here: https://github.com/llvm/llvm-project/blob/1365718778b4ce05587afa22835282c5d3f835b7/clang/test/Driver/flang/flang.f90#L16. (it's a bit more nuanced, that option is not really forwarded, but that's an implementation detail).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92854/new/

https://reviews.llvm.org/D92854