[clang][cli] Fix generation of '-fvisibility' with regards to '-mignore-xcoff-visibility'

[jansvoboda11]

Summary

This patch fixes failure of the CodeGen/aix-ignore-xcoff-visibility.cpp test with command line round-trip.

The absence of '-fvisibility' implies '-mignore-xcoff-visibility'.

The problem is that when '-fvisibility default' is passed to -cc1, it isn't being generated. (This adheres to the principle that generation doesn't produce arguments with default values.)

However, that caused '-mignore-xcoff-visibility' to be implied in the generated command line (without '-fvisibility'), while it wasn't implied in the original command line (with '-fvisibility').

This patch fixes that by always generating '-fvisibility' and explains the situation in comment.

(The '-mginore-xcoff-visibility' option was added in D87451).

[Bigcheese]

LGTM

[DiggerLin]

1. in your summary , it looks clang will emit "-fvisibility default" to cc1 even if there is no -fvisibility in the clang command ? I compiled the clang with your patch, it looks do not always emit "-fvisibility default" to cc1. I understand wrong on your summary ?
2. If clang will emit "-fvisibility default" to cc1 when there is no -fvisibility in the clang command, it is a new functionality , there should be a test case for it?

[jansvoboda11]

In D97552#2607842, @DiggerLin wrote:

1. in your summary , it looks clang will emit "-fvisibility default" to cc1 even if there is no -fvisibility in the clang command ? I compiled the clang with your patch, it looks do not always emit "-fvisibility default" to cc1. I understand wrong on your summary ?
2. If clang will emit "-fvisibility default" to cc1 when there is no -fvisibility in the clang command, it is a new functionality , there should be a test case for it?

1. -fvisibility default will be generated by CompilerInvocation::generateCC1CommandLine, which will be used during Clang modules build. Not by the Clang driver when constructing -cc1 invocation on normal (non-modular) build.
2. This code is tested by a round-trip mechanism when -DCLANG_ROUND_TRIP_CC1_ARGS is turned on: https://lists.llvm.org/pipermail/cfe-dev/2021-February/067714.html

clang/include/clang/Driver/Options.td

def stack_protector : Separate<["-"], "stack-protector">,
   NormalizedValuesScope<"LangOptions">,
   NormalizedValues<["SSPOff", "SSPOn", "SSPStrong", "SSPReq"]>,
   MarshallingInfoEnum<LangOpts<"StackProtector">, "SSPOff">;
 def stack_protector_buffer_size : Separate<["-"], "stack-protector-buffer-size">,
   HelpText<"Lower bound for a buffer to be considered for stack protection">,
   MarshallingInfoInt<CodeGenOpts<"SSPBufferSize">, "8">;
 def fvisibility : Separate<["-"], "fvisibility">,
   HelpText<"Default type and symbol visibility">,
-  MarshallingInfoVisibility<LangOpts<"ValueVisibilityMode">, "DefaultVisibility">;
+  MarshallingInfoVisibility<LangOpts<"ValueVisibilityMode">, "DefaultVisibility">,
+  // Always emitting because of the relation to `-mignore-xcoff-visibility`.
+  AlwaysEmit;
 def ftype_visibility : Separate<["-"], "ftype-visibility">,
   HelpText<"Default type visibility">,
   MarshallingInfoVisibility<LangOpts<"TypeVisibilityMode">, fvisibility.KeyPath>;
 def fapply_global_visibility_to_externs : Flag<["-"], "fapply-global-visibility-to-externs">,
   HelpText<"Apply global symbol visibility to external declarations without an explicit visibility">,
   MarshallingInfoFlag<LangOpts<"SetVisibilityForExternDecls">>;
 def ftemplate_depth : Separate<["-"], "ftemplate-depth">,
   HelpText<"Maximum depth of recursive template instantiation">,

clang/lib/Frontend/CompilerInvocation.cpp

if (O.matches(OPT_fpcc_struct_return) ||
       Opts.setStructReturnConvention(CodeGenOptions::SRCK_OnStack);
     } else {
       assert(O.matches(OPT_freg_struct_return) ||
              O.matches(OPT_msvr4_struct_return));
       Opts.setStructReturnConvention(CodeGenOptions::SRCK_InRegs);
     }
   }
 
+  // This is the reason why '-fvisibility' needs to be always generated:
+  // its absence implies '-mignore-xcoff-visibility'.
+  //
+  // Suppose the original cc1 command line does contain '-fvisibility default':
+  // '-mignore-xcoff-visibility' should not be implied.
+  // * If '-fvisibility' is not generated (as most options with default values
+  //   don't), its absence would imply '-mignore-xcoff-visibility'. This changes
+  //   the command line semantics.
+  // * If '-fvisibility' is generated regardless of its presence and value,
+  //   '-mignore-xcoff-visibility' won't be implied and the command line
+  //   semantics are kept intact.
+  //
+  // When the original cc1 command line does **not** contain '-fvisibility',
+  // '-mignore-xcoff-visibility' is implied. The generated command line will
+  // contain both '-fvisibility default' and '-mignore-xcoff-visibility' and
+  // subsequent calls to `CreateFromArgs`/`generateCC1CommandLine` will always
+  // produce the same arguments.
   if (T.isOSAIX() && (Args.hasArg(OPT_mignore_xcoff_visibility) ||
                       !Args.hasArg(OPT_fvisibility)))
     Opts.IgnoreXCOFFVisibility = 1;
 
   if (Arg *A =
           Args.getLastArg(OPT_mabi_EQ_vec_default, OPT_mabi_EQ_vec_extabi)) {
     if (!T.isOSAIX())
       Diags.Report(diag::err_drv_unsupported_opt_for_target)