[clang][AIX] Fix Overly Strict LTO Option Checking against `data-sections` when `mxcoff-roptr` is in Effect

[qiongsiwu1]

Summary

The LTO -mxcoff-roptr check against data sections is overly strict and it ignores the fact that data sections is on by default on AIX, causing valid LTO compilation to fail when -fdata-sections is not explicitly specified.

This patch revises the check so that an error is reported only if data sections is explicitly turned off for LTO.

[qiongsiwu1]

Gentle ping for review. Thanks so much!

[MaskRay]

clang/lib/Driver/ToolChains/CommonArgs.cpp

751–752

Consider the hasFlagNoClaim variant. OPT_fdata_sections is mainly claimed elsewhere.

[MaskRay]

The LTO mxcoff-roptr check against data

-mxcoff-roptr. Consider not dropping the leading -.

[qiongsiwu1]

Address code review. Thanks @MaskRay for the feedback!

[qiongsiwu1]

Ping for review. Thank you!

[qiongsiwu1]

Revising the logic to pass -data-sections= to libLTO. We now pass -data-sections=1 to libLTO by default unless it is explicitly turned off.

Old behaviour:

1. Pass -data-sections=1 if the target requires uses of separate sections, or the user explicitly sets -fdata-sections.
2. Pass -data-sections=0 if the user explicitly specifies -fno-data-sections.
3. Pass nothing otherwise.

New behaviour:

1. Pass -data-section=1 if nothing is specified, or if the target requires uses of separate sections, or the user explicitly sets -fdata-sections. (Combining cases 1 and 3 above.)
2. Pass -data-sections=0 if the user explicitly specifies -fno-data-sections.

I think the new behaviour is safe because libLTO defaults to use data sections when nothing is specified https://github.com/llvm/llvm-project/blob/829ed96b779c10cb023d7e3dbcfad22413979ec4/llvm/lib/LTO/LTOCodeGenerator.cpp#L426.

@hubert.reinterpretcast @MaskRay could you let me know what you think? Thanks so much!

[qiongsiwu1]

Revert to the DataSections logic. It is better not to change that in this patch.

[hubert.reinterpretcast]

LGTM with minor comment.

clang/lib/Driver/ToolChains/CommonArgs.cpp

732–735

Coding standards suggest that braces be used consistently for each part of an if/else chain.

[qiongsiwu1]

Fixing inconsistent braces. Thanks for the feedback @hubert.reinterpretcast !

clang/lib/Driver/ToolChains/CommonArgs.cpp

#endif
   if (Args.hasFlag(options::OPT_ffunction_sections,
                    options::OPT_fno_function_sections, UseSeparateSections))
     CmdArgs.push_back(
         Args.MakeArgString(Twine(PluginOptPrefix) + "-function-sections=1"));
   else if (Args.hasArg(options::OPT_fno_function_sections))
     CmdArgs.push_back(
         Args.MakeArgString(Twine(PluginOptPrefix) + "-function-sections=0"));
 
+  bool DataSectionsTurnedOff = false;
   if (Args.hasFlag(options::OPT_fdata_sections, options::OPT_fno_data_sections,
                    UseSeparateSections))
     CmdArgs.push_back(
         Args.MakeArgString(Twine(PluginOptPrefix) + "-data-sections=1"));
-  else if (Args.hasArg(options::OPT_fno_data_sections))
+  else if (Args.hasArg(options::OPT_fno_data_sections)) {
+    DataSectionsTurnedOff = true;
     CmdArgs.push_back(
         Args.MakeArgString(Twine(PluginOptPrefix) + "-data-sections=0"));
+  }
 
   if (Args.hasArg(options::OPT_mxcoff_roptr) ||
       Args.hasArg(options::OPT_mno_xcoff_roptr)) {
     bool HasRoptr = Args.hasFlag(options::OPT_mxcoff_roptr,
                                  options::OPT_mno_xcoff_roptr, false);
     StringRef OptStr = HasRoptr ? "-mxcoff-roptr" : "-mno-xcoff-roptr";
 
     if (!IsOSAIX)
       D.Diag(diag::err_drv_unsupported_opt_for_target)
           << OptStr << ToolChain.getTriple().str();
 
     if (HasRoptr) {
-      if (!Args.hasFlag(options::OPT_fdata_sections,
-                        options::OPT_fno_data_sections, UseSeparateSections))
+      // The data sections option is on by default on AIX. We only need to error
+      // out when -fno-data-sections is specified explicitly to turn off data
+      // sections.
+      if (DataSectionsTurnedOff)
         D.Diag(diag::err_roptr_requires_data_sections);
 
       CmdArgs.push_back(
           Args.MakeArgString(Twine(PluginOptPrefix) + "-mxcoff-roptr"));
     }
   }
 
   // Pass an option to enable split machine functions.

clang/test/Driver/ppc-roptr.c

 // RUN: %clang -### --target=powerpc-ibm-aix-xcoff -mxcoff-roptr %s 2>&1 | \
 // RUN:     FileCheck %s --check-prefixes=ROPTR,LINK
 // RUN: %clang -### --target=powerpc-ibm-aix-xcoff -c -mxcoff-roptr %s 2>&1 | \
 // RUN:     FileCheck %s --check-prefix=ROPTR
 // RUN: %clang -### --target=powerpc-ibm-aix-xcoff -mxcoff-roptr -mno-xcoff-roptr %s 2>&1 | \
 // RUN:     FileCheck %s --check-prefix=NO_ROPTR
 
 // RUN: %clang -### --target=powerpc64-ibm-aix-xcoff -mxcoff-roptr %s 2>&1 | \
 // RUN:     FileCheck %s --check-prefixes=ROPTR,LINK
 // RUN: %clang -### --target=powerpc64-ibm-aix-xcoff -S -mxcoff-roptr %s 2>&1 | \
 // RUN:     FileCheck %s --check-prefix=ROPTR
 // RUN: %clang -### --target=powerpc-ibm-aix-xcoff %s 2>&1 | \
 // RUN:     FileCheck %s --check-prefix=NO_ROPTR
 // RUN: %clang -### --target=powerpc64-ibm-aix-xcoff -mxcoff-roptr -flto %s 2>&1 | \
-// RUN:     FileCheck %s --check-prefixes=ROPTR,LINK,LTO_ROPTR
+// RUN:     FileCheck %s --check-prefixes=NO_DATA_SECTION_ERR,ROPTR,LINK,LTO_ROPTR
 // RUN: touch %t.o
 // RUN: %clang -### --target=powerpc64-ibm-aix-xcoff -mxcoff-roptr %t.o 2>&1 | \
 // RUN:     FileCheck %s --check-prefix=LINK
 
 // RUN: %clang -### --target=powerpc64le-unknown-linux-gnu -mxcoff-roptr \
 // RUN:     %s 2>&1 | FileCheck %s --check-prefix=TARGET_ROPTR_ERR
 // RUN: %clang -### --target=powerpc64le-unknown-linux-gnu -mno-xcoff-roptr \
 // RUN:     %s 2>&1 | FileCheck %s --check-prefix=TARGET_NOROPTR_ERR
 // RUN: touch %t.o
 // RUN: %clang -### --target=powerpc64-ibm-aix-xcoff -mxcoff-roptr -shared \
 // RUN:     %t.o 2>&1 | FileCheck %s --check-prefix=SHARED_ERR
 // RUN: %clang -### --target=powerpc64le-unknown-linux-gnu -mxcoff-roptr -flto \
 // RUN:     %t.o 2>&1 | FileCheck %s --check-prefix=TARGET_ROPTR_ERR
 // RUN: %clang -### --target=powerpc64-ibm-aix-xcoff -mxcoff-roptr -flto -fno-data-sections \
 // RUN:     %t.o 2>&1 | FileCheck %s --check-prefix=DATA_SECTION_ERR
 // RUN: %clang -### --target=powerpc64-ibm-aix-xcoff -mno-xcoff-roptr -flto -fno-data-sections \
 // RUN:     %t.o 2>&1 | FileCheck %s --check-prefix=NO_DATA_SECTION_ERR
 // RUN: %clang -### --target=powerpc64le-unknown-linux-gnu -mno-xcoff-roptr -flto \
 // RUN:     %t.o 2>&1 | FileCheck %s --check-prefix=TARGET_NOROPTR_ERR
 
-// ROPTR: "-mxcoff-roptr"
-// LINK: "-bforceimprw"
-// LTO_ROPTR: "-bplugin_opt:-mxcoff-roptr"
-// NO_ROPTR-NOT: "-mxcoff-roptr"
-// NO_ROPTR-NOT: "-bforceimprw"
-
 // DATA_SECTION_ERR: error: -mxcoff-roptr is supported only with -fdata-sections
 // NO_DATA_SECTION_ERR-NOT: error: -mxcoff-roptr is supported only with -fdata-sections
 // TARGET_ROPTR_ERR: error: unsupported option '-mxcoff-roptr' for target 'powerpc64le-unknown-linux-gnu'
 // TARGET_NOROPTR_ERR: error: unsupported option '-mno-xcoff-roptr' for target 'powerpc64le-unknown-linux-gnu'
 // SHARED_ERR: error: -mxcoff-roptr is not supported with -shared
+
+// ROPTR: "-mxcoff-roptr"
+// LINK: "-bforceimprw"
+// LTO_ROPTR: "-bplugin_opt:-mxcoff-roptr"
+// NO_ROPTR-NOT: "-mxcoff-roptr"
+// NO_ROPTR-NOT: "-bforceimprw"