Intel ICC C++17 compatibility

[mkuron]

Adds C++17 support.

• ICC can’t handle complex enable if’s inline, like enable_if_t<all_of<is_positional<Args>...>::value>>, but if it is refactored into a separate constexpr function, it works.
  • But MSVC doesn't like it if the function is in the return type.
• Fold expressions cause an internal compiler error, so we have to avoid turning them on even in C++17 mode
• Plain noexcept can cause weird errors with following syntax, noexcept(true) works fine, workaround in tests

This also fixes the NVIDIA HPC SDK compiler in C++17 mode.

Description

The Intel compiler has trouble with pybind11 when compiling in C++17 mode. The first two examples in the code below result in errors like "no instance of function template [...] matches the argument list", while the third one triggers an internal compiler error.

#include <pybind11/pybind11.h>

void test1() {
  pybind11::print("Hello World");
}

void test2(pybind11::object obj) {
   obj();
}

class C {
};

void test3(pybind11::module_ &m) {
	pybind11::class_<C>(m, "C");
}

Details can be found in #2707. @tobiasleibner implemented the fix for the template mismatch, while the workaround for the internal compiler error is mine. Part of @tobiasleibner's work went into #2573 already, which hasn't been merged yet. Since he currently doesn't have time, he suggested I post a pull request.

Related to #2573
Fixes #2714
Fixes #2707

Suggested changelog entry:

* Support ICC and NVIDIA HPC SDK in C++17 mode

[tobiasleibner]

Thanks for creating the PR!

Maybe you can add a comment to your fix explaining why it is needed? Just a short sentence similar to the comments I added to my changes.

You should probably rebase on/merge in the master branch, maybe that fixes some of the non-ICC failures in CI. Not sure what the test failure in the ICC branches is about, I think I haven't seen that error before.

[mkuron]

Thanks for the comment. I added a note about the internal compiler error and merged master (still getting unrelated CI errors though). It might make sense to wait for #2573 to be merged first and then squash/rebase the present pull request to eliminate the overlap.

[bstaletic]

Rebase is needed. Plus there's that extra empty line in the yaml file.

[henryiii]

CI always bases "PR" builds on the latest master when it changes (including when it reopens). I think the empty line was in a different PR and I fixed it already.

[henryiii]

Oh, you mean it needs to be rebased after #2573? Yes, apparently so.

[bstaletic]

Oh, you mean it needs to be rebased after #2573? Yes, apparently so.

No, no. I thought it needed a rebase after fixing the CI.

---

The test_aligned() fails and it could be argued that it's pybind's fault. This test wants to make sure that an overaligned type is correctly overaligned by specifying alignas(1024). Here's why this doesn't work:

1. The definition of "extended alignment"/"overaligned type" is here. It's essentially any type for which alignof(T) > alignof(std::max_align_t).
2. The same clause of the standard says that exact support for overaligned types/extended alignment is implementation defined.
3. This clause explains what are the requirements of alignas(1024).
   3.1. The first requirement just says that our 1024 has to be a compile time constant integer.
   3.2. The second requirement says that if our 1024 is not supported by the C++ implementation, the program is ill-formed.

Therefore, ICC is well within its rights to disregard the extended alignment request. Granted, the standard says ill-formed, not ill-formed, no diagnostic required. At least according to cppreference, ICC is required to display a warning.

---

For what it's worth, we asked for a 1024-byte alignment and we got 16-byte alignment.

[bstaletic]

On the other hand, compiler explorer says that 1024-byte alignment works fine in ICC 21.1.

https://godbolt.org/z/f8oqPq

[mkuron]

Okay, so what do we do about the alignment test? I played around with it a bit yesterday and was unable to fix it, but also unable to reproduce outside pybind11. Disable it on Intel?

[bstaletic]

https://github.com/pybind/pybind11/blob/master/tests/test_class.cpp#L392-L399

Maybe just add && !defined(__INTEL__) or whatever is the ICC-identifying macro? Though I'm curious if ICC would respect alignment in an extension module if we remove pybind11:

#include <Python.h>

struct alignas(1024) Aligned {
	uintptr_t ptr() { return (uintptr_t)this; }
};

typedef struct {
    PyObject_HEAD
    Aligned* aligned;
} PyAligned;

static void PyAligned_dealloc(PyAligned *self) {
    delete self->aligned;
    Py_TYPE(self)->tp_free((PyObject *) self);
}

static PyObject * PyAligned_ptr(PyAligned *self, PyObject *Py_UNUSED(ignored)) {
    if (self->aligned == NULL) {
        PyErr_SetString(PyExc_AttributeError, "aligned");
        return NULL;
    }
    return PyLong_FromUnsignedLong(self->aligned->ptr());
}
static PyObject * PyAligned_new(PyTypeObject *type, PyObject *args, PyObject *kwds) {
    PyAligned *self = (PyAligned *) type->tp_alloc(type, 0);
    if (self != NULL) self->aligned = new Aligned;
    return (PyObject *) self;
}


static PyMethodDef PyAligned_methods[] = {
    {"ptr", (PyCFunction) PyAligned_ptr, METH_NOARGS},
    {NULL}  /* Sentinel */
};

static PyTypeObject AlignedType = {
    PyVarObject_HEAD_INIT(NULL, 0)
    .tp_name = "t.Aligned",
    .tp_basicsize = sizeof(PyAligned),
    .tp_itemsize = 0,
    .tp_dealloc = (destructor) PyAligned_dealloc,
    .tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,
    .tp_methods = PyAligned_methods,
    .tp_new = PyAligned_new,
};

static PyModuleDef tmodule = {
    PyModuleDef_HEAD_INIT,
    .m_name = "t",
    .m_size = -1,
};

PyMODINIT_FUNC PyInit_t(void) {
    if (PyType_Ready(&AlignedType) < 0)
        return NULL;

    PyObject* m = PyModule_Create(&tmodule);
    if (m == NULL) return NULL;

    Py_INCREF(&AlignedType);
    if (PyModule_AddObject(m, "Aligned", (PyObject *) &AlignedType) < 0) {
        Py_DECREF(&AlignedType);
        Py_DECREF(m);
        return NULL;
    }

    return m;
}

Test with:

python -c 'print(__import__("t").Aligned().ptr() % 1024)'

One difference between what pybind11 is doing and what the above (almost) C is doing is how the bindings are made. Pybind11 adds an empty type object to the module object and then adds python methods to modules or types, as it encounters def calls. The above C statically prepares the entire type object and adds the complete thing to the t module.

[mkuron]

$ icpc -std=c++17 -shared -fPIC -o t.so t.cpp $(python3-config --cflags --ldflags)
$ python3 -c 'print(__import__("t").Aligned().ptr() % 1024)'
736

So it's not aligned either. I'll add the #ifdef __INTEL_COMPILER to disable the test.

[bstaletic]

The remaining test failure, is also present in the C++11 ICC run. I'm not sure what's going on there.

As for the alignment problem, here's a tiny example that repro's the problem: https://godbolt.org/z/MYj5r5
Considering that lbistdc++ does have aligned new and the ICC uses whatever standard library comes with the system, this has to be a result of ICC not defining some feature test macro!

Sure enough, libstdc++ protects the aligned new operators with __cpp_aligned_new. So, what happens if we lie a bit?
https://godbolt.org/z/sbM4vK

cppreference says that ICC supports aligned new since version 19, but Intel seems to be lazy about feature test macros...
https://en.cppreference.com/w/cpp/compiler_support

[mkuron]

Sure enough, libstdc++ protects the aligned new operators with __cpp_aligned_new. So, what happens if we lie a bit?

Excellent find! If I add the lie to your pybind11-less example from earlier today, it aligns correctly. What would you think about adding

#if defined(__INTEL_COMPILER) && __INTEL_COMPILER >= 1900 && defined(PYBIND11_CPP17)
  // Intel compiler doesn't advertise that it supports alignas, so libstdc++ doesn't
  // enable the aligned new operators by default.
  #define __cpp_aligned_new 1
#endif

somewhere near

pybind11/include/pybind11/detail/common.h

Line 104 in 7bd4b39

?

[bstaletic]

I was wary about suggesting that. I'm not sure what would happen if one translation contained that lie but another did not and both tried to use overaligned types? We would still be in ill-formed land, with a bit of ODR violations sprinkled around? Would it be possible to construct a case where one TU would think the pointer is overaligned when it really isn't?

That's why I don't want to make the decision and would rather let others chime in with their thoughts.

[mkuron]

I've found an alternative and less hacky solution: adding #include <aligned_new>. That's an Intel-provied header file that defines global operator news and operator deletes and is documented as the official solution to getting aligned new support in ICC: https://software.intel.com/content/www/us/en/develop/articles/aligned-operator-new-support-in-intel-c-compiler.html, https://software.intel.com/content/www/us/en/develop/documentation/cpp-compiler-developer-guide-and-reference/top/optimization-and-programming-guide/automatically-aligned-dynamic-allocation/automatically-aligned-dynamic-allocation-1.html . The documentation says nothing about that one has to include this header in all translation units, so I assume it's safe to only do so in some.

Since this is Intel's official (and not standard-compliant) way of requesting aligned new support, I guess the correct thing for us to do would be to not add #include <aligned_new> to the pybind11 headers, but only do so in test_class.cpp. If someone wants to use alignas in the code they are interfacing with pybind11, they should already be including that file if they want to build with Intel.

[bstaletic]

That header, at least on linux, overrides the aligned new operators and makes them a thin wrapper around posix_memalign().

I guess the correct thing for us to do would be to not add #include <aligned_new> to the pybind11 headers, but only do so in test_class.cpp.

Does ICC support -include flag, like gcc and clang do? If so, we could add -include aligned_new to CXXFLAGS in cmake, when we detect C++17 and ICC.

[mkuron]

Does ICC support -include flag, like gcc and clang do?

It does, but anyone using alignas in their code with an Intel compiler should already be including aligned_new per Intel's documentation. Since pybind11 doesn't do any aligning by itself, it's not really our job to inject it into the CXXFLAGS. We would only include it in the test case.

The remaining test failure, is also present in the C++11 ICC run. I'm not sure what's going on there.

Regarding that failing test_python_alreadyset_in_destructor: I cannot reproduce the problem locally.

[henryiii]

I'd be okay just to disable that test for now (for Intel) to get this in.

[mkuron]

Okay, skipping the test now. I don't really understand how the exception handling works, so I don't think I would have been able to fix it. Perhaps the original author of that test (@jbarlow83, #2372) has an idea and could fix it at some later point.

Thanks for milestoning, looking forward to seeing full Intel support in a release soon.

[henryiii]

Looks like aligned is acting up again?

[jbarlow83]

@mkuron

1. What's the error message for the test_python_alreadyset_in_destructor failure? Or does it core dump?
2. If you comment out the line in test_exceptions.cpp, ex.discard_as_unraisable(s);, how does the test fail?

[mkuron]

@jbarlow83: I cannot reproduce this error locally, so I can only answer your first question from the CI build log:

../../tests/test_exceptions.py:77: AssertionError
=============================== warnings summary ===============================
test_exceptions.py::test_python_alreadyset_in_destructor
  /home/runner/.local/lib/python3.8/site-packages/_pytest/unraisableexception.py:78: PytestUnraisableExceptionWarning: Exception ignored in: 'already_set demo'
  
  Traceback (most recent call last):
    File "/home/runner/work/pybind11/pybind11/tests/test_exceptions.py", line 71, in test_python_alreadyset_in_destructor
      assert m.python_alreadyset_in_destructor("already_set demo") is True
  KeyError: 'bar'
  
    warnings.warn(pytest.PytestUnraisableExceptionWarning(msg))

[mkuron]

Looks like aligned is acting up again?

Sorry about that, I put the #ifdef in the wrong place. We need to #include <aligned_new> before pybind11 is included so the new operator gets picked up everywhere, but I accidentally moved it to a place where PYBIND11_CPP17 is not defined yet. Should be fixed now.

[jbarlow83]

What failed was our test for whether an appropriate error message was sent to stderr. I suspect it's related to the version of pytest or somehow CI-related. It should be compiler independent.

(The traceback is not a concern. That part is supposed to happen.)

[mkuron]

I suspect it's related to the version of pytest or somehow CI-related. It should be compiler independent.

@jbarlow83, I could not reproduce this on Ubuntu 18.04 after upgrading pytest to the same version as provided by Ubuntu 20.04. I also could not reproduce it on Ubuntu 20.04 with GCC instead of Intel ICC (I don't have Intel available on that machine). The NVCC job in CI is also running on Ubuntu 20.04 though, so it would have the exact same Python environment as the ICC job, yet it does not exhibit the problem -- so there might be some compiler dependency. No idea why the choice of compiler should affect the capturing of stderr inside pytest though.

[henryiii]

Is there a way to pin a version? This seems to just pull "latest", which could change and break our tests.

[henryiii]

We need a sign off from @wjakob for this one. He’s worried about worried about working around bugs in the Intel compiler instead of getting Intel to fix their bugs. If we provide a clear path forward, that will help. Tomorrow I’m going to see if the old ifdef workarounds are still needed. If we can drop a few workarounds while adding more, that will help. Yannicks idea for cataloging workarounds could help too. I’ll also try to see if I can get the bugs reported to Intel if I don’t have an issue number for them (I have very few currently, maybe one).

[ax3l]

@jeffhammond are your compiler folks interested in triaging those oneAPI icpc C++17 bugs? (See also #2573 for baseline ICC-hackery)

[ax3l]

C++17 compiler bug request submitted as intel bug 04916230 on https://supporttickets.intel.com

[henryiii]

C++17 compiler bug request submitted

Which bug is this? The enable_if_t problems on C++17? Can we collect a list (@ax3l please edit below if needed)?

• arg()/arg{} transformation lifetime issue submitted as intel bug 04916223
• C++17 compiler bug (failing enable_if_t usage) request submitted as intel bug 04916230

(This would be a great use for GitHub Discussions, actually ;) )

[ax3l]

Yes, the enable_if_t part in this PR. I pointed to both PRs in the bug reports with a brief summary, so that the compiler devs can see the work-arounds needed for ICC. It's probably a good approach to rebase this PR once #2573 is merged so the work-arounds here are easier to spot.

[mkuron]

It's probably a good approach to rebase this PR once #2573 is merged so the work-arounds here are easier to spot.

Will do. I‘ll squash the commits into one per bug report and add the bug numbers to the commit messages. Could you please also file a bug report for the internal compiler error in PYBIND11_EXPAND_SIDE_EFFECTS?

[henryiii]

@bstaletic @YannickJadoul Any idea why MSVC hates my final commit, but everybody else is fine with it, and it looks pretty much like what we are already doing? f9ccfd2

[YannickJadoul]

@henryiii, what about using satisfies_all_of? Does that help?

EDIT: Well, I now see that's going back to what we +- had, so probably not :-(

[henryiii]

Interesting, so MSVC gets a different any_of/all_of. I could try satisfies_any_of, just in case, but it ought to be similar. Don't see how <expression> and !<expression> can manage not to be different.

[YannickJadoul]

To state the obvious: the difference with the working example above is that enable_if is here the return value, rather than an optional template argument. So maybe it gets SFINAEd too late? The return value is not part of overloading, so ... maybe?

Is there a way we can add this as an argument? Or maybe in two steps, with an _impl, or so?

[bstaletic]

MSVC not liking valid templates in C++17 usually smells of a missing /permissive- flag. Does that help?

[henryiii]

It should be there in some places, and it's also failing in 14. It's failing for all MSVC, 2015, 2017, and 2019.

[henryiii]

I think I came up with an impl that works. If it passes all the way, I'll push a comment and README update.

[henryiii]

Okay, ready for a review. I've cleaned up the one ugly workaround, and added greppable comments and such, and listed the three workarounds implemented at the top of the PR.

[YannickJadoul]

Is MSVC failing on the constexpr or on the combination with std::enable_if?

[henryiii]

The original all-in-one was too complicated for ICC to compile. Refactoring it to match the others, with a constexpr function, confused MSVC if it was in the return type. This is simpler to compile and avoids a constexpr function in a return type. As I don't think that this function is an external interface, and it's only used here, I could avoid the intermediate function and call the "impl" style function directly, if that's seen as cleaner.

[YannickJadoul]

After #2573, I think this one can go in as well. Workarounds are never pretty, but let's hope we can get rid of them soon enough, if Intel is working on it :-)