Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AIX][TOC] Fix buildbot failures from commit b4ae8df #85303

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

syzaara
Copy link
Contributor

@syzaara syzaara commented Mar 14, 2024

The following tests fail when built with Address
and Undefined sanitizers:
CodeGen/PowerPC/basic-toc-data-def.ll
CodeGen/PowerPC/toc-data-large-array2.ll

Subtarget may be null in emitGlobalVariable, for example in the testcase where we have no functions in the IR. The fix moves this function from PPCSubtarget to a static helper function. This only fails with sanitizers because the Subtarget is not used in the member function.

The following tests fail when built with Address
and Undefined sanitizers:
CodeGen/PowerPC/basic-toc-data-def.ll
CodeGen/PowerPC/toc-data-large-array2.ll

Subtarget may be null in emitGlobalVariable, for example in the testcase
where we have no functions in the IR. The fix moves this function from
PPCSubtarget to a static helper function.
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 14, 2024

@llvm/pr-subscribers-backend-powerpc

Author: Zaara Syeda (syzaara)

Changes

The following tests fail when built with Address
and Undefined sanitizers:
CodeGen/PowerPC/basic-toc-data-def.ll
CodeGen/PowerPC/toc-data-large-array2.ll

Subtarget may be null in emitGlobalVariable, for example in the testcase where we have no functions in the IR. The fix moves this function from PPCSubtarget to a static helper function.


Full diff: https://github.com/llvm/llvm-project/pull/85303.diff

3 Files Affected:

  • (modified) llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp (+22-1)
  • (modified) llvm/lib/Target/PowerPC/PPCSubtarget.cpp (-22)
  • (modified) llvm/lib/Target/PowerPC/PPCSubtarget.h (-2)
diff --git a/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp b/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
index 542854ec9b99fd..45f446bd2042f0 100644
--- a/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
+++ b/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
@@ -2651,6 +2651,27 @@ uint64_t PPCAIXAsmPrinter::getAliasOffset(const Constant *C) {
   return 0;
 }
 
+static void tocDataChecks(unsigned PointerSize, const GlobalVariable *GV) {
+  // TODO: These asserts should be updated as more support for the toc data
+  // transformation is added (struct support, etc.).
+  assert(
+      PointerSize >= GV->getAlign().valueOrOne().value() &&
+      "GlobalVariables with an alignment requirement stricter than TOC entry "
+      "size not supported by the toc data transformation.");
+
+  Type *GVType = GV->getValueType();
+  assert(GVType->isSized() && "A GlobalVariable's size must be known to be "
+                              "supported by the toc data transformation.");
+  if (GV->getParent()->getDataLayout().getTypeSizeInBits(GVType) >
+      PointerSize * 8)
+    report_fatal_error(
+        "A GlobalVariable with size larger than a TOC entry is not currently "
+        "supported by the toc data transformation.");
+  if (GV->hasPrivateLinkage())
+    report_fatal_error("A GlobalVariable with private linkage is not "
+                       "currently supported by the toc data transformation.");
+}
+
 void PPCAIXAsmPrinter::emitGlobalVariable(const GlobalVariable *GV) {
   // Special LLVM global arrays have been handled at the initialization.
   if (isSpecialLLVMGlobalArrayToSkip(GV) || isSpecialLLVMGlobalArrayForStaticInit(GV))
@@ -2660,7 +2681,7 @@ void PPCAIXAsmPrinter::emitGlobalVariable(const GlobalVariable *GV) {
   // when we emit the .toc section.
   if (GV->hasAttribute("toc-data")) {
     unsigned PointerSize = GV->getParent()->getDataLayout().getPointerSize();
-    Subtarget->tocDataChecks(PointerSize, GV);
+    tocDataChecks(PointerSize, GV);
     TOCDataGlobalVars.push_back(GV);
     return;
   }
diff --git a/llvm/lib/Target/PowerPC/PPCSubtarget.cpp b/llvm/lib/Target/PowerPC/PPCSubtarget.cpp
index 884f2f5c57b258..5380ec1c4c0d9c 100644
--- a/llvm/lib/Target/PowerPC/PPCSubtarget.cpp
+++ b/llvm/lib/Target/PowerPC/PPCSubtarget.cpp
@@ -185,28 +185,6 @@ bool PPCSubtarget::enableSubRegLiveness() const {
   return UseSubRegLiveness;
 }
 
-void PPCSubtarget::tocDataChecks(unsigned PointerSize,
-                                 const GlobalVariable *GV) const {
-  // TODO: These asserts should be updated as more support for the toc data
-  // transformation is added (struct support, etc.).
-  assert(
-      PointerSize >= GV->getAlign().valueOrOne().value() &&
-      "GlobalVariables with an alignment requirement stricter than TOC entry "
-      "size not supported by the toc data transformation.");
-
-  Type *GVType = GV->getValueType();
-  assert(GVType->isSized() && "A GlobalVariable's size must be known to be "
-                              "supported by the toc data transformation.");
-  if (GV->getParent()->getDataLayout().getTypeSizeInBits(GVType) >
-      PointerSize * 8)
-    report_fatal_error(
-        "A GlobalVariable with size larger than a TOC entry is not currently "
-        "supported by the toc data transformation.");
-  if (GV->hasPrivateLinkage())
-    report_fatal_error("A GlobalVariable with private linkage is not "
-                       "currently supported by the toc data transformation.");
-}
-
 bool PPCSubtarget::isGVIndirectSymbol(const GlobalValue *GV) const {
   // Large code model always uses the TOC even for local symbols.
   if (TM.getCodeModel() == CodeModel::Large)
diff --git a/llvm/lib/Target/PowerPC/PPCSubtarget.h b/llvm/lib/Target/PowerPC/PPCSubtarget.h
index d913f22bd5ba9d..306a52dca8362b 100644
--- a/llvm/lib/Target/PowerPC/PPCSubtarget.h
+++ b/llvm/lib/Target/PowerPC/PPCSubtarget.h
@@ -245,8 +245,6 @@ class PPCSubtarget : public PPCGenSubtargetInfo {
   /// True if the GV will be accessed via an indirect symbol.
   bool isGVIndirectSymbol(const GlobalValue *GV) const;
 
-  void tocDataChecks(unsigned PointerSize, const GlobalVariable *GV) const;
-
   /// True if the ABI is descriptor based.
   bool usesFunctionDescriptors() const {
     // Both 32-bit and 64-bit AIX are descriptor based. For ELF only the 64-bit

Copy link
Contributor

@mandlebug mandlebug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with 1 minor comment - I would suggest adding to the description that this only fails with sanitizers because the sub target is not used in the member function and so we never use the NULL this pointer.

@syzaara syzaara merged commit 00ba2a6 into llvm:main Mar 14, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants