Commit: rG7f0244afa828: [libc++abi] NFC: adding a new parameter base to functions for calculating… (authored by xingxue). Closing.

TBH, cxa_personality is somewhat below what I usually touch, so I don't really have a concern here.

I'd like to note that this patch does trigger -Wunused-parameter, though - would it be possible to fix those, or are those seemingly unused parameters actually necessary to reduce the diff between upstream and a downstream fork?

Removed the dependent patch which has landed. Try to trigger the pre-merge check.

Addressed comments:

• Added parameter base to readEncodedPointer() calls.

Ping.

@libc++abi, @mclow.lists Hi, This NFC patch has been reviewed and approved by @sfertile but it needs the approval from blocking reviewer the libc++abi group before it can be landed. We are working on the Clang compiler for the AIX platform and libunwind/libc++abi are used for EH. For AIX, the DW_EH_PE_datarel encoding is used for calculating relative addresses in the range table. Therefore, the code for scanning the range table is extended to have one more parameter base. This NFC patch and the follow-on patch https://reviews.llvm.org/D101298 are the changes needed to process the range table for Clang on AIX. It is important to us to get the changes reviewed and landed to unblock the buildbot. It would be greatly appreciated if the group can take a look and let us know if there are any concerns.

Changes:

• Dynamically resolve symbol __xlcxx_personality_v0 that is the personality for the state table so that libunwind does not have hard dependency on libc++abi on AIX.

Hi @MaskRay, Do you have any further comments/concerns? Thanks!

Hi @MaskRay, Do you have any further comments/concerns? Thanks!

Changes from clang-format.

Rebased after changes for PowerPC assembly code in NFC patch (https://reviews.llvm.org/D101179).

Minor change from clang-format.

In D101179#2736893, @MaskRay wrote:

Do we need GPR? ld 0, 4(3) works as ld r0, 4(r3) in GNU flavored assembly. Other macros may be similar.

Addressed comments:

• Changed the representation of PowperPC registers from %rn, %fn, %vsn, and %vrn to the register number alone. The GNU flavored assembler and the AIX assembler can determine the register kind based on the context of the instruction in which the register is used.

Hi @MaskRay @compnerd, Do you have any further comments?

Refreshed after changes in the base NFC patch https://reviews.llvm.org/D101545.

Addressed comment:

In D101545#2726036, @MaskRay wrote:

I keep feeling that the AIX implementation probably needs its own file. Making Itanium/ARM/SEH share some functions actually make generic code improvement harder.

Refreshed to base on the split out NFC patch https://reviews.llvm.org/D101545.

This version is clean of format changes made by clang-format.

In D101179#2721113, @compnerd wrote:

While I think that this is fine in general, I'm not sure why the shift operator needs to be replaced. Could you shed some light on that?

Removed the incorrect setting of parent revision. Reload the patch to trigger the pre-merge check. Sorry for the noise.

Changes:

• Split off changes for enabling calculating relative addresses with the DW_EH_PE_datarel encoding using a base (https://reviews.llvm.org/D101298).
• Reverted to making __xlcxx_personality_v1 an alias of __gxx_personality_v0.

In D100504#2703703, @MaskRay wrote:

In sanitizer runtime, we create separate files and duplicate some needed functions when a platform has sufficient different implementation. See compiler-rt/lib/sanitizer_common/*.cpp and compiler-lib/lib/lsan/lsan_*.cpp for examples.

I have simplified the Itanium LSDA a lot. From my experience the code sharing with arm EHABI actually made it really different to refactor.

Addressed comment:

• Split off the NFC part of the macro changes for PowerPC assembly code (https://reviews.llvm.org/D101179).

Changes:

• Added #include <stdio.h> for fprintf() as suggested.
• Make __xlcxx_personality_v1 a wrapper of __gxx_personality_v0.

Fixed pre-merge issues for Arm.

Fixed some more assembly issues from the formatter.

Dear Reviewers.

Fixed the issue caused by git-clang-format in file assembly.h.

Renamed variables in Unwind_AIXExtras.cpp.

Updated after running git-clang-format.

Addressed comments:

• Added a comment as suggested.

Addressed comments:

• Leave the existing handling of type code g unchanged as suggested.

Revision no longer needed now that the GO binding test code has been reverted. Thanks!

Addressed comments:

In D74163#1891408, @uweigand wrote:

In D74163#1891378, @xingxue wrote:

I'm still wondering about Intel. Can there ever be a literal encoded using 'g' on Intel? If yes, then treating it as "long double" would still be wrong, because 'g' encodes IEEE128 (__float128), while "long double" is the Intel extended (80-bit) format, right?

On the other hand, if 'g' encoded literals can never happen on Intel (or other platforms), maybe it would be better to have the code handling 'g' within a #ifdef section only active on powerpc and s390?

For X86, 'e' is used for 80-bit long double and 'g' is used for 128-bit long double. The following is the code in Clang.

clang/lib/Basic/Targets/X86.h
....
const char *getLongDoubleMangling() const override {
  return LongDoubleFormat == &llvm::APFloat::IEEEquad() ? "g" : "e";
}
...

But then this patch must be incorrect, given that it does

return getDerived().template parseFloatingLiteral<long double>();

for both the 'e' and 'g' cases on Intel. Now I guess it depends on how the file is compiled (with -mlong-double-80 or -mlong-double-128), but either way, one of the cases will be handled incorrectly.

I'm still wondering about Intel. Can there ever be a literal encoded using 'g' on Intel? If yes, then treating it as "long double" would still be wrong, because 'g' encodes IEEE128 (__float128), while "long double" is the Intel extended (80-bit) format, right?

On the other hand, if 'g' encoded literals can never happen on Intel (or other platforms), maybe it would be better to have the code handling 'g' within a #ifdef section only active on powerpc and s390?

For X86, 'e' is used for 80-bit long double and 'g' is used for 128-bit long double. The following is the code in Clang.

clang/lib/Basic/Targets/X86.h
....
const char *getLongDoubleMangling() const override {
  return LongDoubleFormat == &llvm::APFloat::IEEEquad() ? "g" : "e";
}
...

Hi @uweigand, Thanks for your comments. Please see my explanations below.

@uweigand, Hi, I've addressed your comments. Any further comments?

LGTM.

Addressed comments:

Test file aix-xcoff-data.ll needs a rebase.

LGTM.

Revised based on changes in patch https://reviews.llvm.org/D69112.

Addressed comments:

• Changed variable name CSection -> Csect;
• Added the checking of section headers in the object file.

Changed from Data to Data.Index.

Addressed comments:

• use csects in comment.
• use section storage class from CSection.
• set the section start address as the address after the previous section.

This patch needs a rebase.

Merged with the master and fixed a typo.

Addressed comments:

Addressed comments:

LGTM.

LGTM.