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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103202