[PATCH] D103202: [AIX] Add traceback ssp canary bit support

Thu May 27 11:06:26 PDT 2021

jsji added a comment.

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?

================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:2182
+  // the attributes are on.
+  return true;
+}
----------------
hubert.reinterpretcast wrote:
> jsji wrote:
> > jsji wrote:
> > > hubert.reinterpretcast wrote:
> > > > 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.
> > > 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.
> > 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.
> 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.
> 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.

================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:2182
+  // the attributes are on.
+  return true;
+}
----------------
jsji wrote:
> hubert.reinterpretcast wrote:
> > jsji wrote:
> > > jsji wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > 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.
> > > > 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.
> > > 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.
> > 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.
> > 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.
> 
> 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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103202/new/

https://reviews.llvm.org/D103202