search

SWI-Prolog / packages-cpp

The SWI-Prolog C++ interface
29 stars 13 forks source link

9.2.6: Use of deleted function #87

Closed jamesjer closed 1 month ago

jamesjer commented 1 month ago

I just attempted to build swipl 9.2.6 for Fedora Rawhide, with gcc 14.1.1. The build failed while building the cpp package:

In file included from /builddir/build/BUILD/pl-9.2.6-build/swipl-9.2.6/packages/swipl-win/SwiPrologEngine.h:40,
                 from /builddir/build/BUILD/pl-9.2.6-build/swipl-9.2.6/packages/swipl-win/ConsoleEdit.h:51,
                 from /builddir/build/BUILD/pl-9.2.6-build/swipl-9.2.6/packages/swipl-win/Preferences.h:41,
                 from /builddir/build/BUILD/pl-9.2.6-build/swipl-9.2.6/packages/swipl-win/ansi_esc_seq.cpp:35:
/builddir/build/BUILD/pl-9.2.6-build/swipl-9.2.6/packages/cpp/SWI-cpp2.h: In member function ‘PlTermScoped PlTermScoped::release()’:
/builddir/build/BUILD/pl-9.2.6-build/swipl-9.2.6/packages/cpp/SWI-cpp2.h:819:12: error: use of deleted function ‘PlTermScoped::PlTermScoped(const PlTermScoped&)’
  819 |     return PlTermScoped(t);
      |            ^~~~~~~~~~~~~~~
/builddir/build/BUILD/pl-9.2.6-build/swipl-9.2.6/packages/cpp/SWI-cpp2.h:797:3: note: declared here
  797 |   PlTermScoped(const PlTermScoped&) = delete;
      |   ^~~~~~~~~~~~
/builddir/build/BUILD/pl-9.2.6-build/swipl-9.2.6/packages/cpp/SWI-cpp2.h:819:12: note: use ‘-fdiagnostics-all-candidates’ to display considered candidates
  819 |     return PlTermScoped(t);
      |            ^~~~~~~~~~~~~~~
JanWielemaker commented 1 month ago

I hope @kamahen understands this ... Just to be sure I ran a clean build on Fedora 40 (also gcc 14.1). No issues. I had the same problem trying to build a snap that is based on Ubuntu 20.4 (gcc 9) and building the Docker image based on Debian Bullseye (gcc 10), so I thought this was about old g++ versions.

C++ is too complicated for simple souls like me :cry:

kamahen commented 1 month ago

It might be due to incomplete support for some C++-17 features. I'll try to figure it out, but I'd first need to build gcc 9 to confirm.

On Wed, 24 Jul 2024 at 08:10, Jan Wielemaker @.***> wrote:

I hope @kamahen https://github.com/kamahen understands this ... Just to be sure I ran a clean build on Fedora 40 (also gcc 14.1). No issues. I had the same problem trying to build a snap that is based on Ubuntu 20.4 (gcc 9) and building the Docker image based on Debian Bullseye (gcc 10), so I thought this was about old g++ versions.

C++ is too complicated for simple souls like me 😢

— Reply to this email directly, view it on GitHub https://github.com/SWI-Prolog/packages-cpp/issues/87#issuecomment-2248264189, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIIGNNNICD3G4HNVI4VKSVDZN67U3AVCNFSM6AAAAABLMUVG3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBYGI3DIMJYHE . You are receiving this because you were mentioned.Message ID: @.***>

kamahen commented 1 month ago

If this does turn out to be a problem in an old compiler, what's the best solution?

BTW, I suspect a problem with "move" semantics, which are a relatively recent addition to C++ -- it seems that there were a number of defects reported in the C-11 spec.

On Wed, 24 Jul 2024 at 09:39, Peter Ludemann @.***> wrote:

It might be due to incomplete support for some C++-17 features. I'll try to figure it out, but I'd first need to build gcc 9 to confirm.

On Wed, 24 Jul 2024 at 08:10, Jan Wielemaker @.***> wrote:

I hope @kamahen https://github.com/kamahen understands this ... Just to be sure I ran a clean build on Fedora 40 (also gcc 14.1). No issues. I had the same problem trying to build a snap that is based on Ubuntu 20.4 (gcc 9) and building the Docker image based on Debian Bullseye (gcc 10), so I thought this was about old g++ versions.

C++ is too complicated for simple souls like me 😢

— Reply to this email directly, view it on GitHub < https://github.com/SWI-Prolog/packages-cpp/issues/87#issuecomment-2248264189>,

or unsubscribe < https://github.com/notifications/unsubscribe-auth/AIIGNNNICD3G4HNVI4VKSVDZN67U3AVCNFSM6AAAAABLMUVG3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBYGI3DIMJYHE>

. You are receiving this because you were mentioned.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/SWI-Prolog/packages-cpp/issues/87#issuecomment-2248461231, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIIGNNPUK3F6FBCEOARO3Y3ZN7KCLAVCNFSM6AAAAABLMUVG3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBYGQ3DCMRTGE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

jamesjer commented 1 month ago

Indeed, move semantics seem to be at play here. On a whim, I commented out lines 797 and 798:

  //PlTermScoped(const PlTermScoped&) = delete;
  //PlTermScoped& operator=(PlTermScoped const&) = delete;

Then I added the suggested -fdiagnostics-all-candidates compiler flag. Now the error reads:

In file included from /builddir/build/BUILD/pl-9.2.6-build/swipl-9.2.6/packages/swipl-win/SwiPrologEngine.h:40,
                 from /builddir/build/BUILD/pl-9.2.6-build/swipl-9.2.6/packages/swipl-win/ConsoleEdit.h:51,
                 from /builddir/build/BUILD/pl-9.2.6-build/swipl-9.2.6/packages/swipl-win/FlushOutputEvents.cpp:35:
/builddir/build/BUILD/pl-9.2.6-build/swipl-9.2.6/packages/cpp/SWI-cpp2.h: In member function 'PlTermScoped PlTermScoped::release()':
/builddir/build/BUILD/pl-9.2.6-build/swipl-9.2.6/packages/cpp/SWI-cpp2.h:819:12: error: use of deleted function 'constexpr PlTermScoped::PlTermScoped(const PlTermScoped&)'
  819 |     return PlTermScoped(t);
      |            ^~~~~~~~~~~~~~~
/builddir/build/BUILD/pl-9.2.6-build/swipl-9.2.6/packages/cpp/SWI-cpp2.h:769:7: note: 'constexpr PlTermScoped::PlTermScoped(const PlTermScoped&)' is implicitly declared as deleted because 'PlTermScoped' declares a move constructor or move assignment operator
  769 | class PlTermScoped : public PlTerm
      |       ^~~~~~~~~~~~
/builddir/build/BUILD/pl-9.2.6-build/swipl-9.2.6/packages/cpp/SWI-cpp2.h:769:7: note: candidate: 'constexpr PlTermScoped::PlTermScoped(const PlTermScoped&)' (deleted)
/builddir/build/BUILD/pl-9.2.6-build/swipl-9.2.6/packages/cpp/SWI-cpp2.h:780:3: note: candidate: 'PlTermScoped::PlTermScoped(term_t)'
  780 |   PlTermScoped(term_t t)
      |   ^~~~~~~~~~~~
/builddir/build/BUILD/pl-9.2.6-build/swipl-9.2.6/packages/cpp/SWI-cpp2.h:784:12: note: candidate: 'PlTermScoped::PlTermScoped(PlTermScoped&&)' (ignored)
  784 |   explicit PlTermScoped(PlTermScoped&& moving) noexcept
      |            ^~~~~~~~~~~~
/builddir/build/BUILD/pl-9.2.6-build/swipl-9.2.6/packages/cpp/SWI-cpp2.h:776:12: note: candidate: 'PlTermScoped::PlTermScoped(PlTerm)' (ignored)
  776 |   explicit PlTermScoped(PlTerm t)
      |            ^~~~~~~~~~~~
/builddir/build/BUILD/pl-9.2.6-build/swipl-9.2.6/packages/cpp/SWI-cpp2.h:772:12: note: candidate: 'PlTermScoped::PlTermScoped()' (ignored)
  772 |   explicit PlTermScoped()
      |            ^~~~~~~~~~~~

Just to emphasize: this is with GCC 14.1.1, a very new compiler.

kamahen commented 1 month ago

There are also reports with an old compiler, so I wonder what's going on.

Where did you get GCC 14.1.1? I sync-ed to the latest at https://gcc.gnu.org/git/gcc.git and the only "14" tags I saw were shows basepoints/gcc-14. releases/gcc-14.1.0. Apparently GCC 13 has "Simpler implicit move", whatever that means.

(I've read and re-read the documentation on "move" semantics and admit to not fully understanding it, although I think that PlTermScoped uses a fairly simple flavor of move semantics. I could remove the use of "move", but that would open up some potential bugs if PlTermSolved isn't used properly, and the compiler wouldn't complain.

Anyway, for now, I'll build gcc 14.1.0 and see what happens when I use it. This could take a few days (I have some other things that are keeping me busy).

jamesjer commented 1 month ago

It's in Fedora 40 and Rawhide. The gcc spec file says:

# The source for this package was pulled from upstream's vcs.
# %%{gitrev} is some commit from the
# https://gcc.gnu.org/git/?p=gcc.git;h=refs/vendors/redhat/heads/gcc-%%{gcc_major}-branch
# branch.  Use the following command to generate the tarball:
# ./update-gcc.sh %%{gitrev}

That tells me that version 14.1.1 may not be generally available yet.

kamahen commented 1 month ago

I was not able to reproduce the problem with 14.1.0. :( Is there a way to look a .o or .so file, to verify which compiler it was built with?

@jan reported that the problem also shows with g++-10, but I couldn't find that package for Debian bookworm (there is a gcc-10, but that doesn't help). And there doesbn't seem to be a gcc-13 or gcc-14. I'm not sure how to proceed in replicating the problem, but perhaps I can figure it out from the error messages. On the other hand, it might be a problem with 14.1.1?

kamahen commented 1 month ago

One other thing -- SWI-cpp2.h requires C++-17 -- I recall seeing error messages like that when only C++-11 was required. I noticed that the CMakeLists.txt has C++-20 for MSVC, so could you try changing to C++-20 for 14.1.1 and see if that fixes thing?

kamahen commented 1 month ago

I've confirmed my C++ compiler version (using current_prolog_flag(c_cxx,V)) and everything compiles, if I specify C++-17 or later -- it fails for C++-14 and C++-11. Looking at the list of C++ features (https://gcc.gnu.org/projects/cxx-status.html#cxx17), it appears that my code requires "Guaranteed copy elision" (I'm going by the title; the referenced text in the standard might as well be written in Rongorongo, for all that I can understand it). (This probably is also the situation for older compilers that I don't have, such as g++ 9 and 10; support for C++-17 was added in g++ 7)

Before I proceed further with this, please confirm that you're specifying C++-17 or later. The file packages/swipl-win/CMakeLists.txt has 2 places with set(CMAKE_CXX_STANDARD 17) (inside if(Qt5Widgets_FOUND)) and the file packages/cpp/CMakeLists.txt has a few places also.

The commits that I used: cpp: 57bdfad62eb87f5b93a631696dc9be22936acb66 swipl-win: 41b2d3ccefefc413582dd92627d58a316ea873b1

PS: @jan - does swipl-ld ensure the appropriate -std flag? Also, when I experimented with changing the CMAKE_CXX_STANDARD flag to 11, I noticed that the compilation flags included

-std=c++17
-std=gnu++11

which is a bit surprising. The full command line was:

/home/peter/.local/gcc-14/bin/g++ -Dplugin_test_cpp_EXPORTS -I/home/peter/src/swipl-devel/build.debug/packages/cpp -I/home/peter/src/swipl-devel/src/os -I/home/peter/src/swipl-devel/src -DO_DEBUG -O0 -gdwarf-2 -g3  -DO_CHECK_TERM_REFS -DSECURE_GC -DCHK_SECURE -DO_MAINTENANCE -gdwarf-2 -g3 -fPIC -Wall -D__SWI_PROLOG__ -std=c++17 -Wpedantic -std=gnu++11 -MD -MT packages/cpp/CMakeFiles/plugin_test_cpp.dir/test_cpp.cpp.o -MF packages/cpp/CMakeFiles/plugin_test_cpp.dir/test_cpp.cpp.o.d -o packages/cpp/CMakeFiles/plugin_test_cpp.dir/test_cpp.cpp.o -c /home/peter/src/swipl-devel/packages/cpp/test_cpp.cpp

BTW, looking at that command line, my extra CXXFLAGS don't seem to be respected by Cmake ... how do I add things like -Wextra -Wsign-compare -Wshadow without modifying the CMakeList.txt files?

JanWielemaker commented 1 month ago

@kamahen, adding -std=c++17 to the Makefile of hdt fixes that for gcc-10. It also compiles swipl-win cleanly. Also gcc-9 compiles swipl-win cleanly. This makes we wonder why it fails to build in the snap builder ... I'll try to figure out later.

CFLAGS and CXXFLAGS should be hounered by CMake as initial set of flags. AFAIK, this works for me. It probably only works on the first call though, i.e., it might not work if you run cmake again in the same build dir to update te configuration.

Thanks! --- Jan

JanWielemaker commented 1 month ago

@jamesjer , My Fedora 40 also uses gcc 14.1.1 20240701 (Red Hat 14.1.1-7). I see no issues.

kamahen commented 1 month ago

So, this can be closed? Does swipl-ld do the right thing with C++ -std= flags?

jamesjer commented 1 month ago

I see the problem. I've got it building against Qt5 headers, so packages/swipl-win/CMakeLists.txt does:

if(Qt5Widgets_FOUND)
  set(CMAKE_CXX_STANDARD 11)

I didn't realize Qt6 support was available. Switching to Qt6 results in a clean build, with no other action necessary.

kamahen commented 1 month ago

Your swipl-win/CmakeLists.txt seems to be out of date; the current one specifies C++-17 for both with and without Qt5. commit 41b2d3ccefefc413582dd92627d58a316ea873b1

JanWielemaker commented 1 month ago

Your swipl-win/CmakeLists.txt seems to be out of date

That explains. I missed updating the swipl-win submodule for the 9.2.6 release. As I have Qt6, this caused no problems. I think we can close this. I'll probably have to release 9.2.7 to deal with the Docker and snap issues :cry:

The git source is updated. Thanks for all the input.

kamahen commented 1 month ago

It seems a bit strange that MSVC requires C++-20 for packages/cpp but Linux requires C++-17 (and C++-17 for packages/swipl-win'). If thecpp` tests didn't pass with C++-17 on Windows, I suggest requiring C++-20 everywhere.

(Also, both sides of the if(Qt5Widgets_FOUND) now specify C++-17, so it might make sense to change the code)

JanWielemaker commented 1 month ago

I suggest requiring C++-20 everywhere.

I'm not a fan of that. There are a lot of still maintained Linux distros that ship with gcc-9, possibly even older. I like to maintain compatibility to all maintained system unless it gets really hard.

kamahen commented 1 month ago

Presumably MSVCC didn't compile the cpp tests with C++-17, but did with C++-20? (Due to not fully implementing "guaranteed copy elision".) If so, we should probably document this and change the requirements in swipl-win also, even if not needed for that package.

JanWielemaker commented 1 month ago

Presumably MSVCC didn't compile the cpp tests with C++-17, but did with C++-20? (Due to not fully implementing "guaranteed copy elision".)

packages/cpp/CMakeLists.txt sets this to 20. packages/swipl-win is not used on Windows, so the distribution is fine for VS2020 (tested when releasing 9.2.6). Many of the C++ packages won't build with MSVC anyway due to the use of make and many other non-Windows assumptions. You can probably build them using MSYS2 (and some work).