[PATCH] D157151: [Driver] Refactor to use llvm Option's new Visibility flags

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D157151#4582441 <https://reviews.llvm.org/D157151#4582441>, @bogner wrote:

> This is a little bit complicated by the fact that the Option library is already partially extracted out of clang (and used for other drivers, like lld and lldb). The "Default" visibility is defined in llvm's Option library, so calling it something like "Clang" would be pretty confusing for users that aren't the clang Driver library.

Ah, I see. I guess `clangDriver` is a bit unique in the sense that there's quite a few drivers that rely on its Options.td:

- `clang,` `clang -cc1`, `clang -cc1as`, `clang-cl`,
- `flang-new`, `flang-new -fc1`.

> I suppose one option would be to throw something like `using ClangDriver = llvm::Driver::Default;` in Options.h inside the `clang::driver::options` namespace, and then using the alias in Options.td. Would that help?

That would make sense to me, yes. Otherwise the meaning of `Default` is unclear. These things tend to be obvious to folks familiar with the implementation. However, Options.td would normally be edited/updated by people less familiar with the fine details and more concerned about their favorite option and how it's implemented in their favorite tool.

> `Default` options are either options that don't specify `Vis` at all or explicitly mention `Default`. They are not visible in `flang-new` after this change unless they also specify `FlangOption`.

That's not quite true ATM. Although not used,  the following C++ option _is visible_ in `flang-new`:

  flang-new -fno-experimental-relative-c++-abi-vtables file.f90
  flang-new: warning: argument unused during compilation: '-fno-experimental-relative-c++-abi-vtables'

This can be tweaked in `getOptionVisibilityMask` (extracted from this patch):

  llvm::opt::Visibility
  Driver::getOptionVisibilityMask(bool UseDriverMode) const {
    if (!UseDriverMode)
      return llvm::opt::Visibility(llvm::opt::Default);
    if (IsCLMode())
      return llvm::opt::Visibility(options::CLOption);
    if (IsDXCMode())
      return llvm::opt::Visibility(options::DXCOption);
    if (IsFlangMode()) {
      // vvvvvvvvvvvvvvv HERE vvvvvvvvvvvvvvvvvvv
      // TODO: Does flang really want *all* of the clang driver options?
      // We probably need to annotate more specifically.
      return llvm::opt::Visibility(llvm::opt::Default | options::FlangOption);
    }
    return llvm::opt::Visibility(llvm::opt::Default);
  }

Now, prior to this change I couldn't find a way to disable unsupported Clang options in Flang. With this patch,

- all Clang options gain a visibility flag (atm that's `Default`),
- that flag can be used to disable options unsupported in Flang.

For this to work, all options supported by Flang need to have their visibility flags set accordingly. That's the goal, but I can see that we have missed quite a few options over the time (*). Updated here: https://reviews.llvm.org/D157837.

(*) There was no mechanism to enforce that. This is now changing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157151