Enable -Wctad-maybe-unsupported in LLVM build

[dblaikie]

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))

[dblaikie]

Posted a thread to discuss this here: https://discourse.llvm.org/t/use-of-class-template-argument-deduction/64507/

[dblaikie]

@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?

[MaskRay]

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

795

typo in comment

[dblaikie]

Update cmake comment and patch description

[dblaikie]

In D131727#3717576, @MaskRay wrote:

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

795

ah, thanks!

[kparzysz]

In D131727#3717576, @MaskRay wrote:

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]

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]

llvm/cmake/modules/HandleLLVMOptions.cmake

795

[dblaikie]

Fix typo

[dblaikie]

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

795

Thanks for the catch!

[jloser]

In D131727#3720366, @dblaikie wrote:

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]

In D131727#3720405, @jloser wrote:

In D131727#3720366, @dblaikie wrote:

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]

With libcxx fixes

[dblaikie]

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..

[philnik]

In D131727#3720886, @dblaikie wrote:

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.

[Mordante]

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.

In D131727#3720507, @dblaikie wrote:

In D131727#3720405, @jloser wrote:

In D131727#3720366, @dblaikie wrote:

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]

remove libcxx fixes and disable the warning for libcxx instead
rebase

[dblaikie]

In D131727#3720994, @philnik wrote:

In D131727#3720886, @dblaikie wrote:

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?

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.

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!

[jloser]

In D131727#3721438, @dblaikie wrote:

In D131727#3720994, @philnik wrote:

In D131727#3720886, @dblaikie wrote:

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?

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.

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?

[dblaikie]

In D131727#3721466, @jloser wrote:

In D131727#3721438, @dblaikie wrote:

In D131727#3720994, @philnik wrote:

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]

In D131727#3722154, @dblaikie wrote:

In D131727#3721466, @jloser wrote:

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 ↗

(On Diff #452457)

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.

[DavidSpickett]

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.

[dblaikie]

In D131727#3725610, @DavidSpickett wrote:

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.

[klausler]

In D131727#3726411, @dblaikie wrote:

In D131727#3725610, @DavidSpickett wrote:

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.

[DavidSpickett]

Awesome thanks for that, it got a bit further: https://lab.llvm.org/buildbot/#/builders/180/builds/6979

[kiranchandramohan]

@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]

In D131727#3726568, @kiranchandramohan wrote:

@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.

llvm/cmake/modules/HandleLLVMOptions.cmake

if (NOT C_WCOMMENT_ALLOWS_LINE_WRAP)
     append("-Wno-comment" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
   endif()
 
   # Enable -Wstring-conversion to catch misuse of string literals.
   add_flag_if_supported("-Wstring-conversion" STRING_CONVERSION_FLAG)
 
   # Prevent bugs that can happen with llvm's brace style.
   add_flag_if_supported("-Wmisleading-indentation" MISLEADING_INDENTATION_FLAG)
+
+  # Enable -Wstring-conversion to catch misuse of string literals.
+  add_flag_if_supported("-Wctad-maybe-unsupported" CTAD_MAYBE_UNSPPORTED_FLAG)
 endif (LLVM_ENABLE_WARNINGS AND (LLVM_COMPILER_IS_GCC_COMPATIBLE OR CLANG_CL))
 
 if (LLVM_COMPILER_IS_GCC_COMPATIBLE AND NOT LLVM_ENABLE_WARNINGS)
   append("-w" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 endif()
 
 macro(append_common_sanitizer_flags)
   if (NOT MSVC)

llvm/include/llvm/CodeGen/RDFGraph.h

namespace rdf {
   template <typename T>
   struct Print {
     Print(const T &x, const DataFlowGraph &g) : Obj(x), G(g) {}
 
     const T &Obj;
     const DataFlowGraph &G;
   };
 
-  template <typename T>
+  template<typename T>
+  Print(const T&, const DataFlowGraph&) -> Print<T>;
+
+  template <typename T>
   struct PrintNode : Print<NodeAddr<T>> {
     PrintNode(const NodeAddr<T> &x, const DataFlowGraph &g)
       : Print<NodeAddr<T>>(x, g) {}
   };
 
   raw_ostream &operator<<(raw_ostream &OS, const Print<RegisterRef> &P);
   raw_ostream &operator<<(raw_ostream &OS, const Print<NodeId> &P);
   raw_ostream &operator<<(raw_ostream &OS, const Print<NodeAddr<DefNode *>> &P);