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

error: static_assert failed due to requirement '__is_cpp17_move_insertable<std::__1::allocator<FunctionParserBase<double>>, false>::value' "The specified type does not meet the requirements of Cpp17MoveInsertable" #2710

Closed
yurivict opened this issue Sep 21, 2020 · 27 comments

Comments

@yurivict
Copy link

clang-11 fails to compile libmesh-1.4.1 on FreeBSD:

In file included from src/apps/calculator.C:24:
In file included from /usr/include/c++/v1/map:481:
In file included from /usr/include/c++/v1/__tree:15:
/usr/include/c++/v1/memory:1634:13: error: static_assert failed due to requirement '__is_cpp17_move_insertable<std::__1::allocator<FunctionParserBase<double>>, false>::value' "The specified type does not meet the requirements of Cpp17MoveInsertable"
            static_assert(__is_cpp17_move_insertable<allocator_type>::value,
            ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/c++/v1/vector:954:21: note: in instantiation of function template specialization 'std::__1::allocator_traits<std::__1::allocator<FunctionParserBase<double>>>::__construct_backward_with_exception_guarantees<FunctionParserBase<double> *>' requested here
    __alloc_traits::__construct_backward_with_exception_guarantees(
                    ^
/usr/include/c++/v1/vector:1631:5: note: in instantiation of member function 'std::__1::vector<FunctionParserBase<double>, std::__1::allocator<FunctionParserBase<double>>>::__swap_out_circular_buffer' requested here
    __swap_out_circular_buffer(__v);
    ^
/usr/include/c++/v1/vector:1644:9: note: in instantiation of function template specialization 'std::__1::vector<FunctionParserBase<double>, std::__1::allocator<FunctionParserBase<double>>>::__push_back_slow_path<const FunctionParserBase<double> &>' requested here
        __push_back_slow_path(__x);
        ^
./include/libmesh/parsed_fem_function.h:632:15: note: in instantiation of member function 'std::__1::vector<FunctionParserBase<double>, std::__1::allocator<FunctionParserBase<double>>>::push_back' requested here
      parsers.push_back(fp);
              ^
./include/libmesh/parsed_fem_function.h:349:9: note: in instantiation of member function 'libMesh::ParsedFEMFunction<double>::partial_reparse' requested here
  this->partial_reparse(expression);
        ^
./include/libmesh/parsed_fem_function.h:219:9: note: in instantiation of member function 'libMesh::ParsedFEMFunction<double>::reparse' requested here
  this->reparse(expression);
        ^
src/apps/calculator.C:157:29: note: in instantiation of member function 'libMesh::ParsedFEMFunction<double>::ParsedFEMFunction' requested here
  ParsedFEMFunction<Number> goal_function(old_sys, calcfunc);
                            ^
4 warnings and 1 error generated.

Log: http://beefy18.nyi.freebsd.org/data/head-amd64-default/p549079_s365919/logs/errors/libmesh-1.4.1_3.log (IPv6 URL)

@jwpeterson
Copy link
Member

From a brief analysis, the line which triggers the error:

parsers.push_back(fp);

where

FunctionParserBase<Output> fp;

We have explicitly deleted the move assignment operator and move constructor for this class (see: contrib/fparser/fparser.hh)

    // This class manages its own memory via reference counting, so the
    // compiler-generated move constructor and move assignment operator
    // are not safe to use. By explicitly deleting them we can prevent
    // users from assuming this class can be moved when it really can't.
    FunctionParserBase (FunctionParserBase &&) = delete;
    FunctionParserBase & operator= (FunctionParserBase &&) = delete;

so it does make sense to me that it would fail to meet the requirements of "Cpp17MoveInsertable", however what's not clear to me is why that's an issue... we are just calling push_back() so the compiler should just make a copy IMO.

I would be happy to patch 1.4.1 if a satisfactory resolution to this issue can be found, but I don't have ready access to either clang-11 or FreeBSD so I'm afraid I won't be able to do much testing myself.

@yurivict
Copy link
Author

I would be happy to patch 1.4.1 if a satisfactory resolution to this issue can be found, but I don't have ready access to either clang-11 or FreeBSD so I'm afraid I won't be able to do much testing myself.

Please install clang-11 on the OS of your choice and you should be able to reproduce this problem.

@roystgnr
Copy link
Member

My apologies, but I vaguely recall getting a second-hand report of running into this problem months ago, and I hastily decided the solution must be "go find a better compiler" since obviously push_back() does a copy construction not a move construction ... but looking into it more deeply today it seems like the new clang++ behavior here is still standards-compliant.

I think the most broadly-compatible easy fix is for us to turn that parsers vector into a vector of unique_ptr ... but I also don't have a system handy with more than clang-10 in the repos, and my last few times trying to build clang++ from scratch did not go well, so I'll have no good way to test the patch.

@jwpeterson
Copy link
Member

jwpeterson commented Sep 24, 2020

Out of curiosity, I made a list of classes in libmesh with deleted move constructors. As I understand it, these classes do not satisfy the "MoveInsertable" requirements and therefore cannot be inserted into a std::vector with push_back():

git grep "= delete" | grep "&&" | grep -v operator= | less

It came back with the following classes where the move constructor is deleted because it is not (easily) possible to default:

  • FunctionParserBase
  • FunctionParserADBase
  • All Elem-derived classes
  • LaspackMatrix, LaspackVector
  • PetscMatrix, PetscVector
  • EpetraMatrix, EpetraVector

And these in which the move constructor could probably be defaulted with a small amount of work:

  • RBEIMConstruction (RBConstructionBase move ctor could possibly be implemented)
  • MeshFunction (_point_locator could become std::unique_ptr)

So, I think we are for the most part OK, since those in the first list are not typically stored by value in vectors (although we store vectors of Elem pointers quite often). I may take a crack at fixing the last two at some point.

jwpeterson added a commit to jwpeterson/libmesh that referenced this issue Sep 24, 2020
The main reason behind this change is to make the class default
move-constructible, which should theoretically allow it to be stored
by value in a std::vector via push_back().

Refs libMesh#2710
jwpeterson added a commit to jwpeterson/libmesh that referenced this issue Sep 24, 2020
* RBConstructionBase
* RBParametrized
* RBEIMConstruction

In general we should not call clear() in any class destructor unless
we need to manually clean up memory managed by that class. Instead, we
should just let the member destructors do their thing.

Refs libMesh#2710
jwpeterson added a commit to jwpeterson/libmesh that referenced this issue Sep 29, 2020
This is an attempt to fix the issue raised in libMesh#2710. Since
FunctionParserBase has a deleted move constructor, it is not
considered to be MoveInsertable, which means it cannot be passed to
std::vector::push_back() by value.
jwpeterson added a commit to jwpeterson/libmesh that referenced this issue Sep 29, 2020
This is an attempt to fix the issue raised in libMesh#2710. Since
FunctionParserBase has a deleted move constructor, it is not
considered to be MoveInsertable, which means it cannot be passed to
std::vector::push_back() by value.

Adding a vector<unique_ptr> to the class means that it can no longer
be default copy constructed. We had a unit test of this but, no actual
uses within the library so hopefully it's OK to drop it.
@yurivict
Copy link
Author

yurivict commented Oct 3, 2020

1.6.0-rc1 release asserts with all clang versions: 8,9, 10, 11.

@roystgnr
Copy link
Member

roystgnr commented Oct 3, 2020

8 through 10 too? Now that I might be able to replicate; thanks!

@yurivict
Copy link
Author

yurivict commented Oct 3, 2020

Yes, 1.6.0-rc1 fails with more compilers.

@jwpeterson
Copy link
Member

8 through 10 too? Now that I might be able to replicate; thanks!

I don't see how this can be possible? MOOSE guys have been using these compilers, at least versions 8 and 9, and have not run into any issues as far as I know. Note also that the potential fix merged in #2720 is not in 1.6.0-rc1 yet. I felt we should wait to see if it would work before backporting it to any releases.

@yurivict
Copy link
Author

yurivict commented Oct 5, 2020

1.6.0-rc1 fails too.

@jwpeterson
Copy link
Member

@yurivict Can you let us know how libmesh is configured when it hits the static_assert? Maybe it's that you are using -std=c++17 (or -std=c++17 is the default for your compiler), which is something that we don't really test. By the way, I tried to click on the link in your original post but it doesn't work for me...

@yurivict
Copy link
Author

yurivict commented Oct 5, 2020

The log linked above has all command lines.

@yurivict
Copy link
Author

yurivict commented Oct 5, 2020

It might be that the compiler defaults to -std=c++17. You need to set your level which you don't currently do,

@yurivict
Copy link
Author

yurivict commented Oct 5, 2020

I tried to click on the link in your original post but it doesn't work for me...

It's an IPv6 URL.

@yurivict
Copy link
Author

yurivict commented Oct 5, 2020

Setting -std=c++11 doesn't solve the problem in 1.6.0-rc1 with clang-10.

@yurivict
Copy link
Author

yurivict commented Oct 5, 2020

In include/libmesh/fparser_ad.hh you have deleted move-constructors:

  FunctionParserADBase (FunctionParserADBase &&) = delete;
  FunctionParserADBase & operator= (const FunctionParserADBase &) = delete;
  FunctionParserADBase & operator= (FunctionParserADBase &&) = delete;

This directly causes the failure at include/libmesh/parsed_function.h:561 - you can't use such type for vector<>::push_back().

Gcc doesn't catch basic modern C++ bugs any more.

Please either remove the delete statements, or the push_back statement.

@jwpeterson
Copy link
Member

Yes, I understand the problem and we have already patched it in master... if you can try master on your system that would be very helpful.

It's an IPv6 URL.

OK... well I click the link in my browser (Chrome) and it doesn't work. Is there something special I need to do to view IPv6 links?

@yurivict
Copy link
Author

yurivict commented Oct 5, 2020

OK... well I click the link in my browser (Chrome) and it doesn't work. Is there something special I need to do to view IPv6 links?

You obviously don't have IPv6. When I raised this issue with the people who support the servers they said that about everybody is supposed to have IPv6 addresses because they are the future.

Personally, I view these links through the Tor network. Tor exit nodes usually are IPv6-enabled.

@yurivict
Copy link
Author

yurivict commented Oct 5, 2020

Yes, I understand the problem and we have already patched it in master... if you can try master on your system that would be very helpful.

I tried the latest revision 2873a0e and it still fails the same way.

@yurivict
Copy link
Author

yurivict commented Oct 5, 2020

gcc-9 also fails to build rc1:

tecsrc/arrlist.cpp: In function 'Boolean_t ArrayListIsValid(ArrayList_pa)':
tecsrc/arrlist.cpp:390:26: error: 'VALID_REF' was not declared in this scope; did you mean 'INVALID_REF'?
  390 |     Boolean_t IsValid = (VALID_REF(ArrayList) &&
      |                          ^~~~~~~~~
      |                          INVALID_REF

@jwpeterson
Copy link
Member

I tried the latest revision 2873a0e and it still fails the same way.

The code is different now (the vector stores unique_ptrs instead of objects) so while I believe it may still not be quite right, I don't think the error can be the same as it was before.

tecsrc/arrlist.cpp: In function 'Boolean_t ArrayListIsValid(ArrayList_pa)':

This is an error in the Tecplot contrib code that hasn't been updated in ages, probably best to just configure with --disable-tecplot on your system and not worry about it for the moment.

@yurivict
Copy link
Author

yurivict commented Oct 5, 2020

rev. 2873a0e fails like this:

/bin/sh ./libtool  --tag=CXX   --mode=compile c++ -DHAVE_CONFIG_H -I. -I./include  -DNDEBUG  -DLIBMESH_IS_COMPILING_ITSELF -I./contrib/metaphysicl/src/numerics/include -I./contrib/metaphysicl/src/core/include -I./contrib/metaphysicl/src/utilities/include -I./contrib/nanoflann/include -I./contrib/fparser -I./contrib/libHilbert/include -I./contrib/gmv -I./contrib/qhull/qhull/src -I./contrib/qhull/qhull/src/libqhullcpp -I./contrib/triangle -I./contrib/tetgen -I./contrib/metis/include -I./contrib/gzstream -I./contrib/sfcurves -I./contrib/laspack -I./contrib/timpi/src/utilities/include -I./contrib/timpi/src/utilities/include -I./contrib/timpi/src/parallel/include -I./contrib/timpi/src/algorithms/include  -I/usr/local/include/eigen3 -D_THREAD_SAFE -pthread -I/usr/local/include -I./include  -fno-omit-frame-pointer -O2 -felide-constructors -Qunused-arguments -Wunused-parameter -Wunused    -fopenmp   -O2 -pipe -fno-omit-frame-pointer -fstack-protector-strong -fno-strict-aliasing -fno-omit-frame-pointer   -MT src/base/libmesh_opt_la-dof_map.lo -MD -MP -MF src/base/.deps/libmesh_opt_la-dof_map.Tpo -c -o src/base/libmesh_opt_la-dof_map.lo `test -f 'src/base/dof_map.C' || echo './'`src/base/dof_map.C
libtool: compile:  c++ -DHAVE_CONFIG_H -I. -I./include -DNDEBUG -DLIBMESH_IS_COMPILING_ITSELF -I./contrib/metaphysicl/src/numerics/include -I./contrib/metaphysicl/src/core/include -I./contrib/metaphysicl/src/utilities/include -I./contrib/nanoflann/include -I./contrib/fparser -I./contrib/libHilbert/include -I./contrib/gmv -I./contrib/qhull/qhull/src -I./contrib/qhull/qhull/src/libqhullcpp -I./contrib/triangle -I./contrib/tetgen -I./contrib/metis/include -I./contrib/gzstream -I./contrib/sfcurves -I./contrib/laspack -I./contrib/timpi/src/utilities/include -I./contrib/timpi/src/utilities/include -I./contrib/timpi/src/parallel/include -I./contrib/timpi/src/algorithms/include -I/usr/local/include/eigen3 -D_THREAD_SAFE -pthread -I/usr/local/include -I./include -fno-omit-frame-pointer -O2 -felide-constructors -Qunused-arguments -Wunused-parameter -Wunused -fopenmp -O2 -pipe -fno-omit-frame-pointer -fstack-protector-strong -fno-strict-aliasing -fno-omit-frame-pointer -MT src/base/libmesh_opt_la-dof_map.lo -MD -MP -MF src/base/.deps/libmesh_opt_la-dof_map.Tpo -c src/base/dof_map.C  -fPIC -DPIC -o src/base/.libs/libmesh_opt_la-dof_map.o
In file included from src/apps/fparser_parse.C:23:
In file included from ./include/libmesh/parsed_function.h:22:
In file included from ./include/libmesh/function_base.h:24:
In file included from ./include/libmesh/libmesh_common.h:46:
In file included from /usr/include/c++/v1/complex:246:
In file included from /usr/include/c++/v1/sstream:173:
In file included from /usr/include/c++/v1/ostream:138:
In file included from /usr/include/c++/v1/ios:215:
In file included from /usr/include/c++/v1/__locale:14:
In file included from /usr/include/c++/v1/string:504:
In file included from /usr/include/c++/v1/string_view:175:
In file included from /usr/include/c++/v1/__string:57:
In file included from /usr/include/c++/v1/algorithm:643:
/usr/include/c++/v1/memory:1720:13: error: static_assert failed due to requirement '__is_cpp17_move_insertable<std::__1::allocator<FunctionParserADBase<double> >, false>::value' "The
      specified type does not meet the requirements of Cpp17MoveInsertable"
            static_assert(__is_cpp17_move_insertable<allocator_type>::value,
            ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/c++/v1/vector:952:21: note: in instantiation of function template specialization 'std::__1::allocator_traits<std::__1::allocator<FunctionParserADBase<double> >
      >::__construct_backward_with_exception_guarantees<FunctionParserADBase<double> *>' requested here
    __alloc_traits::__construct_backward_with_exception_guarantees(
                    ^
/usr/include/c++/v1/vector:1627:5: note: in instantiation of member function 'std::__1::vector<FunctionParserADBase<double>, std::__1::allocator<FunctionParserADBase<double> >
      >::__swap_out_circular_buffer' requested here
    __swap_out_circular_buffer(__v);
    ^
/usr/include/c++/v1/vector:1640:9: note: in instantiation of function template specialization 'std::__1::vector<FunctionParserADBase<double>, std::__1::allocator<FunctionParserADBase<double>
      > >::__push_back_slow_path<const FunctionParserADBase<double> &>' requested here
        __push_back_slow_path(__x);
        ^
./include/libmesh/parsed_function.h:561:15: note: in instantiation of member function 'std::__1::vector<FunctionParserADBase<double>, std::__1::allocator<FunctionParserADBase<double> >
      >::push_back' requested here
      parsers.push_back(fp);
              ^
./include/libmesh/parsed_function.h:254:9: note: in instantiation of member function 'libMesh::ParsedFunction<double, libMesh::VectorValue<double> >::partial_reparse' requested here
  this->partial_reparse(expression);
        ^
./include/libmesh/parsed_function.h:223:9: note: in instantiation of member function 'libMesh::ParsedFunction<double, libMesh::VectorValue<double> >::reparse' requested here
  this->reparse(expression);
        ^
src/apps/fparser_parse.C:36:20: note: in instantiation of member function 'libMesh::ParsedFunction<double, libMesh::VectorValue<double> >::ParsedFunction' requested here
  ParsedFunction<> func(function_string);
                   ^
1 error generated.
gmake[3]: *** [Makefile:31329: src/apps/fparser_parse_opt-fparser_parse.o] Error 1
gmake[3]: *** Waiting for unfinished jobs....

@jwpeterson
Copy link
Member

OK, I see, the issue is that we had the same type of code in both parsed_function.h and parsed_fem_function.h, and the fix in #2720 was only for the latter. I will go ahead and push the same fix for that one.

jwpeterson added a commit to jwpeterson/libmesh that referenced this issue Oct 5, 2020
This is similar to the change made in 783091b for
ParsedFEMFunction. For a class to be pushed-back onto a vector, it
must be MoveInsertable, which FunctionParserADBase is not.

Refs libMesh#2710
@jwpeterson
Copy link
Member

@yurivict when you have some time, can you please try compiling master again? As of 746b489, we've updated it with a fix for the most recent issue.

@yurivict
Copy link
Author

yurivict commented Oct 6, 2020

The problem is fixed in rev. 746b489, thank you.

@yurivict yurivict closed this as completed Oct 6, 2020
@jwpeterson
Copy link
Member

OK, great, thanks for letting us know.

@roystgnr
Copy link
Member

roystgnr commented Oct 6, 2020

Thanks for the constant feedback! I know debugging-via-chat is the most painful sort of debugging; I'm glad it worked out this time. Here's hoping that just backporting these fixes will be sufficient for the release too.

@yurivict
Copy link
Author

yurivict commented Oct 6, 2020

Thanks for fixing it!

jwpeterson added a commit that referenced this issue Oct 6, 2020
The master hashes of the commits cherry-picked and squashed to create this commit:

PR #2720
783091b

PR #2731
b67b3e4
0a1dbe4
73d58f2
3a61395
a6f14fe
00cd55d
1310eac

A similar "combined" commit should also be cherry-picked onto the
1.4.x and 1.5.x series and new patch-level releases should be made for
them, since this is an issue which prevents certain compilers from
being able to compile the library.

Refs #2710
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

No branches or pull requests

3 participants