[WIP] Warnings

[jodavies]

I'm trying to clean up the compilation warnings. Remaining on my system, is a uninitialized and a keyword-macro (with clang 14).

[jodavies]

The uninitialized warning comes from the d_u_m_m_y declared here: https://github.com/vermaseren/form/blob/b82093350ae986ad6c65a9ad25bf748efa53d7be/sources/optimize.cc#L132

This warning can be removed by initializing it in the constructor:

for (unsigned i=0; i < sizeof(d_u_m_m_y); i++) {
   d_u_m_m_y[i] = 0;
}

or, by just removing the padding. At least on my system (gcc 11.4.0) sizeof(tree_node) is 48 bytes regardless.

Which do you prefer?

[coveralls]

coverage: 48.796% (+0.04%) from 48.759% when pulling d54f5e05be9b0e879b012fd7cbda71bd428d97a8 on jodavies:warnings into e58ebd3f344ad3a6e3ae5393be3f20d17a79192f on vermaseren:master.

[tueda]

You can define copy/move constructors/assignment operators that skip copying the padding:

    tree_node(const tree_node& other):
        childs(other.childs),
        sum_results(other.sum_results),
        num_visits(other.num_visits),
        var(other.var),
        finished(other.finished) {}

    tree_node(const tree_node&& other) noexcept :
        childs(std::move(other.childs)),
        sum_results(other.sum_results),
        num_visits(other.num_visits),
        var(other.var),
        finished(other.finished) {}

    tree_node& operator=(const tree_node& other) {
        if (this != &other) {
            childs = other.childs;
            sum_results = other.sum_results;
            num_visits = other.num_visits;
            var = other.var;
            finished = other.finished;
        }
        return *this;
    }

    tree_node& operator=(tree_node&& other) noexcept {
        if (this != &other) {
            childs = std::move(other.childs);
            sum_results = other.sum_results;
            num_visits = other.num_visits;
            var = other.var;
            finished = other.finished;
        }
    }

[jodavies]

Ah, right, now I understand where the warning comes from. You need a return *this; in the second assignment operator. I'll paste these in.

On my system at least, the build is now clean, except the still-disabled stringop-overflow. That one is entirely due to the NCOPY macro, but it is used in many different places. On first inspection, it doesn't look easy to build some kind of bounds check into the macro...

[jodavies]

I think we don't want to use -Werror, unless perhaps it can be enabled for 64bit builds only. It looks like the 32bit build throws up a lot of problems due to type conflicts.

(I wonder, if FORM 5 could actually be the time to drop 32bit support? #422 #426 )

The current fix for 12ce0a5 fails on macOS because it needs to be built specifying c++11. We could do that, or change the fix.

[tueda]

For the "C++11 extension" errors, we can test if the compiler accepts -std=gnu++11 by AX_CHECK_COMPILE_FLAG and use it if OK. Maybe -std=gnu++14 or higher could generate faster machine code.

[jodavies]

That's fine as long as macOS will always support those flags. I don't have a mac to test this.

[tueda]

-Werror is not good because we can't guarantee that all future compilers won't give any warnings. Actually, GCC 12.3.0 doesn't work:

docker run -it --rm ubuntu:22.04

apt update
apt-get install -y automake g++-12 git libgmp-dev libmpfr-dev make ruby zlib1g-dev
git clone https://github.com/jodavies/form.git
cd form
git checkout 56f5146d7d43e3d9dc8faddcae42621639274e2e
autoreconf -if
CC=gcc-12 CXX=g++-12 ./configure
make

g++-12 -DHAVE_CONFIG_H -I. -I..    -Wall -Wextra -pedantic -Werror -Wno-stringop-overflow -O3 -fomit-frame-pointer -march=native  -MT form-polyfact.o -MD -MP -MF .deps/form-polyfact.Tpo -c -o form-polyfact.o `test -f 'polyfact.cc' || echo './'`polyfact.cc
In file included from /usr/include/x86_64-linux-gnu/c++/12/bits/c++allocator.h:33,
                 from /usr/include/c++/12/bits/allocator.h:46,
                 from /usr/include/c++/12/string:41,
                 from poly.h:33,
                 from polyfact.cc:35:
In member function 'void std::__new_allocator<_Tp>::deallocate(_Tp*, size_type) [with _Tp = int]',
    inlined from 'static void std::allocator_traits<std::allocator<_CharT> >::deallocate(allocator_type&, pointer, size_type) [with _Tp = int]' at /usr/include/c++/12/bits/alloc_traits.h:496:23,
    inlined from 'void std::_Vector_base<_Tp, _Alloc>::_M_deallocate(pointer, std::size_t) [with _Tp = int; _Alloc = std::allocator<int>]' at /usr/include/c++/12/bits/stl_vector.h:387:19,
    inlined from 'std::_Vector_base<_Tp, _Alloc>::~_Vector_base() [with _Tp = int; _Alloc = std::allocator<int>]' at /usr/include/c++/12/bits/stl_vector.h:366:15,
    inlined from 'std::vector<_Tp, _Alloc>::~vector() [with _Tp = int; _Alloc = std::allocator<int>]' at /usr/include/c++/12/bits/stl_vector.h:733:7,
    inlined from 'const std::vector<poly> polyfact::factorize_squarefree(const poly&, const std::vector<int>&)' at polyfact.cc:1443:15:
/usr/include/c++/12/bits/new_allocator.h:158:33: error: 'void operator delete(void*, std::size_t)' called on pointer '<unknown>' with nonzero offset [1, 9223372036854775804] [-Werror=free-nonheap-object]
  158 |         _GLIBCXX_OPERATOR_DELETE(_GLIBCXX_SIZED_DEALLOC(__p, __n));
      |                                 ^
In member function '_Tp* std::__new_allocator<_Tp>::allocate(size_type, const void*) [with _Tp = int]',
    inlined from 'static _Tp* std::allocator_traits<std::allocator<_CharT> >::allocate(allocator_type&, size_type) [with _Tp = int]' at /usr/include/c++/12/bits/alloc_traits.h:464:28,
    inlined from 'std::_Vector_base<_Tp, _Alloc>::pointer std::_Vector_base<_Tp, _Alloc>::_M_allocate(std::size_t) [with _Tp = int; _Alloc = std::allocator<int>]' at /usr/include/c++/12/bits/stl_vector.h:378:33,
    inlined from 'void std::_Vector_base<_Tp, _Alloc>::_M_create_storage(std::size_t) [with _Tp = int; _Alloc = std::allocator<int>]' at /usr/include/c++/12/bits/stl_vector.h:395:44,
    inlined from 'std::_Vector_base<_Tp, _Alloc>::_Vector_base(std::size_t, const allocator_type&) [with _Tp = int; _Alloc = std::allocator<int>]' at /usr/include/c++/12/bits/stl_vector.h:332:26,
    inlined from 'std::vector<_Tp, _Alloc>::vector(size_type, const value_type&, const allocator_type&) [with _Tp = int; _Alloc = std::allocator<int>]' at /usr/include/c++/12/bits/stl_vector.h:566:47,
    inlined from 'const std::vector<poly> polyfact::factorize_squarefree(const poly&, const std::vector<int>&)' at polyfact.cc:1443:15:
/usr/include/c++/12/bits/new_allocator.h:137:55: note: returned from 'void* operator new(std::size_t)'
  137 |         return static_cast<_Tp*>(_GLIBCXX_OPERATOR_NEW(__n * sizeof(_Tp)));
      |                                                       ^
cc1plus: all warnings being treated as errors
make[2]: *** [Makefile:5553: form-polyfact.o] Error 1

[jodavies]

Yes, I will remove that again and clean up this patch series. For https://github.com/vermaseren/form/pull/480/commits/33f70213428ab8c06129fa0917b41b3c32e343b4, should we remove c++11 features, or add it to the build?

[tueda]

Perhaps, we may need a consensus on the minimal C/C++ requirements, like C99/C++11. The workshop would be a good opportunity for it.

Even if we do not exclude C++98/C++03, we may use the #if preprocessor directive to check the value of __cplusplus to see whether the compiler supports C++11.

[jodavies]

I cleaned this up somewhat. For now I just removed the cpp11 code, it compiles without warnings without the move constructor anyway. Hopefully macOS is happy this time...

[tueda]

Is this PR still in progress? Or is it ready to merge?

[jodavies]

At the moment the build is not completely warning free, depending on which compiler or version you use. I don't think that is necessarily the goal though. Primarily I wanted to clean up things that potentially lead to bugs, such as uninitialized variables and, particularly, misleading indentation (sorting that one out took me several iterations to do correctly, in the SKIPBRA macros -- precisely the reason why it should be resolved, I suppose).

I would say this can be merged, and additional warnings can be cleaned up in the future as and when it seems necessary.

[jodavies]

@tueda : would you want these commits squashed or as they are now: one per warning?

[tueda]

I think both options are fine; keeping them as a bunch of separate commits is reasonable for this case. It is just I haven't tried to merge this PR because there are still warnings. Merging it would be OK.

One possible improvement could be: BracketInfo.dummy is a dummy field for padding. Maybe we can skip assigning 0 to this field in the default constructor (thought its costs would be negligible) if we define the copy constructor and the copy assignment operator so that dummy is not copied.

[jodavies]

Which compiler do you see this warning with?

[tueda]

I meant the warnings with 32-bit containers in CI. With GCC 14 on Ubuntu 64-bit, there are no warnings. (Warnings for LLP64 are compatibility bugs, so that's another story.)

[jodavies]

The 32bit build has a number of warnings due to incompatible-pointer-types: some are in the float code and can be ignored. But some are not, and in principle could be fixed.

With gcc-12 (both 32 and 64 bits) I also actually get a free-nonheap-object:

In file included from /usr/include/x86_64-linux-gnu/c++/12/bits/c++allocator.h:33,
                 from /usr/include/c++/12/bits/allocator.h:46,
                 from /usr/include/c++/12/string:41,
                 from poly.h:33,
                 from polyfact.cc:35:
In member function ‘void std::__new_allocator<_Tp>::deallocate(_Tp*, size_type) [with _Tp = int]’,
    inlined from ‘static void std::allocator_traits<std::allocator<_CharT> >::deallocate(allocator_type&, pointer, size_type) [with _Tp = int]’ at /usr/include/c++/12/bits/alloc_traits.h:496:23,
    inlined from ‘void std::_Vector_base<_Tp, _Alloc>::_M_deallocate(pointer, std::size_t) [with _Tp = int; _Alloc = std::allocator<int>]’ at /usr/include/c++/12/bits/stl_vector.h:387:19,
    inlined from ‘std::_Vector_base<_Tp, _Alloc>::~_Vector_base() [with _Tp = int; _Alloc = std::allocator<int>]’ at /usr/include/c++/12/bits/stl_vector.h:366:15,
    inlined from ‘std::vector<_Tp, _Alloc>::~vector() [with _Tp = int; _Alloc = std::allocator<int>]’ at /usr/include/c++/12/bits/stl_vector.h:733:7,
    inlined from ‘const std::vector<poly> polyfact::factorize_squarefree(const poly&, const std::vector<int>&)’ at polyfact.cc:1443:15:
/usr/include/c++/12/bits/new_allocator.h:158:33: warning: ‘void operator delete(void*, std::size_t)’ called on pointer ‘<unknown>’ with nonzero offset [1, 9223372036854775804] [-Wfree-nonheap-object]
  158 |         _GLIBCXX_OPERATOR_DELETE(_GLIBCXX_SIZED_DEALLOC(__p, __n));
      |                                 ^
In member function ‘_Tp* std::__new_allocator<_Tp>::allocate(size_type, const void*) [with _Tp = int]’,
    inlined from ‘static _Tp* std::allocator_traits<std::allocator<_CharT> >::allocate(allocator_type&, size_type) [with _Tp = int]’ at /usr/include/c++/12/bits/alloc_traits.h:464:28,
    inlined from ‘std::_Vector_base<_Tp, _Alloc>::pointer std::_Vector_base<_Tp, _Alloc>::_M_allocate(std::size_t) [with _Tp = int; _Alloc = std::allocator<int>]’ at /usr/include/c++/12/bits/stl_vector.h:378:33,
    inlined from ‘void std::_Vector_base<_Tp, _Alloc>::_M_create_storage(std::size_t) [with _Tp = int; _Alloc = std::allocator<int>]’ at /usr/include/c++/12/bits/stl_vector.h:395:44,
    inlined from ‘std::_Vector_base<_Tp, _Alloc>::_Vector_base(std::size_t, const allocator_type&) [with _Tp = int; _Alloc = std::allocator<int>]’ at /usr/include/c++/12/bits/stl_vector.h:332:26,
    inlined from ‘std::vector<_Tp, _Alloc>::vector(size_type, const value_type&, const allocator_type&) [with _Tp = int; _Alloc = std::allocator<int>]’ at /usr/include/c++/12/bits/stl_vector.h:566:47,
    inlined from ‘const std::vector<poly> polyfact::factorize_squarefree(const poly&, const std::vector<int>&)’ at polyfact.cc:1443:15:
/usr/include/c++/12/bits/new_allocator.h:137:55: note: returned from ‘void* operator new(std::size_t)’
  137 |         return static_cast<_Tp*>(_GLIBCXX_OPERATOR_NEW(__n * sizeof(_Tp)));
      |                                                       ^

I don't think the code is really doing anything wrong here, probably we can ignore this one: https://github.com/vermaseren/form/blob/83e3d4185efca2e5938c665a6df9d67d6d9492ca/sources/polyfact.cc#L1442-L1443