[flang-commits] [PATCH] D129156: Add -fpass-plugin option to Flang

[Andrzej Warzynski via Phabricator via flang-commits]

awarzynski added a comment.

In D129156#3639153 <https://reviews.llvm.org/D129156#3639153>, @tarunprabhu wrote:

>> 5. Will this work on all platforms? (e.g. Windows?)
>
> I don't know. Unfortunately, I don't have access to a Windows machine on which to test this. I imagine that since this largely uses LLVM's machinery, if it works with clang, it should work for us too. However, it would still need to be tested. Do you have any suggestions?

"Officially", plugins in LLVM are not supported on Windows :) I'm not aware of one single "source of truth" for this, but there are plenty of references on Discourse and Phabricator:

- https://reviews.llvm.org/D16761
- https://discourse.llvm.org/t/clang-wont-run-example-plugins-on-windows/57231

Going over https://reviews.llvm.org/D56935 should also help understand the implementation in Clang (and the logic behind the semantics of this option). That should help writing the documentation for Flang too.



================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:55
 void Flang::AddOtherOptions(const ArgList &Args, ArgStringList &CmdArgs) const {
   Args.AddAllArgs(CmdArgs,
                   {options::OPT_module_dir, options::OPT_fdebug_module_writer,
----------------
tarunprabhu wrote:
> awarzynski wrote:
> > This is not really something within the scope of this patch, but `AddOtherOptions` should be renamed as e.g. `ForwardToFC1` or something similar.
> I don't think that I should change that here, unless you want me to. As you say, it probably belongs to a different patch.
I agree, this would be an "unrelated change". Hopefully one of us will remember to rename this in the future ;-)


================
Comment at: flang/include/flang/Frontend/CodeGenOptions.h:49
+
 public:
   CodeGenOptions();
----------------
tarunprabhu wrote:
> awarzynski wrote:
> > DELETEME
> Do you want me to delete the second "public:" label?
it is not needed, is it?


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:674
+    } else {
+      diags.Report(clang::diag::err_fe_unable_to_load_plugin)
+          << pluginFile << passPlugin.takeError();
----------------
tarunprabhu wrote:
> awarzynski wrote:
> > IMO, it's a bit weird to report an error and to continue anyway. Apparently that's what Clang [[ https://github.com/llvm/llvm-project/blob/f80a4321ef1bafcd8041884bcb85d9ba24335adb/clang/lib/CodeGen/BackendUtil.cpp#L791-L800 | does ]] 🤔 . Do you know why?
> I think the idea is to display as many errors as reasonably possible instead of just failing on the first error that was encountered. I imagine that the Diagnostics object was intended to be used when parsing where this approach is helpful. But when using it elsewhere, you end up with more ... peculiar behavior. 
So, an "error" indicates that something went fundamentally wrong and indeed, `flang-new` will fail (that's why you had to use `not` in tests). If the driver does not immediately return here, could you add a comment to hint "why"? I also went over the discussion in https://reviews.llvm.org/D56935 and the rationale in Clang is still unclear to me. How about you?

If you are simply trying to reproduce the Clang semantics here, then that would be a good comment for me.


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

https://reviews.llvm.org/D129156