fma and fmaf have been enabled on the bots so this is ready to go.

Does this need clang-11?

In D101895#2748640, @sivachandra wrote:

The builders use clang-7 but I am working on upgrading them to clang-11. If you think clang-12 would be ideal, I can try getting it up to clang-12. I will try now and report back here.

In D101895#2748649, @gchatelet wrote:

Where can I see the real error? I can't find it on the page you linked in. All I can find is the python executor error which is not helpful :-/
Also, I'm puzzled as to why the failures triggers right now, the patch has been sent more than 9h ago.

In D101895#2748627, @gchatelet wrote:

Do you happen to know which clang version is being used by the buildbot?

This change also triggers a failure on the aarch64 bots like this: https://lab.llvm.org/buildbot/#/builders/138/builds/4761

Unfortunately, I had to revert this as the bots were failing like this: https://lab.llvm.org/buildbot/#/builders/78/builds/11551.
I looked around for a bit to see if I can fix forward, but I do not think I know enough about the various x86_64 arch names. May be -march=k8 is all we need, but I will let you decide what the best course of action is.

Thanks for explaining.

Overall LGTM. I have few questions I want to get clarity on.

Sorry I missed this.

LGTM with one additional suggestion.

Mixed build - You only pick those parts from LLVM libc which you are interested in and get the rest from the system libc. In this mode, we don't use headers from LLVM libc.
Full build - This mode behaves as if LLVM libc is the only libc. So, we want to ensure that the libc build does not get mixed up with headers from the system libc. The lint step helps us with this.

I will send a patch to fix the CMake logic.

Ah, you are probably doing a mixed build with linting enabled. I think that is a bug in the CMake logic. It should not allow you to switch on linting in mixed build mode (mixed build mode is the default mode). Linting should be on only in full build mode.
FWIW, we have full build builders on the public buildbot which build with linting enabled. Example: https://lab.llvm.org/buildbot/#/builders/163

Can you share what linting errors you are getting? The include order is setup so that when doing a full build, the build picks up LLVM libc's header files and does not lead to lint errors. If you are seeing lint errors, it could imply that DEPS in the CMake targets are incomplete. The includes for <math.h> and <fenv.h> are as they are to keep the mixed builds working.

Sorry for the long delay. I have a nitty comment inline. Also, can you please change the way the subject line is written: Instead of "Introduces asctime, asctime_r to LLVM libc, based on C99/C2X/Single Unix Sp.", can you change it to, "Introduce asctime, asctime_r to LLVM libc". Change the rest of the description in a similar way.

• Fix a typo.
• Add similar targets for cosf as well.

LGTM but I have added a comment to add a sollya script. Good to go after that.

Overall, a very nice change. Thanks for doing it. The clang-tidy warnings in the equals method are legitimate and should be fixed before submitting.

In D100811#2700415, @tschuett wrote:

Stupid question: why assembler instead of builtins:
https://github.com/gcc-mirror/gcc/blob/master/gcc/testsuite/gcc.target/ia64/builtin-fma-1.c
I guess there is a tradeoff between control over the process and helping the register allocator.

Add fma to the x86_64 entrypoint list.

I have not yet fully tested this patch on x86_64. Will update soon if it requires any changes.

Note that Fuchsia does not block us from using this feature in LLVM libc source tree as only ctype tests from LLVM libc run in Fuchsia.

In D100571#2694526, @gchatelet wrote:

Thx for the suggestion, I didn't go with explicit member function as it would allow for integer promotion.
In the case of endianness we want to make sure we only accept one of uint8_t, uint16_t, uint32_t, uint64_t and no other type.
I've added the symmetric functions as you suggested.

Accepting this as it looks like zxtest can be extended to have this feature. Irrespective though, as I think more about it, it is definitely of value when listing the differently-typed tests that can all be listed in a single file/single test.

In D100631#2695208, @gchatelet wrote:

In D100631#2695126, @sivachandra wrote:

While I am not opposed to this, I want to learn the usefulness of this feature. IIUC, this limits to a single ParamType - as in a uni-dimensional type variance. We have examples in math functions where there is two-dimensional type variance. I am not sure the feature added in this patch can help with it. For such cases, have used template test classes and helper macros to list the various combination of tests: https://github.com/llvm/llvm-project/blob/main/libc/test/src/math/RoundToIntegerTest.h.

Yes I've spotted these tests but I found that it scaled poorly as you define more tests since you have to define the tests into a macro which makes for poor editing.
You ended up moving the body of the test to an external function to help with this but I think it's not ideal (especially for failure location).

While I am not opposed to this, I want to learn the usefulness of this feature. IIUC, this limits to a single ParamType - as in a uni-dimensional type variance. We have examples in math functions where there is two-dimensional type variance. I am not sure the feature added in this patch can help with it. For such cases, have used template test classes and helper macros to list the various combination of tests: https://github.com/llvm/llvm-project/blob/main/libc/test/src/math/RoundToIntegerTest.h.

In D100657#2695069, @sivachandra wrote:

FYI this has caused widespread redness in the bots. The fixes seem to be straightforward so I do not think we need to revert.

FYI this has caused widespread redness in the bots. The fixes seem to be straightforward so I do not think we need to revert.

In D100569#2691816, @gchatelet wrote:

Should we start to add tests?

In D100571#2691821, @gchatelet wrote:

Let me know if you want this header to live somewhere else, or if you had other plans in mind.

Instead of defaulting to empty string here, do you want it to default to
${CMAKE_INSTALL_PREFIX}:
https://github.com/llvm/llvm-project/blob/main/libc/CMakeLists.txt#L17
That seems reasonable to me so feel free to send a patch with that fix.
Else, feel free to suggest other options.

In D99046#2642356, @MaskRay wrote:

https://llvm.org/docs/DeveloperPolicy.html#commit-messages The subject line is usually an imperative sentence. Words like "This introduces" can be deleted.

Couple of questions inline. Feel free to submit after answering/addressing them.

In D97736#2605535, @phosek wrote:

Have you considered using an input linker script? We could generate libc.so that could look something like:

INPUT(libllvmlibc.a /lib/libc.so)

We would need to pass --sysroot to the linker for this to work. The driver could remain completely agnostic of whether you're using LLVM libc or not.

Remove empty '//' in lit test file.

In D97736#2605432, @echristo wrote:

In addition to the bikeshed below, I'm not a huge fan of this in general. I think we should assume that the libc we link is complete and thus it would just be named "libc.<ext>" and in a sysroot somewhere. Another option is perhaps looking at the rtlib option, but I'd want to see a bit of a writeup there so we can see what we'd be doing.

In D97736#2605432, @echristo wrote:

In addition to the bikeshed below, I'm not a huge fan of this in general. I think we should assume that the libc we link is complete and thus it would just be named "libc.<ext>" and in a sysroot somewhere. Another option is perhaps looking at the rtlib option, but I'd want to see a bit of a writeup there so we can see what we'd be doing.