User Details
- User Since
- Jun 17 2019, 7:18 AM (196 w, 6 d)
Feb 1 2023
Jan 16 2023
I've reduced a regression on:
Jan 10 2023
I had a typo in the commit so this didn't auto close.
Jan 3 2023
I suggest adding a reproducer for the specific error we're seeing on AIX. I expect there is an issue when the affected bad_function_call symbols are generated in user objects with hidden visibility, which conflict with the definition provided in the library. A test would document this understanding.
Nov 30 2022
Nov 29 2022
Nov 23 2022
LGTM.
Nov 22 2022
Looks good but I just want to confirm that the profiled -L paths are appended in the correct order with respect to the non-profiled paths.
Nov 10 2022
I believe the fix for a52d151f9dde7 inadvertently exposed a code path where by the linkage of a static local of a static function, which would otherwise return LinkageInfo::none() may now return VisibleNoLinkage depending on the incoming computation argument.
Nov 9 2022
Test only patch is safe to commit.
Test only patch is safe to commit.
Nov 7 2022
Suggest: [Test][AIX][p] Add 64-bit driver expected output
Use the [Test] tag in the abstract.
Oct 25 2022
I have no further concerns with this patch and can confirm the solution works for EH support of the XL C++ V16 xlclang++ compiler. No further concerns.
Oct 21 2022
Oct 20 2022
Oct 11 2022
Oct 7 2022
Jul 21 2022
The new test passes/fails based on wchar_t/wint_t types, which is tedious to manage across targets with XFAIL so I've decided to enable it only for AIX/zOS targets where it is expected to pass.
Jul 20 2022
New test fails on macos targets (as it should) but I needed to correct a typo on the XFAIL line.
Jul 19 2022
Formatting and typo.
Thanks for catching the formatting problems Xing. I'll need to look at my workflow to see why I didn't catch those.
Jul 17 2022
Fix formatting and a typo in the __config
Jul 15 2022
- Only add fill set as needed for _LIBCPP_ABI > 1 for all targets
- zOS 64-bit always needs fill set for compatibility
- tweaked the test to use the char value for eof as it exposes the problem for both signed and unsigned char types
Plan to revise the config to pick up the "fill set member as needed" mode in the config for all targets when _LIBCPP_ABI_VERSION >= 2.
Jun 30 2022
Jun 25 2022
Quick testcase fix.
Any basic_ios where sizeof(char_type) >= sizeof(int_type) has at least some char value that overlaps with the eof character. I've modified the proposed test to demonstrate signed char types are affected at -1.
Jun 24 2022
Writing revision to use SentinelFill/OptionalFill classes.
Jun 23 2022
Jun 22 2022
- The selection of the fill set member is moved to the __config.
- This patch retains ABI compatibility for any of the libc++ CI that reported failures on this issue.
- Always inject the fill set member for AIX to retain compatibility for AIX.
- This only fixes basic_ios<wchar_t>. It is not compliant to instantiate basic_ios with a char type where char_traits::eof is not a sentinel value. #wchar_t is an exception because C++ shouldn't be adding constraints beyond what C guarantees.
Jun 20 2022
Jun 15 2022
Jun 14 2022
The new approach injects the new __fill_set_ only for wchar_t and only if WEOF is in the range [WCHAR_MIN, WCHAR_MAX]. I didn't use numeric_limits because the solution needs to work in C++03.
Jun 8 2022
using _FillType = _If<sizeof(char_type) >= sizeof(typename traits_type::int_type) && !_OptOutForABICompat, _OptionalFill<_Traits>, _SentinelValueFill<_Traits> >;
May 9 2022
May 5 2022
May 4 2022
Addressed early comments. Some offline discussion occurred to decide to only enable the new member on zOS in 64-bit. I've XFAILed other affected targets. They can opt into the new member in the __config at their discretion as the change affects compatibility, though I've mitigated the compatibility break as much as possible.
- Only enabled on zOS in 64-bit.
- Added XFAILs for other targets from the CI that also fail the new test.
- Only check the new __fill_set_ member when necessary to mitigate compat problems if a target already shipped the version without the member.
May 3 2022
May 2 2022
Apr 28 2022
Apr 27 2022
Apr 19 2022
Revised after D123519.
Apr 6 2022
The change looks good to me but I think others should comment on whether or not the option name change is going to cause anyone problems.
Mar 29 2022
I've had a thorough review and access to the proprietary XL "state table EH" code and I believe this patch is ready to land, nonetheless, I do not have the authority to grant permission to commit this patch. This patch should not affect targets other than AIX and Xing has already run thorough proprietary test coverage against this implementation. Essentially this is a tidied-up version of the EH runtime support we already provide with the libc++ we distribute for AIX.
I updated the patch to use the previous union pattern to bypass the singleton destructors but added constinit, constexpr and attribute((no_destroy)) as possible to avoid the runtime overhead of using a static local.
I was writing a revision of this patch but hit a problem:
Mar 28 2022
The AIX default of ignoring source-level visibility is problematic. I agree the test should be xfailed for AIX.
Mar 25 2022
Mar 18 2022
Makes sense to me.
Mar 1 2022
I'd like to take over this patch and rebase a revision if there are no objections.
Feb 28 2022
This patch has sat idle for 2+ years but I think the current patch is an improvement. Currently, the error_category singletons are implemented as static locals for which there is no guarantee that the destructors won't run prematurely. The proposed patch fixes that problem.
Feb 22 2022
Looks good. I didn't find any semantic issues but I did have a number of suggestions on further refinements for clarity of the code. A vast improvement over the proprietary XL code btw! Thanks Xing.
Jan 12 2022
More comments.
My remaining suggestions are very minor. I've had a thorough look through this patch though I'll look to others to approve the patch to be committed.
Jan 10 2022
Review in still on-going.
Early comments. Continuing to work through the review.
Jan 7 2022
Jan 6 2022
Jan 5 2022
I haven't completed my review. In particular I need to look at the longtbtable, has_vec mix up with more scrutiny but I wanted to submit my comments thus far.
Sep 23 2021
As we lift the backend fatal error we expose the risk of generating silently incompatible code with the XLC compiler on AIX. I think a clang warning diagnostic is warranted for 16 byte aligned byval args where the struct is 16 byte aligned and does not contain a vector member. I don't think this case is common so the diagnostic shouldn't be too verbose.
Sep 1 2021
LGTM.
The XL compiler only defined this weird form of the macros on AIX, but not on the XL Linux on Power compiler. I think it's preferable that we define these macros only on AIX and in the source/commit messages indicate that we're only doing so for AIX XL C/C++ compatibility. Users should prefer the PPC and powerpc macros.
Aug 31 2021
I'd like to see the rationale for adding these forms of the macros in the review and also in the extended commit message. These forms are being added primarily because the AIX XL compiler documented and defined them. The patch itself looks fine.
Aug 10 2021
Given that we have a legacy XL macro with no legacy cross compiler I think it's fine if we set this according to the target only. It's fully redundant to _AIX but we'll define it for any working use-case with the current xlC compiler. If IBM has a compelling use case for a host-specific macro we can always revive this discussion. For now, you're probably making us from making a mistake ;)
Aug 9 2021
The new version is better than the previous. LGTM.
Aug 6 2021
LGTM
LGTM.
Aug 5 2021
It would be nice if the diagnostic could be deferred to layout, in case the struct is defined but not used in a header, but I understand that #pragma pack(1) and #pragma align(packed) become ambiguous at the point of layout. I think this is a reasonable diagnostic given the impact of silently incompatible codegen.
Aug 4 2021
LGTM.
LGTM. THW_PPC is a macro historically defined by xlc on AIX and defining it may help users port to clang.
The divergence with GCC is regrettable. Unfortunately defining _ARCH_PPC64 in 32-bit has been a long-standing discrepancy between xlc and gcc and one I don't expect gcc to change. This patch is preferable in order to retain preprocessing behaviour on AIX for customers porting 32-bit AIX xlc applications to clang.
Jul 21 2021
The decision was made to make -mlong-double-128 an error on AIX: https://reviews.llvm.org/D106074 [AIX] Clang's library integration support for 128-bit long double is incomplete on AIX.
Jul 20 2021
LGTM.