This is an archive of the discontinued LLVM Phabricator instance.

[libc++][AIX] Use C++ overloads from libc++'s math.h
ClosedPublic

Authored by daltenty on May 10 2021, 7:55 AM.

Details

Summary

AIX's system header provides these C++ overloads for compatibility with older XL C++ implementations, but they can be disabled by defining __LIBC_NO_CPP_MATH_OVERLOADS__ since AIX 7.2 TL 5 SP 3.

Since D109078 landed clang will define this macro when using libc++ on AIX and we already run the lit tests with it too. This change will enable the overloads in libc++'s math.h and we'll continue to require the compiler to define the macro going forward.

Diff Detail

Event Timeline

jasonliu created this revision.May 10 2021, 7:55 AM
jasonliu requested review of this revision.May 10 2021, 7:55 AM
daltenty accepted this revision.May 11 2021, 9:11 AM

LGTM from the perspective of interaction with the system math.h

This revision is now accepted and ready to land.May 11 2021, 9:11 AM
jasonliu added 1 blocking reviewer(s): Restricted Project.Jun 30 2021, 12:46 PM
This revision now requires review to proceed.Jun 30 2021, 12:46 PM

Ping.
@ldionne any comments?

ldionne requested changes to this revision.Jul 1 2021, 12:01 PM
ldionne added inline comments.
libcxx/include/math.h
300–310

We shouldn't be adding magic incantations to make a C Standard Library work the way it should. We should fix it so that it works properly instead.

The correct behavior for the C library would be to *not* provide those overloads unless explicitly requested, not the other way around. If that can't be fixed, then I suggest the compiler be fixed so that it enforces -D__COMPATMATH__ on AIX.

This revision now requires changes to proceed.Jul 1 2021, 12:01 PM
libcxx/include/math.h
300–310

The solution for the compiler to define __COMPATMATH__ would be improper. This is a documented macro associated with the IBM XL compiler. When the end-user observes that the macro is set, they have some reason to expect that the overloads are not provided.

Since some other C++ libraries on AIX expect the overloads to come from the OS headers, it looks like the direction to explore is for the OS headers to implement a path for newer versions of libc++.

daltenty commandeered this revision.Aug 31 2021, 11:12 AM
daltenty edited reviewers, added: jasonliu; removed: daltenty.
daltenty planned changes to this revision.Sep 1 2021, 7:08 AM
daltenty edited the summary of this revision. (Show Details)Feb 7 2022, 9:09 PM
daltenty updated this revision to Diff 406699.Feb 7 2022, 9:12 PM

Update to a new version that requires the compiler to define the __LIBC_NO_CPP_MATH_OVERLOADS__ macro (which clang does).

daltenty edited reviewers, added: jsji; removed: jasonliu.Feb 7 2022, 9:13 PM
jsji added a comment.Feb 8 2022, 7:50 AM

@daltenty Can you please update the patch to include more *Context*, looks like the Context not available. Or use arc to update it. Thanks.

@daltenty Can you please update the patch to include more *Context*, looks like the Context not available. Or use arc to update it. Thanks.

In case it's needed: The command I use to get context in my diffs (without using arc) is git show -U999 HEAD >filetoupload.diff (or replace HEAD with whatever-commit-hash).

@daltenty Can you please update the patch to include more *Context*, looks like the Context not available. Or use arc to update it. Thanks.

In case it's needed: The command I use to get context in my diffs (without using arc) is git show -U999 HEAD >filetoupload.diff (or replace HEAD with whatever-commit-hash).

Sorry, arcanist broke for me due to an OS update recently and I haven't quite gotten used to uploading the patches manually yet. Will revise with the additional content.

daltenty updated this revision to Diff 406892.Feb 8 2022, 10:41 AM

Add additional context

jsji accepted this revision.Feb 8 2022, 10:48 AM

Thanks @daltenty ! LGTM.

but they can be disabled by defining LIBC_NO_CPP_MATH_OVERLOADS

It would be great if we can mention the TL level that start to define this in the description.

@ldionne Can you have another look too? Thanks!

daltenty edited the summary of this revision. (Show Details)Feb 8 2022, 3:03 PM

@ldionne gentle ping. I think we've addressed your previous concerns here. We've moved responsibility for setting the macro to suppress the AIX libc definitions to the compiler side as you suggested in D109078, so this patch should hopefully be non-controversial now.

Gentle ping

EricWF accepted this revision.Mar 1 2022, 9:34 AM
ldionne accepted this revision.Mar 1 2022, 10:30 AM
This revision is now accepted and ready to land.Mar 1 2022, 10:30 AM
daltenty updated this revision to Diff 412184.Mar 1 2022, 11:29 AM

Rebase to retrigger CI

Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 12:27 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 12:54 PM