This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Make some tests in `math_nodiscard_extensions` Clang-only.
ClosedPublic

Authored by var-const on Jan 31 2023, 4:48 PM.

Details

Summary

Clang implicitly adds the [[gnu::const]] attribute to many math
functions from the C standard library functions. Since this behavior is
Clang-specific, make sure the test only runs on Clang.

Diff Detail

Event Timeline

var-const created this revision.Jan 31 2023, 4:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2023, 4:48 PM
var-const requested review of this revision.Jan 31 2023, 4:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2023, 4:48 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik requested changes to this revision.Jan 31 2023, 4:52 PM
philnik added a subscriber: philnik.

These are marked [[gnu::const]] through clang though. That's why these are regex matchers, not simple text versions.

This revision now requires changes to proceed.Jan 31 2023, 4:52 PM

These are marked [[gnu::const]] through clang though. That's why these are regex matchers, not simple text versions.

Can you elaborate on how exactly this works? (or perhaps it's documented somewhere)

These are marked [[gnu::const]] through clang though. That's why these are regex matchers, not simple text versions.

Can you elaborate on how exactly this works? (or perhaps it's documented somewhere)

It's not exactly documentation, but you can look at clang/include/clang/Basic/Builtins.def to see what attributes are applied to builtins and what builtins there are.

It's not exactly documentation, but you can look at clang/include/clang/Basic/Builtins.def to see what attributes are applied to builtins and what builtins there are.

Thanks. If I'm reading this correctly, though, this applies to __builtin_foo functions only, right? If that's the case, it's not what I'm concerned about. A platform could define e.g. double fabs(double) that would override our implementation (by design). Clang will create __builtin_fabs and mark it as const, but it won't change the platform's definition of fabs (I presume -- but if it did, it seems there would be no need for us to add the attribute in the first place). But since the platform's fabs overrides our fabs, it doesn't necessarily delegate to __builtin_fabs, and in any case, we don't control its definition.

Taking a step back, I also don't think we should be testing compiler behavior or relying on what is essentially an implementation detail of Clang and presumably GCC.

It's not exactly documentation, but you can look at clang/include/clang/Basic/Builtins.def to see what attributes are applied to builtins and what builtins there are.

Thanks. If I'm reading this correctly, though, this applies to __builtin_foo functions only, right?

No, this also applies to the non-__builtin_ versions. The math functions are declared at about line 1200 (currently).

Taking a step back, I also don't think we should be testing compiler behavior or relying on what is essentially an implementation detail of Clang and presumably GCC.

The [[nodiscard]] declarations are specifically there to match the diagnostics produced by the compiler, so I think we should check that the compiler actually produces the same diagnostic for all overloads. (https://godbolt.org/z/87hMPjeq7)

No, this also applies to the non-__builtin_ versions. The math functions are declared at about line 1200 (currently).

Thanks. It means it applies to our definition of double fabs(double), right? (If that's the case, it doesn't need the nodiscard attribute on Clang, though I guess it's there for the sake of GCC)

The [[nodiscard]] declarations are specifically there to match the diagnostics produced by the compiler, so I think we should check that the compiler actually produces the same diagnostic for all overloads. (https://godbolt.org/z/87hMPjeq7)

In the Godbolt link, the non-double overloads of trunc test our implementation, while trunc(0.) essentially tests that Clang adds the const attribute. I don't think we should be testing Clang behavior.

Moreover, this seems Clang-centric. Does GCC have the same behavior? If not, a platform that compiles with GCC and "overrides" one of the math functions will fail the expectation (though to be fair, we could mark the test as unsupported on GCC).

To be clear, I think the idea of adding the attributes is sound. However, testing it is hard, for reasons outside of our control.

No, this also applies to the non-__builtin_ versions. The math functions are declared at about line 1200 (currently).

Thanks. It means it applies to our definition of double fabs(double), right? (If that's the case, it doesn't need the nodiscard attribute on Clang, though I guess it's there for the sake of GCC)

No. Clang only adds them to overloads with C linkage (https://godbolt.org/z/rjY3xeo3o). GCC also warns with -Wall. I don't know where to see that though. So I guess hypothetically there could be a C library that doesn't have these functions declared extern "C" but I doubt these actually exist.

The [[nodiscard]] declarations are specifically there to match the diagnostics produced by the compiler, so I think we should check that the compiler actually produces the same diagnostic for all overloads. (https://godbolt.org/z/87hMPjeq7)

In the Godbolt link, the non-double overloads of trunc test our implementation, while trunc(0.) essentially tests that Clang adds the const attribute. I don't think we should be testing Clang behavior.

I guess we just disagree here. IMO we should test the Clang implementation if we mirror it, especially if it's pretty much trivial to check.

Moreover, this seems Clang-centric. Does GCC have the same behavior? If not, a platform that compiles with GCC and "overrides" one of the math functions will fail the expectation (though to be fair, we could mark the test as unsupported on GCC).

We can mark it as unsuported on GCC, although it seems kind-of pointless, since GCC will most likely never implement -verify and definitely won't mirror clang warning wording.

To be clear, I think the idea of adding the attributes is sound. However, testing it is hard, for reasons outside of our control.

ldionne accepted this revision.Feb 1 2023, 2:47 PM
ldionne added a subscriber: ldionne.

I like this change -- depending on the fact that this is a .verify.cpp test to disable this on non-Clang compilers is too subtle IMO.

libcxx/test/libcxx/diagnostics/math_nodiscard_extensions.verify.cpp
19

We should even be able to drop this regex now since this is consistently nodiscard.

155

I think this is what I would do:

// These tests rely on Clang's behaviour of adding [[gnu::const]] to the double overload of most of the functions below.
// Without that attribute being added implicitly, this test can't be checked consistently because its result depends on whether we're
// getting libc++'s own std::foo(double) or the underlying C library's foo(double).
#ifdef TEST_COMPILER_CLANG
std::round(0.); // expected-warning-re {{....}}
std::trunc(0.); // expected-warning-re {{....}}
etc...
#endif

Further investigation: this test will fail on any platform that compiles with -ffreestanding (which turns off the Clang behaviour to add [[gnu::const]]) where the C Standard library provided std::round doesn't provide the attribute itself. I will have a look at setting that up in the CI.

var-const updated this revision to Diff 494359.Feb 2 2023, 10:27 AM

Address feedback

var-const retitled this revision from [libc++] Disable some tests in `math_nodiscard_extensions`. to [libc++] Make some tests in `math_nodiscard_extensions` Clang-only..Feb 2 2023, 10:28 AM
var-const edited the summary of this revision. (Show Details)
var-const updated this revision to Diff 494384.Feb 2 2023, 11:30 AM

Address feedback

var-const marked 2 inline comments as done.Feb 2 2023, 11:30 AM
This revision was not accepted when it landed; it landed in state Needs Review.Feb 2 2023, 11:30 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

Why didn't you just mark the whole test as // REQUIRES: clang? It's not like we gain anything from "enabling" the test for other compilers. It just makes it a lot harder to verify that all overloads are checked.