SystemZ: Remove override of insertSSPDeclarations

[arsenm]

The runtime library does not add a SYSTEM_CHECK_GUARD
implementation so the default will be a no-op anyway.

[arsenm]

• SystemZ: Remove override of insertSSPDeclarations #163708 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-backend-systemz

Author: Matt Arsenault (arsenm)

Changes

The runtime library does not add a SYSTEM_CHECK_GUARD
implementation so the default will be a no-op anyway.

---

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

1 Files Affected:

• (modified) llvm/lib/Target/SystemZ/SystemZISelLowering.h (-2)

diff --git a/llvm/lib/Target/SystemZ/SystemZISelLowering.h b/llvm/lib/Target/SystemZ/SystemZISelLowering.h
index d5b76031766dd..396e78e2845be 100644
--- a/llvm/lib/Target/SystemZ/SystemZISelLowering.h
+++ b/llvm/lib/Target/SystemZ/SystemZISelLowering.h
@@ -606,8 +606,6 @@ class SystemZTargetLowering : public TargetLowering {
 
   /// Override to support customized stack guard loading.
   bool useLoadStackGuardNode(const Module &M) const override { return true; }
-  void insertSSPDeclarations(Module &M) const override {
-  }
 
   MachineBasicBlock *
   EmitInstrWithCustomInserter(MachineInstr &MI,

[uweigand]

The runtime library does not add a SYSTEM_CHECK_GUARD implementation so the default will be a no-op anyway.

This doesn't look correct to me. Linux on SystemZ uses the LegacyDefaultSystemLibrary settings, which do provide the SYSTEM_CHECK_GUARD, and I verified that after removing the override, the behavior is no longer a no-op.

Now maybe we should have our own settings instead, but right now we do not. (Note that z/OS does have it's own settings, but z/OS is a very different operating system from Linux anyway.)

[arsenm]

This doesn't look correct to me. Linux on SystemZ uses the LegacyDefaultSystemLibrary settings, which do provide the SYSTEM_CHECK_GUARD, and I verified that after removing the override, the behavior is no longer a no-op.

Since no tests change with this, there is a problem. Can you add the missing test coverage @uweigand?

[uweigand]

This doesn't look correct to me. Linux on SystemZ uses the LegacyDefaultSystemLibrary settings, which do provide the SYSTEM_CHECK_GUARD, and I verified that after removing the override, the behavior is no longer a no-op.

Since no tests change with this, there is a problem. Can you add the missing test coverage @uweigand?

Well, there is a (somewhat minimalistic) test in test/CodeGen/SystemZ/stack-guard.ll. This does not show any difference in generated assembler output based on whether or not that declaration is visible - should it?

You can see differences in verbose dumps, e.g. current mainline:

  $r1l = EAR $a0, implicit-def $r1d
  $r1d = SLLG $r1d, $noreg, 32
  $r1l = EAR $a1
  renamable $r1d = LG $r1d, 40, $noreg
  CG killed renamable $r1d, $r15d, 1184, $noreg, implicit-def $cc :: (volatile load (s64) from %stack.0.StackGuardSlot)

vs. with your patch:

  $r1l = EAR $a0, implicit-def $r1d
  $r1d = SLLG $r1d, $noreg, 32
  $r1l = EAR $a1
  renamable $r1d = LG $r1d, 40, $noreg :: (dereferenceable invariant load (s64) from @__stack_chk_guard)
  CG killed renamable $r1d, $r15d, 1184, $noreg, implicit-def $cc :: (volatile load (s64) from %stack.0.StackGuardSlot)

Not sure if there is any more complex code sequence where that could result in observable assembler differences. Either way, it looks to me like current mainline (without the declaration) is more correct as we do not actually use any global variable __stack_chk_guard but rather a TLS slot (the EAR/SLLG/EAR sequence retrieves the address of the TLS area).