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

[SystemZ][zOS] Override emitGlobalAlias for ADA #84829

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ysyeda
Copy link
Contributor

@ysyeda ysyeda commented Mar 11, 2024

This PR overrides emitGlobalAlias to properly handle the indirect symbol case in the ADA for GlobalAlias.

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 11, 2024

@llvm/pr-subscribers-backend-systemz

Author: Yusra Syeda (ysyeda)

Changes

This PR overrides emitGlobalAlias to properly handle the indirect symbol case in the ADA for GlobalAlias.


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

4 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/AsmPrinter.h (+3-1)
  • (modified) llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp (+41-8)
  • (modified) llvm/lib/Target/SystemZ/SystemZAsmPrinter.h (+1)
  • (modified) llvm/test/CodeGen/SystemZ/zos-ada-relocations.ll (+1-3)
diff --git a/llvm/include/llvm/CodeGen/AsmPrinter.h b/llvm/include/llvm/CodeGen/AsmPrinter.h
index a7fbf4aeb74494..857a39a6b71ce6 100644
--- a/llvm/include/llvm/CodeGen/AsmPrinter.h
+++ b/llvm/include/llvm/CodeGen/AsmPrinter.h
@@ -897,12 +897,14 @@ class AsmPrinter : public MachineFunctionPass {
   virtual void emitModuleCommandLines(Module &M);
 
   GCMetadataPrinter *getOrCreateGCPrinter(GCStrategy &S);
+
+protected:
   virtual void emitGlobalAlias(const Module &M, const GlobalAlias &GA);
-  void emitGlobalIFunc(Module &M, const GlobalIFunc &GI);
 
 private:
   /// This method decides whether the specified basic block requires a label.
   bool shouldEmitLabelForBasicBlock(const MachineBasicBlock &MBB) const;
+  void emitGlobalIFunc(Module &M, const GlobalIFunc &GI);
 
 protected:
   virtual bool shouldEmitWeakSwiftAsyncExtendedFramePointerFlags() const {
diff --git a/llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp b/llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp
index 5696ae117d69f0..b69dfad5227dc9 100644
--- a/llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp
@@ -26,6 +26,7 @@
 #include "llvm/MC/MCInstBuilder.h"
 #include "llvm/MC/MCSectionELF.h"
 #include "llvm/MC/MCStreamer.h"
+#include "llvm/MC/MCSymbolGOFF.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/Chrono.h"
 #include "llvm/Support/ConvertEBCDIC.h"
@@ -1026,17 +1027,15 @@ void SystemZAsmPrinter::emitADASection() {
       EmittedBytes += PointerSize;
       break;
     case SystemZII::MO_ADA_INDIRECT_FUNC_DESC: {
-      MCSymbol *Alias = OutContext.createTempSymbol(
-          Twine(Sym->getName()).concat("@indirect"));
-      OutStreamer->emitAssignment(Alias,
-                                  MCSymbolRefExpr::create(Sym, OutContext));
-      OutStreamer->emitSymbolAttribute(Alias, MCSA_IndirectSymbol);
+      MCSymbolGOFF *IndirectSym =
+          static_cast<MCSymbolGOFF *>(OutContext.getOrCreateSymbol(
+              Twine(Sym->getName()).concat("@indirect")));
 
       EMIT_COMMENT("pointer to function descriptor");
       OutStreamer->emitValue(
-          SystemZMCExpr::create(SystemZMCExpr::VK_SystemZ_VCon,
-                                MCSymbolRefExpr::create(Alias, OutContext),
-                                OutContext),
+          SystemZMCExpr::create(
+              SystemZMCExpr::VK_SystemZ_VCon,
+              MCSymbolRefExpr::create(IndirectSym, OutContext), OutContext),
           PointerSize);
       EmittedBytes += PointerSize;
       break;
@@ -1541,6 +1540,40 @@ void SystemZAsmPrinter::emitPPA2(Module &M) {
   OutStreamer->popSection();
 }
 
+void SystemZAsmPrinter::emitGlobalAlias(const Module &M,
+                                        const GlobalAlias &GA) {
+  if (!TM.getTargetTriple().isOSzOS()) {
+    AsmPrinter::emitGlobalAlias(M, GA);
+    return;
+  }
+
+  MCSymbol *Name = getSymbol(&GA);
+  bool IsFunc = isa<Function>(GA.getAliasee()->stripPointerCasts());
+
+  if (GA.hasExternalLinkage() || !MAI->getWeakRefDirective())
+    OutStreamer->emitSymbolAttribute(Name, MCSA_Global);
+  else if (GA.hasWeakLinkage() || GA.hasLinkOnceLinkage())
+    OutStreamer->emitSymbolAttribute(Name, MCSA_WeakReference);
+  else
+    assert(GA.hasLocalLinkage() && "Invalid alias linkage");
+
+  emitVisibility(Name, GA.getVisibility());
+
+  const MCExpr *Expr;
+
+  // For XPLINK, create a VCON relocation in case of a function, and
+  // a direct reference else.
+  MCSymbol *Sym = getSymbol(GA.getAliaseeObject());
+  if (IsFunc)
+    Expr = SystemZMCExpr::create(SystemZMCExpr::VK_SystemZ_VCon,
+                                 MCSymbolRefExpr::create(Sym, OutContext),
+                                 OutContext);
+
+  else
+    Expr = MCSymbolRefExpr::create(Sym, OutContext);
+  OutStreamer->emitAssignment(Name, Expr);
+}
+
 void SystemZAsmPrinter::emitFunctionEntryLabel() {
   const SystemZSubtarget &Subtarget = MF->getSubtarget<SystemZSubtarget>();
 
diff --git a/llvm/lib/Target/SystemZ/SystemZAsmPrinter.h b/llvm/lib/Target/SystemZ/SystemZAsmPrinter.h
index 303cce1a1b6581..4d9605725321cd 100644
--- a/llvm/lib/Target/SystemZ/SystemZAsmPrinter.h
+++ b/llvm/lib/Target/SystemZ/SystemZAsmPrinter.h
@@ -118,6 +118,7 @@ class LLVM_LIBRARY_VISIBILITY SystemZAsmPrinter : public AsmPrinter {
   void emitFunctionEntryLabel() override;
   void emitFunctionBodyEnd() override;
   void emitStartOfAsmFile(Module &M) override;
+  void emitGlobalAlias(const Module &M, const GlobalAlias &GA) override;
 
 private:
   void emitCallInformation(CallType CT);
diff --git a/llvm/test/CodeGen/SystemZ/zos-ada-relocations.ll b/llvm/test/CodeGen/SystemZ/zos-ada-relocations.ll
index e25246917ec099..6bdd42a9bd0a58 100644
--- a/llvm/test/CodeGen/SystemZ/zos-ada-relocations.ll
+++ b/llvm/test/CodeGen/SystemZ/zos-ada-relocations.ll
@@ -56,9 +56,7 @@ entry:
 declare signext i32 @callout(i32 signext)
 
 ; CHECK:     .section    ".ada"
-; CHECK:  .set @@DoFunc@indirect0, DoFunc
-; CHECK:      .indirect_symbol   @@DoFunc@indirect0
-; CHECK:  .quad V(@@DoFunc@indirect0)          * Offset 0 pointer to function descriptor DoFunc
+; CHECK:  .quad V(DoFunc@indirect)             * Offset 0 pointer to function descriptor DoFunc
 ; CHECK:  .quad R(Caller)                      * Offset 8 function descriptor of Caller
 ; CHECK:  .quad V(Caller)
 ; CHECK:  .quad A(i2)                           * Offset 24 pointer to data symbol i2

OutStreamer->emitSymbolAttribute(Alias, MCSA_IndirectSymbol);
MCSymbolGOFF *IndirectSym =
static_cast<MCSymbolGOFF *>(OutContext.getOrCreateSymbol(
Twine(Sym->getName()).concat("@indirect")));
Copy link
Member

Choose a reason for hiding this comment

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

This now just creates a completely new symbol, that is totally unrelated to the original symbol Name. How is that supposed to work? Where will this be associated back to the original symbol and its address?

Copy link
Contributor

Choose a reason for hiding this comment

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

One of the issues is that using the alias construct isn't entirely accurate. If Symbol A aliases Symbol B, then uses of A can (and are) be replaced by uses of B.

However, this isn't really the case here. The MO_ADA_INDIRECT_FUNC_DESC here means that we have a symbol that has a value containing the location of the function descriptor of the symbol it points to. I've included a diagram that (I hope) helps somewhat:

onno_art_is_hard

What we want to do here is to create a new symbol that points to a location in the ADA. That location contains the offset in the ADA to the location where the original symbol's ADA entry resides.

The GOFF writer creates an external reference (ErWx) symbol to represent this newly-created symbol in our symbol table (Specifically, the "external symbol dictionary"). This new symbol will have the same name as the original symbol, and that name is used by the binder to associate it back to the original symbol's address).

@@ -1541,6 +1540,40 @@ void SystemZAsmPrinter::emitPPA2(Module &M) {
OutStreamer->popSection();
}

void SystemZAsmPrinter::emitGlobalAlias(const Module &M,
const GlobalAlias &GA) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary? This seems unrelated to the above change - there was no GlobalAlias involved as far as I can see. Also, there doesn't appear to be any test case verifying what (if any) special handling is needed for GlobalAlias. (B.t.w. is that special handling actually z/OS related, or is it rather GOFF related? If the latter - there is XCOFF specific handling in emitGlobalAlias, so maybe the GOFF related special handling needs to go there too?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is required to complete the implementation for handling indirect symbols. In the case where there is a globalAlias, we require an external reference symbol with the indirect flag set which is handled by emitGlobalAlias.
I added a test for this case.

Copy link

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 41572177d129bf19f13f077a30b582fd3b8f790c 0a8ecf2d8122ea29c345cfb9f5506e304a0c679a -- llvm/include/llvm/CodeGen/AsmPrinter.h llvm/include/llvm/MC/MCSymbolGOFF.h llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp llvm/lib/Target/SystemZ/SystemZAsmPrinter.h
View the diff from clang-format here.
diff --git a/llvm/include/llvm/MC/MCSymbolGOFF.h b/llvm/include/llvm/MC/MCSymbolGOFF.h
index 9ef486d4af..cc9816c2c6 100644
--- a/llvm/include/llvm/MC/MCSymbolGOFF.h
+++ b/llvm/include/llvm/MC/MCSymbolGOFF.h
@@ -31,9 +31,7 @@ public:
   static bool classof(const MCSymbol *S) { return S->isGOFF(); }
 
   bool hasExternalName() const { return !ExternalName.empty(); }
-  void setExternalName(StringRef Name) {
-    ExternalName = Name;
-  }
+  void setExternalName(StringRef Name) { ExternalName = Name; }
   StringRef getExternalName() const { return ExternalName; }
 
   void setIndirect(bool Value = true) {

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