Page MenuHomePhabricator

[AIX] Add traceback ssp canary bit support
ClosedPublic

Authored by jsji on May 26 2021, 1:32 PM.

Details

Summary

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

Diff Detail

Event Timeline

jsji created this revision.May 26 2021, 1:32 PM
jsji requested review of this revision.May 26 2021, 1:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2021, 1:32 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2199

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 added inline comments.May 27 2021, 6:53 AM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2199

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 added inline comments.May 27 2021, 6:58 AM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2199

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.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2199

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 added a comment.May 27 2021, 11:06 AM

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
2199

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.

2199

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.

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.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2199

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.

2199

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 updated this revision to Diff 348430.May 27 2021, 7:16 PM

Add option control to enable this feature.

shchenz added inline comments.Jun 9 2021, 6:06 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2198

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

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2198

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 added inline comments.Jun 9 2021, 7:17 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2198

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 accepted this revision.Jun 9 2021, 7:30 PM

LGTM, thanks.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2198

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.

This revision is now accepted and ready to land.Jun 9 2021, 7:30 PM
This revision was landed with ongoing or failed builds.Jun 9 2021, 7:58 PM
This revision was automatically updated to reflect the committed changes.