Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replacing ICC C++14 with C++17 #3570

Merged
merged 5 commits into from
Jan 11, 2022
Merged

Replacing ICC C++14 with C++17 #3570

merged 5 commits into from
Jan 11, 2022

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Dec 23, 2021

Description

This PR reverts most of PR #3551. To work around the issue that started with Intel 2021.5.0.20211109, -Wno-conversion is added specifically for C++17 (as recommended here: #3570 (comment)).

Quality assurance: #3570 (comment)

Original PR description

For reporting to Intel.

The C++17 build was working with

  • Intel 2021.4.0.20210910 (/opt/intel/oneapi/compiler/2021.4.0/linux/bin/intel64/icpc)

but started failing with

  • Intel 2021.5.0.20211109 (/opt/intel/oneapi/compiler/2022.0.0/linux/bin/intel64/icpc)

See also: PR #3551

Suggested changelog entry:

- Fix: the latest release warned in ``#include <variant>``, which failed ``-DPYBIND11_WERROR=ON``

@rwgk rwgk force-pushed the icc_latest_17 branch 2 times, most recently from 7014775 to 4d501a1 Compare January 4, 2022 15:38
@ax3l
Copy link
Collaborator

ax3l commented Jan 5, 2022

@rscohn2 @xmnboy would this be an oneAPI icc/icpc regression worth filing an internal ticket for?
We use pybind11 in our upcoming Python bindings for many of our C++ codes and C++17 builds recently broke with the latest oneAPI icpc release. Our codes are currently all using C++17.

[  8%] Building CXX object tests/CMakeFiles/pybind11_tests.dir/test_buffers.cpp.o
cd /home/runner/work/pybind11/pybind11/build-17/tests && /opt/intel/oneapi/compiler/2022.0.1/linux/bin/intel64/icpc -Dpybind11_tests_EXPORTS -I/home/runner/work/pybind11/pybind11/include -isystem /usr/include/python3.8 -Os -DNDEBUG -fPIC -fvisibility=hidden -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -Werror-all -diag-disable 11074,11076 -ipo -std=c++17 -MD -MT tests/CMakeFiles/pybind11_tests.dir/test_buffers.cpp.o -MF CMakeFiles/pybind11_tests.dir/test_buffers.cpp.o.d -o CMakeFiles/pybind11_tests.dir/test_buffers.cpp.o -c /home/runner/work/pybind11/pybind11/tests/test_buffers.cpp
In file included from /home/runner/work/pybind11/pybind11/include/pybind11/stl.h(31),
                 from /home/runner/work/pybind11/pybind11/tests/test_buffers.cpp(12):
/usr/include/c++/9/variant(123): error: integer conversion resulted in a change of sign
    inline constexpr size_t variant_npos = -1;
                                           ^

In file included from /home/runner/work/pybind11/pybind11/include/pybind11/stl.h(31),
                 from /home/runner/work/pybind11/pybind11/tests/test_buffers.cpp(12):
/usr/include/c++/9/variant(1727): error: integer conversion resulted in a change of sign
  	constexpr size_t __magic_monostate_hash = -7777;
  	                                          ^

This looks like a general problem with std::variant/#include <variant>:

# include <variant>

We see this on our GitHub actions CI, using the ubuntu-20.04 image (see this PR) and installing the oneAPI .deb packages for icc/icpc classic compilers.

@rscohn2
Copy link

rscohn2 commented Jan 5, 2022

I filed a ticket. I don't know if it will be fixed because there are workarounds (disabling warnings) and we are transitioning to icpx. Here is the reproducer:

rscohn1@rscohn1-mobl1:icx$ /opt/intel/oneapi/compiler/2022.0.1/linux/bin/intel64/icpc -Wconversion -std=c++17 -Werror-all -c variant\
.cpp
In file included from variant.cpp(1):
/usr/include/c++/9/variant(123): error: integer conversion resulted in a change of sign
    inline constexpr size_t variant_npos = -1;
                                           ^

In file included from variant.cpp(1):
/usr/include/c++/9/variant(1727): error: integer conversion resulted in a change of sign
        constexpr size_t __magic_monostate_hash = -7777;
                                                  ^

compilation aborted for variant.cpp (code 2)
rscohn1@rscohn1-mobl1:icx$ cat variant.cpp
#include <variant>
rscohn1@rscohn1-mobl1:icx$ /opt/intel/oneapi/compiler/2021.4.0/linux/bin/intel64/icpc -Wconversion -std=c++17 -Werror-all -c variant\
.cpp
rscohn1@rscohn1-mobl1:icx$ /opt/intel/oneapi/compiler/2022.0.1/linux/bin/intel64/icpc --version
icpc (ICC) 2021.5.0 20211109
Copyright (C) 1985-2021 Intel Corporation.  All rights reserved.

rscohn1@rscohn1-mobl1:icx$ /opt/intel/oneapi/compiler/2021.4.0/linux/bin/intel64/icpc --version
icpc (ICC) 2021.4.0 20210910
Copyright (C) 1985-2021 Intel Corporation.  All rights reserved.

rscohn1@rscohn1-mobl1:icx$

@ax3l
Copy link
Collaborator

ax3l commented Jan 6, 2022

Thanks a lot @rscohn2!
I see, this is then just a diagnostic that went off for the shipped <variant> include.

Yes, fully understand with respect to the icx/icpx effort. We actually try to add coverage for it as well in #2769 :)

@rscohn2
Copy link

rscohn2 commented Jan 6, 2022

The changes you proposed are good workarounds, and I am glad to see you are adding icpx support.

Ralf W. Grosse-Kunstleve and others added 3 commits January 10, 2022 09:40
Try to suppress the `-Werror-all` promotion in `#include <variant>`
@rwgk
Copy link
Collaborator Author

rwgk commented Jan 10, 2022

-DPYBIND11_WERROR=OFF works but -diag-disable:conversion did not:

icpc: warning #10034: Unrecognized keyword 'conversion' for option '-diag-disable'
In file included from /home/runner/work/pybind11/pybind11/include/pybind11/stl.h(31),
                 from /home/runner/work/pybind11/pybind11/tests/test_buffers.cpp(12):
/usr/include/c++/9/variant(123): error: integer conversion resulted in a change of sign
    inline constexpr size_t variant_npos = -1;
                                           ^

@rscohn2 is there a way to enable building with -Werror-all while selectively disabling this diagnostic? I do not see a warning number that we could use with -diag-disable.

@ax3l
Copy link
Collaborator

ax3l commented Jan 10, 2022 via email

@rwgk rwgk marked this pull request as ready for review January 10, 2022 18:17
@rwgk rwgk requested a review from henryiii as a code owner January 10, 2022 18:17
@rwgk
Copy link
Collaborator Author

rwgk commented Jan 10, 2022

Try maybe -Wno-conversion?

CI is running.

@rwgk
Copy link
Collaborator Author

rwgk commented Jan 10, 2022

For quality assurance, copying parts of the C++11 and C++17 compilation commands from the log:

-Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -Werror-all -diag-disable 11074,11076 -ipo -std=c++11 -MD -MT
-Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -Wno-conversion -Werror-all -diag-disable 11074,11076 -ipo -std=c++17 -MD -MT

@rwgk rwgk requested a review from Skylion007 January 10, 2022 23:32
@rwgk
Copy link
Collaborator Author

rwgk commented Jan 10, 2022

@henryiii @ax3l @Skylion007 I think this is good to go in.

@rwgk
Copy link
Collaborator Author

rwgk commented Jan 10, 2022

In view of the transition to icpx, I feel it's not worth the effort troubleshooting the icpc C++20 issue(s).

Copy link
Collaborator

@henryiii henryiii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, irritating to have to turn off a warning, but it's not ours (and the other compilers should catch if we make a mistake)

@ax3l ax3l self-assigned this Jan 11, 2022
@ax3l ax3l merged commit f588810 into pybind:master Jan 11, 2022
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jan 11, 2022
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Jan 12, 2022
@rwgk rwgk deleted the icc_latest_17 branch January 12, 2022 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants