Page MenuHomePhabricator

Enable -Wctad-maybe-unsupported in LLVM build
ClosedPublic

Authored by dblaikie on Aug 11 2022, 2:57 PM.

Details

Reviewers
Mordante
kparzysz
Group Reviewers
Restricted Project
Commits
rGc63f2581f4f8: Enable -Wctad-maybe-unsupported in LLVM build
Summary

(this still has issues with the libcxx build where it warns, but doesn't
error - probably on some cases the warning shouldn't fire (it's not
meant to fire on standard library names - but some are just
implementation details of libcxx so not official standard library
names))

Diff Detail

Event Timeline

dblaikie created this revision.Aug 11 2022, 2:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2022, 2:57 PM
Herald added a subscriber: mgorny. · View Herald Transcript
dblaikie requested review of this revision.Aug 11 2022, 2:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2022, 2:57 PM

@ldionne would you happen to know how I might address the warnings (apparently not errors - not sure why LLVM_ENABLE_WERROR isn't having an effect on the libcxx build) this produces in libcxx? I didn't see anywhere obvious I could turn the warning off for the libcxx build separate from the LLVM build, I guess that might be somewhere in the ENABLE_RUNTIMES handling?

Adding the warning LGTM. The summary should state what -Wctad-maybe-unsupported is and how it is useful.

I see that https://discourse.llvm.org/t/rfc-increasing-the-gcc-and-clang-requirements-to-support-c-17-in-llvm/59983 didn't discuss CTAD.
If there are mixed feelings, I think we should be conservative by doing this, and let folks propose CTAD to make convincing arguments not to use -Wctad-maybe-unsupported

llvm/cmake/modules/HandleLLVMOptions.cmake
798

typo in comment

dblaikie updated this revision to Diff 452000.Aug 11 2022, 3:18 PM

Update cmake comment and patch description

dblaikie marked an inline comment as done.Aug 11 2022, 3:19 PM

Adding the warning LGTM. The summary should state what -Wctad-maybe-unsupported is and how it is useful.

Updated wording.

Mostly wasn't settling on the commit message until the substantive parts of the patch were worked out, like whether people agreed and what to do with libcxx.

llvm/cmake/modules/HandleLLVMOptions.cmake
798

ah, thanks!

Adding the warning LGTM. The summary should state what -Wctad-maybe-unsupported is and how it is useful.

I see that https://discourse.llvm.org/t/rfc-increasing-the-gcc-and-clang-requirements-to-support-c-17-in-llvm/59983 didn't discuss CTAD.

I explicitly mentioned it in that thread, and there was no opposition to it. Nor have I seen any concerns about it in any other relevant thread.

If there are mixed feelings, I think we should be conservative by doing this, and let folks propose CTAD to make convincing arguments not to use -Wctad-maybe-unsupported

I don't think there is any harm in requiring an explicit guide, and it shouldn't hinder the use of CTAD. Do you know of any cases where such a guide cannot be added?

dblaikie marked an inline comment as done.Aug 11 2022, 3:36 PM

If there are mixed feelings, I think we should be conservative by doing this, and let folks propose CTAD to make convincing arguments not to use -Wctad-maybe-unsupported

I don't think there is any harm in requiring an explicit guide, and it shouldn't hinder the use of CTAD. Do you know of any cases where such a guide cannot be added?

None that I know of.

Though the libcxx situation might be weird in some way - it looked like it was warning on standard library types (which the warning is meant to/does usually assume are valid CTAD classes even if they lack explicit guides) & adding all of those might be more than is helpful, so figuring out what's going on there and/or disabling the warning in the libcxx build might be suitable (though I don't know how to do that).

ChuanqiXu added inline comments.
llvm/cmake/modules/HandleLLVMOptions.cmake
798
dblaikie marked an inline comment as done.Aug 12 2022, 3:23 PM

Still open to ideas as to what to do with libc++. I mean it seems it's only a warning and not an error there, even in an LLVM_ENABLE_WERROR build, so maybe it can be committed as-is? But I'm not sure/wouldn't mind a libc++ developer to weigh in. (@ldionne?)

llvm/cmake/modules/HandleLLVMOptions.cmake
798

Thanks for the catch!

jloser added a subscriber: jloser.Aug 12 2022, 3:46 PM

Still open to ideas as to what to do with libc++. I mean it seems it's only a warning and not an error there, even in an LLVM_ENABLE_WERROR build, so maybe it can be committed as-is? But I'm not sure/wouldn't mind a libc++ developer to weigh in. (@ldionne?)

Do you have some examples of libc++ code that this new warning flags? I'm happy to take a look.

dblaikie marked an inline comment as done.EditedAug 12 2022, 4:50 PM

Still open to ideas as to what to do with libc++. I mean it seems it's only a warning and not an error there, even in an LLVM_ENABLE_WERROR build, so maybe it can be committed as-is? But I'm not sure/wouldn't mind a libc++ developer to weigh in. (@ldionne?)

Do you have some examples of libc++ code that this new warning flags? I'm happy to take a look.

My concern is it looks more like some cases of warning false positive, possibly because of the way libc++ builds itself (does it #define _VSTD to be empty or something other than std for its own test builds, perhaps, so as not to interfere with the system compiler's standard library entities?) - which might be thwarting the warning's general suppression for standard library things.

Also some implementation details too, like filesystem/operations.cpp:scope_exit

Anyway, here's the first set of warnings I get when I run ninja cxx:

In file included from /usr/local/google/home/blaikie/dev/llvm/src/libcxx/src/charconv.cpp:12:
/usr/local/google/home/blaikie/dev/llvm/src/libcxx/src/include/to_chars_floating_point.h:901:82: warning: 'less' may not intend to support class template argument deduction [-Wctad-maybe-unsupported]
                return _VSTD::lower_bound(_Table_begin, _Table_end, _Uint_value, less{});
                                                                                 ^
/usr/local/google/home/blaikie/dev/llvm/build/default/include/c++/v1/__functional/operations.h:366:29: note: add a deduction guide to suppress this warning
struct _LIBCPP_TEMPLATE_VIS less
                            ^
In file included from /usr/local/google/home/blaikie/dev/llvm/src/libcxx/src/charconv.cpp:12:
/usr/local/google/home/blaikie/dev/llvm/src/libcxx/src/include/to_chars_floating_point.h:901:82: warning: 'less' may not intend to support class template argument deduction [-Wctad-maybe-unsupported]
                return _VSTD::lower_bound(_Table_begin, _Table_end, _Uint_value, less{});
                                                                                 ^
/usr/local/google/home/blaikie/dev/llvm/src/libcxx/src/include/to_chars_floating_point.h:1067:20: note: in instantiation of function template specialization 'std::_Floating_to_chars_general_precision<double>' requested here
            return _Floating_to_chars_general_precision(_First, _Last, _Value, _Precision);
                   ^
/usr/local/google/home/blaikie/dev/llvm/src/libcxx/src/charconv.cpp:77:10: note: in instantiation of function template specialization 'std::_Floating_to_chars<std::_Floating_to_chars_overload::_Format_precision, double>' requested here
  return _Floating_to_chars<_Floating_to_chars_overload::_Format_precision>(__first, __last, __value, __fmt,
         ^
/usr/local/google/home/blaikie/dev/llvm/build/default/include/c++/v1/__functional/operations.h:366:29: note: add a deduction guide to suppress this warning
struct _LIBCPP_TEMPLATE_VIS less
                            ^
2 warnings generated.
[479/563] Building CXX object libcxx/src/CMakeFiles/cxx_static.dir/charconv.cpp.o
In file included from /usr/local/google/home/blaikie/dev/llvm/src/libcxx/src/charconv.cpp:12:
/usr/local/google/home/blaikie/dev/llvm/src/libcxx/src/include/to_chars_floating_point.h:901:82: warning: 'less' may not intend to support class template argument deduction [-Wctad-maybe-unsupported]
                return _VSTD::lower_bound(_Table_begin, _Table_end, _Uint_value, less{});
                                                                                 ^
/usr/local/google/home/blaikie/dev/llvm/build/default/include/c++/v1/__functional/operations.h:366:29: note: add a deduction guide to suppress this warning
struct _LIBCPP_TEMPLATE_VIS less
                            ^
In file included from /usr/local/google/home/blaikie/dev/llvm/src/libcxx/src/charconv.cpp:12:
/usr/local/google/home/blaikie/dev/llvm/src/libcxx/src/include/to_chars_floating_point.h:901:82: warning: 'less' may not intend to support class template argument deduction [-Wctad-maybe-unsupported]
                return _VSTD::lower_bound(_Table_begin, _Table_end, _Uint_value, less{});
                                                                                 ^
/usr/local/google/home/blaikie/dev/llvm/src/libcxx/src/include/to_chars_floating_point.h:1067:20: note: in instantiation of function template specialization 'std::_Floating_to_chars_general_precision<double>' requested here
            return _Floating_to_chars_general_precision(_First, _Last, _Value, _Precision);
                   ^
/usr/local/google/home/blaikie/dev/llvm/src/libcxx/src/charconv.cpp:77:10: note: in instantiation of function template specialization 'std::_Floating_to_chars<std::_Floating_to_chars_overload::_Format_precision, double>' requested here
  return _Floating_to_chars<_Floating_to_chars_overload::_Format_precision>(__first, __last, __value, __fmt,
         ^
/usr/local/google/home/blaikie/dev/llvm/build/default/include/c++/v1/__functional/operations.h:366:29: note: add a deduction guide to suppress this warning
struct _LIBCPP_TEMPLATE_VIS less
                            ^
2 warnings generated.
[556/563] Building CXX object libcxx/src/CMakeFiles/cxx_static.dir/filesystem/operations.cpp.o
/usr/local/google/home/blaikie/dev/llvm/src/libcxx/src/filesystem/operations.cpp:1427:5: warning: 'scope_exit' may not intend to support class template argument deduction [-Wctad-maybe-unsupported]
    scope_exit close_stream([=] { ::closedir(stream); });
    ^
/usr/local/google/home/blaikie/dev/llvm/src/libcxx/src/filesystem/operations.cpp:1401:8: note: add a deduction guide to suppress this warning
struct scope_exit {
       ^
1 warning generated.
[558/563] Building CXX object libcxx/src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.o
/usr/local/google/home/blaikie/dev/llvm/src/libcxx/src/filesystem/operations.cpp:1427:5: warning: 'scope_exit' may not intend to support class template argument deduction [-Wctad-maybe-unsupported]
    scope_exit close_stream([=] { ::closedir(stream); });
    ^
/usr/local/google/home/blaikie/dev/llvm/src/libcxx/src/filesystem/operations.cpp:1401:8: note: add a deduction guide to suppress this warning
struct scope_exit {
       ^
1 warning generated.
dblaikie updated this revision to Diff 452389.Aug 12 2022, 11:42 PM

With libcxx fixes

Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2022, 11:42 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Updated with the libcxx changes necessary to get a clean build - but I'm still confused by:

  1. why the clang warning fired on these at all (I haven't looked into that in detail, but I can probably figure it out with some closer inspection)
  2. why libcxx doesn't seem to apply LLVM_ENABLE_WERROR & what that means with regards to libcxx's policy on warning cleanliness, etc..

Updated with the libcxx changes necessary to get a clean build - but I'm still confused by:

  1. why the clang warning fired on these at all (I haven't looked into that in detail, but I can probably figure it out with some closer inspection)
  2. why libcxx doesn't seem to apply LLVM_ENABLE_WERROR & what that means with regards to libcxx's policy on warning cleanliness, etc..
  1. The warnings probably fire because we don't consider the headers to be system headers when building the library.
  2. We have LIBCXX_ENABLE_WERROR for that. I don't know why we ignore LLVM_ENABLE_WERROR, but it probably has something to do with GCC not building libc++ cleanly. For example we get warnings that __attribute__((init_priority(100))) is implementation-reserved. When using clang we build with -Werror in the CI.

I think libc++ shouldn't be built with -Wctad-maybe-unsupported, since we can generally assume that using CTAD is OK.
BTW it would help if you ping the #libc group next time you want to ask something. That way all the libc++ developers get an email.

I've only looked at the libc++ changes.

I'm quite wary of these changes for libc++. Adding deduction guides to public types are essentially C++ Standard library extensions. Maybe we should first discuss this further on Discourse.

Still open to ideas as to what to do with libc++. I mean it seems it's only a warning and not an error there, even in an LLVM_ENABLE_WERROR build, so maybe it can be committed as-is? But I'm not sure/wouldn't mind a libc++ developer to weigh in. (@ldionne?)

Do you have some examples of libc++ code that this new warning flags? I'm happy to take a look.

My concern is it looks more like some cases of warning false positive, possibly because of the way libc++ builds itself (does it #define _VSTD to be empty or something other than std for its own test builds, perhaps, so as not to interfere with the system compiler's standard library entities?) - which might be thwarting the warning's general suppression for standard library things.

_VSTD expands to std, in newer code we use std directly. I expect the problem stems from that libc++ isn't included as system include in our tests.

libcxx/include/__iterator/back_insert_iterator.h
63 ↗(On Diff #452389)

This breaks C++03, C++11, and C++14 since it doesn't support CTAD.

dblaikie updated this revision to Diff 452457.Aug 13 2022, 2:10 PM

remove libcxx fixes and disable the warning for libcxx instead
rebase

Updated with the libcxx changes necessary to get a clean build - but I'm still confused by:

  1. why the clang warning fired on these at all (I haven't looked into that in detail, but I can probably figure it out with some closer inspection)
  2. why libcxx doesn't seem to apply LLVM_ENABLE_WERROR & what that means with regards to libcxx's policy on warning cleanliness, etc..
  1. The warnings probably fire because we don't consider the headers to be system headers when building the library.

Oooh yeah, good call - I hadn't thought about that part of it. I think when I saw the description in the Google style guide that said "all types in the std namespace are assumed to have opted in" I took that literally, but you're right that it's probably a system header filter rather than a std namespace filter.

Would libc++ want to compile itself as system headers? Or does it generally want all/most of the extra warnings when compiling its own implementation, I guess?

  1. We have LIBCXX_ENABLE_WERROR for that. I don't know why we ignore LLVM_ENABLE_WERROR, but it probably has something to do with GCC not building libc++ cleanly.

Ah, weird - yeah, would be nice to unify those at some point - other projects in LLVM have the same issues with GCC warnings and we disable them when necessary/appropriate rather than having LLVM_ENABLE_WERROR ignored when the compiler isn't clang or anything like that.

For example we get warnings that __attribute__((init_priority(100))) is implementation-reserved. When using clang we build with -Werror in the CI.

I think libc++ shouldn't be built with -Wctad-maybe-unsupported, since we can generally assume that using CTAD is OK.

Yeah, that was my feeling too but I wasn't sure where/how to disable it in the libc++ build. But I guess maybe I can add it to this list ( https://github.com/llvm/llvm-project/blob/eaf0aa1f1fbd06fbd66417f2c9d50d3074969824/libcxx/CMakeLists.txt#L592 (updated this review to include that change)) and I don't have to worry about compiler compatibility checks because libcxx uses the just-built clang, so it's not going to run against an outdated clang that'll complain that it doesn't know the warning flag?

BTW it would help if you ping the #libc group next time you want to ask something. That way all the libc++ developers get an email.

Oh, right - thanks for the pointer!

Updated with the libcxx changes necessary to get a clean build - but I'm still confused by:

  1. why the clang warning fired on these at all (I haven't looked into that in detail, but I can probably figure it out with some closer inspection)
  2. why libcxx doesn't seem to apply LLVM_ENABLE_WERROR & what that means with regards to libcxx's policy on warning cleanliness, etc..
  1. The warnings probably fire because we don't consider the headers to be system headers when building the library.

Oooh yeah, good call - I hadn't thought about that part of it. I think when I saw the description in the Google style guide that said "all types in the std namespace are assumed to have opted in" I took that literally, but you're right that it's probably a system header filter rather than a std namespace filter.

Would libc++ want to compile itself as system headers? Or does it generally want all/most of the extra warnings when compiling its own implementation, I guess?

  1. We have LIBCXX_ENABLE_WERROR for that. I don't know why we ignore LLVM_ENABLE_WERROR, but it probably has something to do with GCC not building libc++ cleanly.

Ah, weird - yeah, would be nice to unify those at some point - other projects in LLVM have the same issues with GCC warnings and we disable them when necessary/appropriate rather than having LLVM_ENABLE_WERROR ignored when the compiler isn't clang or anything like that.

For example we get warnings that __attribute__((init_priority(100))) is implementation-reserved. When using clang we build with -Werror in the CI.

I think libc++ shouldn't be built with -Wctad-maybe-unsupported, since we can generally assume that using CTAD is OK.

Yeah, that was my feeling too but I wasn't sure where/how to disable it in the libc++ build. But I guess maybe I can add it to this list ( https://github.com/llvm/llvm-project/blob/eaf0aa1f1fbd06fbd66417f2c9d50d3074969824/libcxx/CMakeLists.txt#L592 (updated this review to include that change)) and I don't have to worry about compiler compatibility checks because libcxx uses the just-built clang, so it's not going to run against an outdated clang that'll complain that it doesn't know the warning flag?

BTW it would help if you ping the #libc group next time you want to ask something. That way all the libc++ developers get an email.

Oh, right - thanks for the pointer!

libc++ doesn't always use the just-built clang. We keep support for the last two versions of clang, so the compiler compatibility checks would be needed, right? Was the flag added in llvm14 or llvm15?

I think libc++ shouldn't be built with -Wctad-maybe-unsupported, since we can generally assume that using CTAD is OK.

Yeah, that was my feeling too but I wasn't sure where/how to disable it in the libc++ build. But I guess maybe I can add it to this list ( https://github.com/llvm/llvm-project/blob/eaf0aa1f1fbd06fbd66417f2c9d50d3074969824/libcxx/CMakeLists.txt#L592 (updated this review to include that change)) and I don't have to worry about compiler compatibility checks because libcxx uses the just-built clang, so it's not going to run against an outdated clang that'll complain that it doesn't know the warning flag?

libc++ doesn't always use the just-built clang. We keep support for the last two versions of clang, so the compiler compatibility checks would be needed, right? Was the flag added in llvm14 or llvm15?

ah, OK. Looks like this is supported back to Clang 9: https://godbolt.org/z/5rzMoKoEv (unknown name on Clang 8, no warning on Clang 9)

Mordante accepted this revision.Aug 15 2022, 9:58 AM

libc++ doesn't always use the just-built clang. We keep support for the last two versions of clang, so the compiler compatibility checks would be needed, right? Was the flag added in llvm14 or llvm15?

ah, OK. Looks like this is supported back to Clang 9: https://godbolt.org/z/5rzMoKoEv (unknown name on Clang 8, no warning on Clang 9)

Thanks for looking. It would be even safe when it was added in Clang 15, see my review comment.
The libcxx CI failure is not due to this patch, the time-out for the test seems to be too short.

Can you update the commit summary to reflect the current state of the patch?

LGTM from libc++'s point of view, but please wait for other reviewers to look at the LLVM side.

libcxx/CMakeLists.txt
591

Even if Clang 14 wouldn't support the new flag it would be safe, this CMake macro only adds the flag when it's supported by the compiler.

This revision is now accepted and ready to land.Aug 15 2022, 9:58 AM
kparzysz accepted this revision.Aug 15 2022, 11:02 AM
This revision was landed with ongoing or failed builds.Aug 15 2022, 4:37 PM
This revision was automatically updated to reflect the committed changes.

We (Linaro) have 3 flang bots that enable Werror that are failing. Though https://github.com/llvm/llvm-project/commit/248591aabee7fcc5246b67879b6a71b0bbbc0b9c did help some so thanks for that!

I am looking at how to fix them but will take some time as this is the first time I'm looking at this feature.

We (Linaro) have 3 flang bots that enable Werror that are failing. Though https://github.com/llvm/llvm-project/commit/248591aabee7fcc5246b67879b6a71b0bbbc0b9c did help some so thanks for that!

I am looking at how to fix them but will take some time as this is the first time I'm looking at this feature.

Committed ec3956b6e63c1524d6b024ba5db9ffcd7281ada0 to try to cleanup more - I'll try to follow up/check the buildbots again but feel free to ping me if it looks like it's not settling out.

We (Linaro) have 3 flang bots that enable Werror that are failing. Though https://github.com/llvm/llvm-project/commit/248591aabee7fcc5246b67879b6a71b0bbbc0b9c did help some so thanks for that!

I am looking at how to fix them but will take some time as this is the first time I'm looking at this feature.

Committed ec3956b6e63c1524d6b024ba5db9ffcd7281ada0 to try to cleanup more - I'll try to follow up/check the buildbots again but feel free to ping me if it looks like it's not settling out.

If you want help making f18 be -Wctad-maybe-unsupported clean, please ask for it and I'll work through the failures for you; these unreviewed patches are making me a little nervous.

@dblaikie Since this change causes Flang buildbots to fail and would require more than a few changes in Flang, would it be OK to revert and have the changes in Flang be reviewed before resubmitting?

@dblaikie Since this change causes Flang buildbots to fail and would require more than a few changes in Flang, would it be OK to revert and have the changes in Flang be reviewed before resubmitting?

I'm particularly interested in avoiding backsliding in LLVM if we were to disable the warning entirely & seeing as Flang has broad use of the feature already I've submitted 357e99aff035e981f7d7fc566ffa785925594259 to disable the warning in flang & two follow-up commits to revert my incomplete attempts to address the issue in the Flang codebase - if you/the Flang developers decide the warning is worthwhile, that can now be done independently of ensuring LLVM doesn't regress this constraint. I'm happy to help/discuss/provide examples/etc if that's of interest.