Address review comments

Author: Zaara Syeda

clang/docs/UsersManual.rst

@@ -4046,6 +4046,7 @@ Suitable variables must:
 -  not have internal linkage
 -  not have aliases
 -  not have section attributes
+-  not be thread local storage
 
 The TOC data transformation results in the variable, not its address,
 being placed in the TOC. This eliminates the need to load the address of the

clang/include/clang/Basic/DiagnosticFrontendKinds.td

@@ -93,8 +93,8 @@ def err_fe_backend_error_attr :
 def warn_fe_backend_warning_attr :
   Warning<"call to '%0' declared with 'warning' attribute: %1">, BackendInfo,
   InGroup<BackendWarningAttributes>;
-def warn_toc_unsupported_type : Warning<"The -mtocdata option is ignored "
-  "for %0 because %1.">, InGroup<BackendWarningAttributes>;
+def warn_toc_unsupported_type : Warning<"-mtocdata option is ignored "
+  "for %0 because %1">, InGroup<BackendWarningAttributes>;
 
 def err_fe_invalid_code_complete_file : Error<
     "cannot locate code-completion file %0">, DefaultFatal;

clang/include/clang/Driver/Options.td

@@ -3520,25 +3520,23 @@ def fpass_plugin_EQ : Joined<["-"], "fpass-plugin=">,
   MetaVarName<"<dsopath>">,
   HelpText<"Load pass plugin from a dynamic shared object file (only with new pass manager).">,
   MarshallingInfoStringVector<CodeGenOpts<"PassPlugins">>;
-def mtocdata : Flag<["-"], "mtocdata">,
-  Visibility<[ClangOption, CLOption, DXCOption, CC1Option]>,
-  Flags<[TargetSpecific]>,
-  HelpText<"All suitable variables will have the TOC data transformation applied.">,
-  MarshallingInfoFlag<CodeGenOpts<"AllTocData">>;
-def mno_tocdata : Flag<["-"], "mno-tocdata">,
-  Visibility<[ClangOption, CLOption, DXCOption, CC1Option]>,
-  Flags<[TargetSpecific]>,
-  HelpText<"This is the default. TOC data transformation is not applied to any"
-           "variables. Only variables specified explicitly in -mtocdata= will"
-           "have the TOC data transformation.">;
+defm tocdata : BoolOption<"m","tocdata",
+  CodeGenOpts<"AllTocData">, DefaultFalse,
+  PosFlag<SetTrue, [], [ClangOption, CC1Option],
+  "All suitable variables will have the TOC data transformation applied">,
+  NegFlag<SetFalse, [], [ClangOption, CC1Option],
+  "This is the default. TOC data transformation is not applied to any"
+  "variables. Only variables specified explicitly in -mtocdata= will"
+  "have the TOC data transformation.">,
+  BothFlags<[], [ClangOption, CLOption]>>, Group<m_Group>;
 def mtocdata_EQ : CommaJoined<["-"], "mtocdata=">,
-  Visibility<[ClangOption, CLOption, DXCOption, CC1Option]>,
+  Visibility<[ClangOption, CC1Option]>,
   Flags<[TargetSpecific]>,
   HelpText<"Specifies a list of variables to which the TOC data transformation"
            "will be applied.">,
   MarshallingInfoStringVector<CodeGenOpts<"TocDataVarsUserSpecified">>;
 def mno_tocdata_EQ : CommaJoined<["-"], "mno-tocdata=">,
-  Visibility<[ClangOption, CLOption, DXCOption, CC1Option]>,
+  Visibility<[ClangOption, CC1Option]>,
   Flags<[TargetSpecific]>,
   HelpText<"Specifies a list of variables to be exempt from the TOC data"
            "transformation.">,

clang/lib/CodeGen/CodeGenModule.cpp

@@ -632,9 +632,7 @@ static void checkAliasForTocData(llvm::GlobalVariable *GVar,
   if (GVar->hasAttribute("toc-data")) {
     auto GVId = GVar->getGlobalIdentifier();
     // Is this a global variable specified by the user as local?
-    bool UserSpecifiedTOC =
-        llvm::binary_search(CodeGenOpts.TocDataVarsUserSpecified, GVId);
-    if (UserSpecifiedTOC) {
+    if ((llvm::binary_search(CodeGenOpts.TocDataVarsUserSpecified, GVId))) {
       Diags.Report(Location, diag::warn_toc_unsupported_type)
           << GVId << "the variable has an alias";
     }

clang/lib/CodeGen/Targets/PPC.cpp

@@ -271,57 +271,55 @@ bool AIXTargetCodeGenInfo::initDwarfEHRegSizeTable(
 
 void AIXTargetCodeGenInfo::setTargetAttributes(
     const Decl *D, llvm::GlobalValue *GV, CodeGen::CodeGenModule &M) const {
-  if (auto *GVar = dyn_cast<llvm::GlobalVariable>(GV)) {
-    auto GVId = M.getMangledName(dyn_cast<NamedDecl>(D));
-
-    // Is this a global variable specified by the user as toc-data?
-    bool UserSpecifiedTOC =
-        llvm::binary_search(M.getCodeGenOpts().TocDataVarsUserSpecified, GVId);
-    // Assumes the same variable cannot be in both TocVarsUserSpecified and
-    // NoTocVars.
-    if (UserSpecifiedTOC ||
-        ((M.getCodeGenOpts().AllTocData) &&
-         !llvm::binary_search(M.getCodeGenOpts().NoTocDataVars, GVId))) {
-      const unsigned long PointerSize =
-          GV->getParent()->getDataLayout().getPointerSizeInBits() / 8;
-      ASTContext &Context = D->getASTContext();
-      auto *VarD = dyn_cast<VarDecl>(D);
-      assert(VarD && "Invalid declaration of global variable.");
-
-      unsigned Alignment = Context.toBits(Context.getDeclAlign(D)) / 8;
-      const auto *Ty = VarD->getType().getTypePtr();
-      const RecordDecl *RDecl =
-          Ty->isRecordType() ? Ty->getAs<RecordType>()->getDecl() : nullptr;
-
-      bool EmitDiagnostic = UserSpecifiedTOC && GV->hasExternalLinkage();
-      if (!Ty || Ty->isIncompleteType()) {
-        if (EmitDiagnostic)
-          M.getDiags().Report(D->getLocation(), diag::warn_toc_unsupported_type)
-              << GVId << "of incomplete type";
-      } else if (RDecl && RDecl->hasFlexibleArrayMember()) {
-        if (UserSpecifiedTOC)
-          M.getDiags().Report(D->getLocation(), diag::warn_toc_unsupported_type)
-              << GVId << "it contains a flexible array member";
-      } else if (VarD->getTLSKind() != VarDecl::TLS_None) {
-        if (EmitDiagnostic)
-          M.getDiags().Report(D->getLocation(), diag::warn_toc_unsupported_type)
-              << GVId << "of thread local storage model";
-      } else if (PointerSize < Context.getTypeInfo(VarD->getType()).Width / 8) {
-        if (EmitDiagnostic)
-          M.getDiags().Report(D->getLocation(), diag::warn_toc_unsupported_type)
-              << GVId << "variable is larger than a pointer";
-      } else if (PointerSize < Alignment) {
-        if (EmitDiagnostic)
-          M.getDiags().Report(D->getLocation(), diag::warn_toc_unsupported_type)
-              << GVId << "variable is aligned wider than a pointer";
-      } else if (D->hasAttr<SectionAttr>()) {
-        if (EmitDiagnostic)
-          M.getDiags().Report(D->getLocation(), diag::warn_toc_unsupported_type)
-              << GVId << "of a section attribute";
-      } else if ((GV->hasExternalLinkage() || M.getCodeGenOpts().AllTocData) &&
-                 !GV->hasInternalLinkage()) {
-        GVar->addAttribute("toc-data");
-      }
+  if (!isa<llvm::GlobalVariable>(GV))
+    return;
+
+  auto *GVar = dyn_cast<llvm::GlobalVariable>(GV);
+  auto GVId = M.getMangledName(dyn_cast<NamedDecl>(D));
+
+  // Is this a global variable specified by the user as toc-data?
+  bool UserSpecifiedTOC =
+      llvm::binary_search(M.getCodeGenOpts().TocDataVarsUserSpecified, GVId);
+  // Assumes the same variable cannot be in both TocVarsUserSpecified and
+  // NoTocVars.
+  if (UserSpecifiedTOC ||
+      ((M.getCodeGenOpts().AllTocData) &&
+       !llvm::binary_search(M.getCodeGenOpts().NoTocDataVars, GVId))) {
+    const unsigned long PointerSize =
+        GV->getParent()->getDataLayout().getPointerSizeInBits() / 8;
+    ASTContext &Context = D->getASTContext();
+    auto *VarD = dyn_cast<VarDecl>(D);
+    assert(VarD && "Invalid declaration of global variable.");
+
+    unsigned Alignment = Context.toBits(Context.getDeclAlign(D)) / 8;
+    const auto *Ty = VarD->getType().getTypePtr();
+    const RecordDecl *RDecl =
+        Ty->isRecordType() ? Ty->getAs<RecordType>()->getDecl() : nullptr;
+
+    bool EmitDiagnostic = UserSpecifiedTOC && GV->hasExternalLinkage();
+    auto reportUnsupportedWarning = [&](bool ShouldEmitWarning, StringRef Msg) {
+      if (ShouldEmitWarning)
+        M.getDiags().Report(D->getLocation(), diag::warn_toc_unsupported_type)
+            << GVId << Msg;
+    };
+    if (!Ty || Ty->isIncompleteType()) {
+      reportUnsupportedWarning(EmitDiagnostic, "of incomplete type");
+    } else if (RDecl && RDecl->hasFlexibleArrayMember()) {
+      reportUnsupportedWarning(EmitDiagnostic,
+                               "it contains a flexible array member");
+    } else if (VarD->getTLSKind() != VarDecl::TLS_None) {
+      reportUnsupportedWarning(EmitDiagnostic, "of thread local storage");
+    } else if (PointerSize < Context.getTypeInfo(VarD->getType()).Width / 8) {
+      reportUnsupportedWarning(EmitDiagnostic,
+                               "variable is larger than a pointer");
+    } else if (PointerSize < Alignment) {
+      reportUnsupportedWarning(EmitDiagnostic,
+                               "variable is aligned wider than a pointer");
+    } else if (D->hasAttr<SectionAttr>()) {
+      reportUnsupportedWarning(EmitDiagnostic, "of a section attribute");
+    } else if ((GV->hasExternalLinkage() || M.getCodeGenOpts().AllTocData) &&
+               !GV->hasInternalLinkage()) {
+      GVar->addAttribute("toc-data");
     }
   }
 }

clang/lib/Driver/ToolChains/AIX.cpp

@@ -483,10 +483,8 @@ static void addTocDataOptions(const llvm::opt::ArgList &Args,
         ExplicitlySpecifiedGlobals.erase(Val);
   }
 
-  const bool HasExplicitValues = !ExplicitlySpecifiedGlobals.empty();
-
   auto buildExceptionList = [](const llvm::StringSet<> &ExplicitValues,
-                               const char *OptionSpelling) -> std::string {
+                               const char *OptionSpelling) {
     std::string Option(OptionSpelling);
     bool IsFirst = true;
     for (const auto &E : ExplicitValues) {
@@ -505,10 +503,11 @@ static void addTocDataOptions(const llvm::opt::ArgList &Args,
   // variables which would be exceptions to the default setting.
   const char *TocDataGlobalOption =
       TOCDataGloballyinEffect ? "-mtocdata" : "-mno-tocdata";
+  CC1Args.push_back(TocDataGlobalOption);
+
   const char *TocDataListOption =
       TOCDataGloballyinEffect ? "-mno-tocdata=" : "-mtocdata=";
-  CC1Args.push_back(TocDataGlobalOption);
-  if (HasExplicitValues)
+  if (const bool HasExplicitValues = !ExplicitlySpecifiedGlobals.empty())
     CC1Args.push_back(Args.MakeArgString(llvm::Twine(
         buildExceptionList(ExplicitlySpecifiedGlobals, TocDataListOption))));
 }

clang/test/CodeGen/PowerPC/toc-data-diagnostics.c

@@ -11,27 +11,27 @@ struct large_struct {
   char c;
 };
 
-struct large_struct a;                      // expected-warning {{The -mtocdata option is ignored for a because variable is larger than a pointer.}}
-long long b = 5;                            // expected-warning {{The -mtocdata option is ignored for b because variable is larger than a pointer.}}
-int __attribute__((aligned(128))) c = 0;    // expected-warning {{The -mtocdata option is ignored for c because variable is aligned wider than a pointer.}}
-double d = 2.5;                             // expected-warning {{The -mtocdata option is ignored for d because variable is larger than a pointer.}}
-int e __attribute__((section("foo"))) = 10; // expected-warning {{The -mtocdata option is ignored for e because of a section attribute.}}
-__thread int f;                             // expected-warning {{The -mtocdata option is ignored for f because of thread local storage model.}}
+struct large_struct a;                      // expected-warning {{-mtocdata option is ignored for a because variable is larger than a pointer}}
+long long b = 5;                            // expected-warning {{-mtocdata option is ignored for b because variable is larger than a pointer}}
+int __attribute__((aligned(128))) c = 0;    // expected-warning {{-mtocdata option is ignored for c because variable is aligned wider than a pointer}}
+double d = 2.5;                             // expected-warning {{-mtocdata option is ignored for d because variable is larger than a pointer}}
+int e __attribute__((section("foo"))) = 10; // expected-warning {{-mtocdata option is ignored for e because of a section attribute}}
+__thread int f;                             // expected-warning {{-mtocdata option is ignored for f because of thread local storage}}
 
 struct SomeStruct;
-extern struct SomeStruct g;                 // expected-warning {{The -mtocdata option is ignored for g because of incomplete type.}}
+extern struct SomeStruct g;                 // expected-warning {{-mtocdata option is ignored for g because of incomplete type}}
 
-extern int h[];                             // expected-warning {{The -mtocdata option is ignored for h because of incomplete type.}}
+extern int h[];                             // expected-warning {{-mtocdata option is ignored for h because of incomplete type}}
 
 struct ty3 {
   int A;
   char C[];
 };
-struct ty3 t3 = { 4, "fo" }; // expected-warning {{The -mtocdata option is ignored for t3 because it contains a flexible array member.}}
+struct ty3 t3 = { 4, "fo" }; // expected-warning {{-mtocdata option is ignored for t3 because it contains a flexible array member}}
 
 int globalOneWithAlias = 10;
-__attribute__((__alias__("globalOneWithAlias"))) extern int aliasOne; // expected-warning {{The -mtocdata option is ignored for globalOneWithAlias because the variable has an alias.}}
-__attribute__((__alias__("globalTwoWithAlias"))) extern int aliasTwo; // expected-warning {{The -mtocdata option is ignored for globalTwoWithAlias because the variable has an alias.}}
+__attribute__((__alias__("globalOneWithAlias"))) extern int aliasOne; // expected-warning {{-mtocdata option is ignored for globalOneWithAlias because the variable has an alias}}
+__attribute__((__alias__("globalTwoWithAlias"))) extern int aliasTwo; // expected-warning {{-mtocdata option is ignored for globalTwoWithAlias because the variable has an alias}}
 int globalTwoWithAlias = 20;