[AIX] Add traceback ssp canary bit support

[jsji]

Summary

We will need to set the ssp canary bit in traceback table to communicate
with unwinder about the canary.

[hubert.reinterpretcast]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

2182

A patch like this one would need rather more tests to demonstrate that the insertion of the canary word is in a position that is compatible with where the unwinder and other stack-frame-walking tools would look.

[jsji]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

2182

I think this is chicken and egg issue: there is no SSP support in current libunwind support on AIX yet.
I would prefer we enable this bits first, we can and will definitely do more test while enable this support in libunwind.

[jsji]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

2182

Also, since there is NO *ABI* definition of SSP canary word position, I don't think we should assume that it is the compiler's job to ensure the compatibility of canary word, libunwind can do a better job in ensuring the compatibility with legacy compiler in such case.

[hubert.reinterpretcast]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

2182

It is the compiler's responsibility to ensure compatibility of the canary word so that the non-Clang-based IBM XL C++ stack unwinder continues to have whatever ability it did to unwind past C code (that just happens to be compiled with LLVM). Advanced debugging tools may also have an expectation about the positioning of the canary word.

As for the ability of libunwind itself to do anything reasonable with this bit, even if the specific positioning is not based on the IBM XL compiler, that there is a concise set of rules for the positioning is necessary. Enabling the bit without identifying such a concise set of rules in prose (that is, improving the documentation of the ABI) seems problematic to me.

[jsji]

I respectfully disagree with the requirement of mandating the canary word position.

But considering that libunwind hasn't support this,
I think we can guard this feature with an option, and disable it for now.
Doing this will make it easier for libunwind development and further testing.

Is that OK for you?

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

2182

It is the compiler's responsibility to ensure compatibility of the canary word so that the non-Clang-based IBM XL C++ stack unwinder continues to have whatever ability it did to unwind past C code (that just happens to be compiled with LLVM). Advanced debugging tools may also have an expectation about the positioning of the canary word.

I personally don't agree with this. It doesn't make sense to me to force clang-based compiler to ensure something with legacy unwinder just for mixing linkings -- which is not guarantee to work anyway.

Advanced debugging tools may also have an expectation about the positioning of the canary word.

As I mentioned, there was NO mention of canary word position in ABI, the tooling shouldn't have *expectation* of fixed position. Also our new clang based compiler does NOT claim to be backward compatibility with all existing toolings.
Also, I don't know of any of such advanced debugging tools so far.

2182

As for the ability of libunwind itself to do anything reasonable with this bit, even if the specific positioning is not based on the IBM XL compiler, that there is a concise set of rules for the positioning is necessary. Enabling the bit without identifying such a concise set of rules in prose (that is, improving the documentation of the ABI) seems problematic to me.

I don't see the benefit justify the effort here.

[hubert.reinterpretcast]

In D103202#2785332, @jsji wrote:

I think we can guard this feature with an option, and disable it for now.
Doing this will make it easier for libunwind development and further testing.

Is that OK for you?

Yes, an option control (maybe -mllvm and not a top-level "Clang driver" option) sounds good.

[hubert.reinterpretcast]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

2182

It doesn't make sense to me to force clang-based compiler to ensure something with legacy unwinder just for mixing linkings -- which is not guarantee to work anyway.

My understanding is that there is a design intent for C code to be usable in mixed scenarios.

2182

I don't see the benefit justify the effort here.

Given the additional option control, I'm fine with deferring this to a later patch that enables this functionality by default.

[jsji]

Add option control to enable this feature.

[jsji]

Ping...

[jsji]

Ping ...

[shchenz]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

2181

Will it cause some issue in unwinder if we set the ssp canary bit in the traceback table but there is no canary word in the stack?

Could you please comment @xingxue @DiggerLin @hubert.reinterpretcast

[hubert.reinterpretcast]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

2181

The FIXME indicates a problem (as is appropriate for a FIXME). Even if it is possible to recover from this by more complicated logic in the unwinder, the preference should be for it to be resolved by the compiler (pay for it at compile time). The patch does not have to be blocked by the pending FIXME, because the bit is only set when an internal/experimental flag is set.

[jsji]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

2181

As I mentioned, there is NO unwinder support yet, so this is chicken and egg issue. We can fix this in a following patch if the unwinder will not able to handle it and complain.

Also this is a very corner case -- with legacy XL, we set this bit on whenever the options is on, no checking, and we never hit problem -- so legacy unwinder can tolerate this.

[shchenz]

LGTM, thanks.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

2181

OK, LGTM then.

We can revisit the impact of the redundant ssp canary bit later when unwinder supports this. And also this flag will not be set by default in the compiler.

llvm/include/llvm/CodeGen/TargetLoweringObjectFileImpl.h

 };
 
 class TargetLoweringObjectFileXCOFF : public TargetLoweringObjectFile {
 public:
   TargetLoweringObjectFileXCOFF() = default;
   ~TargetLoweringObjectFileXCOFF() override = default;
 
   static bool ShouldEmitEHBlock(const MachineFunction *MF);
+  static bool ShouldSetSSPCanaryBitInTB(const MachineFunction *MF);
 
   static MCSymbol *getEHInfoTableSymbol(const MachineFunction *MF);
 
   void Initialize(MCContext &Ctx, const TargetMachine &TM) override;
 
   bool shouldPutJumpTableInFunctionSection(bool UsesLabelDifference,
                                            const Function &F) const override;
 

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

const GlobalValue *Per =
       dyn_cast<GlobalValue>(F.getPersonalityFn()->stripPointerCasts());
   assert(Per && "Personality routine is not a GlobalValue type.");
   if (isNoOpWithoutInvoke(classifyEHPersonality(Per)))
     return false;
 
   return true;
 }
 
+bool TargetLoweringObjectFileXCOFF::ShouldSetSSPCanaryBitInTB(
+    const MachineFunction *MF) {
+  const Function &F = MF->getFunction();
+  if (!F.hasStackProtectorFnAttr())
+    return false;
+  // FIXME: check presence of canary word
+  // There are cases that the stack protectors are not really inserted even if
+  // the attributes are on.
+  return true;
+}
+
 MCSymbol *
 TargetLoweringObjectFileXCOFF::getEHInfoTableSymbol(const MachineFunction *MF) {
   return MF->getMMI().getContext().getOrCreateSymbol(
       "__ehinfo." + Twine(MF->getFunctionNumber()));
 }
 
 MCSymbol *
 TargetLoweringObjectFileXCOFF::getTargetSymbol(const GlobalValue *GV,

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

if (FirstHalfOfMandatoryField & TracebackTable::IsAllocaUsedMask) {
     OutStreamer->AddComment("AllocaUsed");
     OutStreamer->emitIntValueInHex(AllocReg, sizeof(AllocReg));
   }
 
   uint8_t ExtensionTableFlag = 0;
   if (SecondHalfOfMandatoryField & TracebackTable::HasExtensionTableMask) {
     if (ShouldEmitEHBlock)
       ExtensionTableFlag |= ExtendedTBTableFlag::TB_EH_INFO;
+    if (TargetLoweringObjectFileXCOFF::ShouldSetSSPCanaryBitInTB(MF))
+      ExtensionTableFlag |= ExtendedTBTableFlag::TB_SSP_CANARY;
 
     CommentOS << "ExtensionTableFlag = "
               << getExtendedTBTableFlagString(ExtensionTableFlag);
     EmitCommentAndValue(ExtensionTableFlag, sizeof(ExtensionTableFlag));
   }
 
   if (ExtensionTableFlag & ExtendedTBTableFlag::TB_EH_INFO) {
     auto &Ctx = OutStreamer->getContext();

llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-ssp.ll

+; RUN: llc -mtriple=powerpc64-ibm-aix-xcoff -O0 < %s | FileCheck %s
+
+; CHECK-LABEL: f:
+; CHECK: __ssp_canary_word
+; CHECK: TB_SSP_CANARY
+define i32 @f() #0 personality i8* bitcast (i32 (...)* @__xlcxx_personality_v1 to i8*) {
+  invoke i32 undef(i8* undef)
+    to label %invoke unwind label %lpad
+
+  invoke:
+    %var = alloca i32, align 4
+    store i32 0, i32* %var, align 4
+    %gep = getelementptr inbounds i32, i32* %var, i32 1
+    %ret = load i32, i32* %gep, align 4
+    ret i32 %ret
+  lpad:
+    landingpad { i8*, i32 }
+  catch i8* null
+    unreachable
+
+}
+
+; CHECK-LABEL: f2:
+; CHECK: __ssp_canary_word
+; Not emitting traceback bit when no unwinding needed.
+; CHECK-NOT: TB_SSP_CANARY
+define i32 @f2() #0 {
+  %var = alloca i32, align 4
+  store i32 0, i32* %var, align 4
+  %gep = getelementptr inbounds i32, i32* %var, i32 1
+  %ret = load i32, i32* %gep, align 4
+  ret i32 %ret
+}
+
+declare i32 @__xlcxx_personality_v1(...)
+attributes #0 = { sspstrong }
+