This is an archive of the discontinued LLVM Phabricator instance.

[libc++][CI] Updates GCC to version 12.
ClosedPublic

Authored by Mordante on May 30 2022, 9:40 AM.

Details

Reviewers
ldionne
philnik
Group Reviewers
Restricted Project
Restricted Project
Commits
rGeb12ad9d7ff6: [libc++][CI] Updates GCC to version 12.

Diff Detail

Event Timeline

Mordante created this revision.May 30 2022, 9:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2022, 9:40 AM
Mordante updated this revision to Diff 435660.Jun 9 2022, 1:07 PM

Rebased on the new Jammy based CI.

Mordante updated this revision to Diff 436135.Jun 11 2022, 6:27 AM

Rebased and fixed some issues.

Mordante updated this revision to Diff 436154.Jun 11 2022, 10:50 AM

Test whether -Wno-tautological-compare is still needed.

Mordante added inline comments.
libcxx/utils/libcxx/test/params.py
33 ↗(On Diff #436154)

This disabled leads to a lot of errors in C++11 with GCC especially in the string tests.

The issue is the code is tested constexpr starting with C++20. Therefore the compiler can deduce that in C++11 mode the function never is constexpr and thus the test always false.

In file included from /home/libcxx-builder/.buildkite-agent/builds/28dd04a444a9-1/llvm-project/libcxx-ci/libcxx/test/std/strings/basic.string/string.modifiers/string_copy/copy.pass.cpp:20:

/home/libcxx-builder/.buildkite-agent/builds/28dd04a444a9-1/llvm-project/libcxx-ci/libcxx/test/std/strings/basic.string/string.modifiers/string_copy/copy.pass.cpp: In function 'void test(S, typename S::value_type*, typename S::size_type, typename S::size_type)':

/home/libcxx-builder/.buildkite-agent/builds/28dd04a444a9-1/llvm-project/libcxx-ci/libcxx/test/support/test_macros.h:146:68: error: 'std::is_constant_evaluated' always evaluates to false in a non-'constexpr' function [-Werror=tautological-compare]

  146 | # define TEST_IS_CONSTANT_EVALUATED __builtin_is_constant_evaluated()

      |                                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~

/home/libcxx-builder/.buildkite-agent/builds/28dd04a444a9-1/llvm-project/libcxx-ci/libcxx/test/std/strings/basic.string/string.modifiers/string_copy/copy.pass.cpp:38:15: note: in expansion of macro 'TEST_IS_CONSTANT_EVALUATED'

   38 |     else if (!TEST_IS_CONSTANT_EVALUATED)

      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~

I see several possible solutions

  • Disable the compile flag in C++11 (This works since we only test GCC in C++11 and C++latest.)
  • Disable the compile flag unconditionally
  • add macros like TEST_IS_CONSTANT_EVALUATED_AFTER_11

I think solution 1 would be the best, @ldionne, @philnik WDYT?

philnik added inline comments.Jun 11 2022, 12:40 PM
libcxx/utils/libcxx/test/params.py
33 ↗(On Diff #436154)

Do you know if the TEST_IS_CONSTANT_EVALUATED cases are the only ones? I think we should refactor them anyways. In hindsight this should have never been a macro. I would change it to something like

namespace test {
  enum Version {
    Cpp20,
    Cpp23,
  };

  constexpr bool is_constant_evaluated_impl() {
#if TEST_STD_VER >= 20 && defined(__cpp_lib_is_constant_evaluated) && __cpp_lib_is_constant_evaluated >= 201811L
        return std::is_constant_evaluated();
#else
        return false;
#endif
  }

  constexpr bool is_constant_evaluated(Version ver) {
    switch (ver) {
      case Cpp20:
        return is_constant_evaluated_impl() && TEST_STD_VER >= 20;
      case Cpp23:
        return is_constant_evaluated_impl() && TEST_STD_VER >= 23;

    }
    assert(false);
  }
}

This allows us to replace the current uses of TEST_IS_CONSTANT_EVALUATED with test::is_constant_evaluated(test::Cpp20) and for constexpr std::unique_ptr use test::is_constant_evaluated(test::Cpp23). I also think that we should just remove the __builtin_is_constant_evaluated() part. It's unused anyways currently. If we need it at any point we can still add it later. According to https://godbolt.org/z/cqdv88xez GCC is also happy with this reworked version.

Mordante added inline comments.Jun 11 2022, 12:54 PM
libcxx/utils/libcxx/test/params.py
33 ↗(On Diff #436154)

I'm not sure whether these are the only ones. If we go with your approach or TEST_IS_CONSTANT_EVALUATED_AFTER_11 it should be a separate patch. Then this patch needs to be on top of that one. Then it's easier to investigate whether more work is needed.

Interesting approach. I'll give this one some more thought.

ldionne added inline comments.Jun 13 2022, 7:20 AM
libcxx/utils/libcxx/test/params.py
33 ↗(On Diff #436154)

I like @philnik 's approach, but I am not sure it will resolve the issues. Technically, GCC could still figure out that we're returning false if it evaluates the is_constant_evaluated(Cpp20) expression deep enough. But yeah, I'd definitely welcome a patch that tries to do this -- personally I've never been a huge fan of the TEST_IS_CONSTANT_EVALUATED macro anyways.

Mordante updated this revision to Diff 437952.Jun 17 2022, 9:57 AM

Rebased on D127978 to see whether that commit fixes all new GCC-12 diagnostics.

Mordante published this revision for review.Jun 17 2022, 12:00 PM
Mordante added reviewers: ldionne, philnik.
Mordante added inline comments.
libcxxabi/test/catch_member_function_pointer_02.pass.cpp
18

I'll remove gcc-11 before landing or with the next iteration of this patch.

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 17 2022, 12:00 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik accepted this revision as: philnik.Jun 17 2022, 12:09 PM

LGTM.

libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.char_array.pass.cpp
10

Couldn't you just use std::copy_n for this?

Thanks for the review!

libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.char_array.pass.cpp
10

I probably can; however when I wrote the test (in the GCC-10 era) there were a lot more issues.
(Actually that's not true, it was for the formatter.const_char_array.pass.cpp which this is a copy-paste of.)

But I want to look whether I can refactor this test, I learned a bit more about "string literals" as template argument.
However I prefer to do that in a separate commit.

ldionne accepted this revision.Jun 20 2022, 12:37 PM

Thanks for keeping the CI up-to-date! This LGTM, however I would support landing this with -Wno-tautological-compare in the test suite to get this unblocked until (or if) we find a definitive solution for the issue (since I'm not sure that D127978 is what we want).

This revision is now accepted and ready to land.Jun 20 2022, 12:37 PM
Mordante updated this revision to Diff 438725.Jun 21 2022, 8:49 AM

Removed the parent commit and test again with -Wno-tautological-compare.

This revision was automatically updated to reflect the committed changes.