fizzy icon indicating copy to clipboard operation
fizzy copied to clipboard

ci: Add Github Action for codeql

Open axic opened this issue 4 years ago • 9 comments

axic avatar Sep 08 '20 23:09 axic

Codecov Report

Merging #524 (7b700be) into master (1fa057b) will decrease coverage by 0.13%. The diff coverage is n/a.

:exclamation: Current head 7b700be differs from pull request most recent head 7f19e0e. Consider uploading reports for the commit 7f19e0e to get more accurate results

@@            Coverage Diff             @@
##           master     #524      +/-   ##
==========================================
- Coverage   99.26%   99.12%   -0.14%     
==========================================
  Files          88       85       -3     
  Lines       13268    13010     -258     
==========================================
- Hits        13170    12896     -274     
- Misses         98      114      +16     
Flag Coverage Δ
rust 99.90% <0.00%> (+1.42%) :arrow_up:
spectests 89.98% <0.00%> (?)
unittests 98.94% <0.00%> (?)
unittests-32 99.03% <0.00%> (-0.29%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
tools/wasi/wasi.cpp 60.18% <0.00%> (-32.41%) :arrow_down:
lib/fizzy/capi.cpp 95.41% <0.00%> (-0.28%) :arrow_down:
lib/fizzy/execute.cpp 99.29% <0.00%> (-0.02%) :arrow_down:
lib/fizzy/execute.hpp 100.00% <0.00%> (ø)
lib/fizzy/instructions.cpp 100.00% <0.00%> (ø)
lib/fizzy/execution_context.hpp 100.00% <0.00%> (ø)
test/unittests/execute_test.cpp 100.00% <0.00%> (ø)
test/unittests/instantiate_test.cpp 100.00% <0.00%> (ø)
test/unittests/capi_execute_test.cpp 100.00% <0.00%> (ø)
test/unittests/floating_point_utils_test.cpp 100.00% <0.00%> (ø)
... and 8 more

codecov[bot] avatar Oct 07 '20 11:10 codecov[bot]

codeql is failing, it seems to be using gcc 7.5.0:

Scanning dependencies of target fizzy
[  1%] Building CXX object lib/fizzy/CMakeFiles/fizzy.dir/asserts.cpp.o
[  2%] Building CXX object lib/fizzy/CMakeFiles/fizzy.dir/capi.cpp.o
In file included from /usr/include/c++/7/vector:62:0,
                 from /home/runner/work/fizzy/fizzy/lib/fizzy/types.hpp:12,
                 from /home/runner/work/fizzy/fizzy/lib/fizzy/module.hpp:7,
                 from /home/runner/work/fizzy/fizzy/lib/fizzy/instantiate.hpp:10,
                 from /home/runner/work/fizzy/fizzy/lib/fizzy/execute.hpp:9,
                 from /home/runner/work/fizzy/fizzy/lib/fizzy/capi.cpp:6:
/usr/include/c++/7/bits/stl_construct.h: In instantiation of ‘void std::_Construct(_T1*, _Args&& ...) [with _T1 = fizzy::Import; _Args = {const fizzy::Import&}]’:
/usr/include/c++/7/bits/stl_uninitialized.h:83:18:   required from ‘static _ForwardIterator std::__uninitialized_copy<_TrivialValueTypes>::__uninit_copy(_InputIterator, _InputIterator, _ForwardIterator) [with _InputIterator = __gnu_cxx::__normal_iterator<const fizzy::Import*, std::vector<fizzy::Import> >; _ForwardIterator = fizzy::Import*; bool _TrivialValueTypes = false]’
/usr/include/c++/7/bits/stl_uninitialized.h:134:15:   required from ‘_ForwardIterator std::uninitialized_copy(_InputIterator, _InputIterator, _ForwardIterator) [with _InputIterator = __gnu_cxx::__normal_iterator<const fizzy::Import*, std::vector<fizzy::Import> >; _ForwardIterator = fizzy::Import*]’
/usr/include/c++/7/bits/stl_uninitialized.h:289:37:   required from ‘_ForwardIterator std::__uninitialized_copy_a(_InputIterator, _InputIterator, _ForwardIterator, std::allocator<_Tp>&) [with _InputIterator = __gnu_cxx::__normal_iterator<const fizzy::Import*, std::vector<fizzy::Import> >; _ForwardIterator = fizzy::Import*; _Tp = fizzy::Import]’
/usr/include/c++/7/bits/stl_vector.h:331:31:   required from ‘std::vector<_Tp, _Alloc>::vector(const std::vector<_Tp, _Alloc>&) [with _Tp = fizzy::Import; _Alloc = std::allocator<fizzy::Import>]’
/home/runner/work/fizzy/fizzy/lib/fizzy/module.hpp:14:8:   required from ‘typename std::_MakeUniq<_Tp>::__single_object std::make_unique(_Args&& ...) [with _Tp = fizzy::Module; _Args = {const fizzy::Module&}; typename std::_MakeUniq<_Tp>::__single_object = std::unique_ptr<fizzy::Module, std::default_delete<fizzy::Module> >]’
/home/runner/work/fizzy/fizzy/lib/fizzy/capi.cpp:359:69:   required from here
/usr/include/c++/7/bits/stl_construct.h:75:7: error: use of deleted function ‘fizzy::Import::Import(const fizzy::Import&)’
     { ::new(static_cast<void*>(__p)) _T1(std::forward<_Args>(__args)...); }
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/runner/work/fizzy/fizzy/lib/fizzy/module.hpp:7:0,
                 from /home/runner/work/fizzy/fizzy/lib/fizzy/instantiate.hpp:10,
                 from /home/runner/work/fizzy/fizzy/lib/fizzy/execute.hpp:9,
                 from /home/runner/work/fizzy/fizzy/lib/fizzy/capi.cpp:6:
/home/runner/work/fizzy/fizzy/lib/fizzy/types.hpp:325:8: note: ‘fizzy::Import::Import(const fizzy::Import&)’ is implicitly deleted because the default definition would be ill-formed:
 struct Import
        ^~~~~~
/home/runner/work/fizzy/fizzy/lib/fizzy/types.hpp:325:8: error: use of deleted function ‘fizzy::Import::<unnamed union>::<constructor>(const fizzy::Import::<unnamed union>&)’
/home/runner/work/fizzy/fizzy/lib/fizzy/types.hpp:331:5: note: ‘fizzy::Import::<unnamed union>::<constructor>(const fizzy::Import::<unnamed union>&)’ is implicitly deleted because the default definition would be ill-formed:
     {
     ^
/home/runner/work/fizzy/fizzy/lib/fizzy/types.hpp:333:16: error: union member ‘fizzy::Import::<unnamed union>::memory’ with non-trivial ‘constexpr fizzy::Memory::Memory(const fizzy::Memory&)’
         Memory memory;
                ^~~~~~
/home/runner/work/fizzy/fizzy/lib/fizzy/types.hpp:335:15: error: union member ‘fizzy::Import::<unnamed union>::table’ with non-trivial ‘constexpr fizzy::Table::Table(const fizzy::Table&)’
         Table table;
               ^~~~~
cc1plus: error: unrecognized command line option ‘-Wno-unknown-attributes’ [-Werror]
cc1plus: all warnings being treated as errors
make[2]: *** [lib/fizzy/CMakeFiles/fizzy.dir/capi.cpp.o] Error 1
make[1]: *** [lib/fizzy/CMakeFiles/fizzy.dir/all] Error 2
lib/fizzy/CMakeFiles/fizzy.dir/build.make:94: recipe for target 'lib/fizzy/CMakeFiles/fizzy.dir/capi.cpp.o' failed
CMakeFiles/Makefile2:441: recipe for target 'lib/fizzy/CMakeFiles/fizzy.dir/all' failed
make: *** [all] Error 2
Makefile:159: recipe for target 'all' failed
Error: Process completed with exit code 2.

@gumb0 @chfast do you think we can easily fix this? If it is messy, we should just mark old gcc unsupported (should also have the minimum compiler versions set in cmake).

axic avatar Jan 27 '21 00:01 axic

@gumb0 @chfast do you think we can easily fix this? If it is messy, we should just mark old gcc unsupported (should also have the minimum compiler versions set in cmake).

Looks to be a limitation of std::optional in old GCC:

  • union is not allowed to have non-trivially constructed members
  • we have Import containing union containing Memory and Table that have Limits containing std::optional<uint32_t> max;
  • std::optional was (retroactively) prescribed in 2018 to have trivial constructors in case value it contains is trivially-constructed image http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0602r4.html
  • GCC implemented this fix in version 8.3 https://gcc.gnu.org/onlinedocs/gcc-10.2.0/libstdc++/manual/manual/status.html image

I don't see any easy fix for this other than replacing std::optional with something else like bool+uint32_t, maybe it's not worth it.

gumb0 avatar Jan 28 '21 10:01 gumb0

I'd vote for requiring gcc 8.3 as the minimum.

axic avatar Jan 28 '21 10:01 axic

  • union is not allowed to have non-trivially constructed members

Actually this was true only till C++11, now we can define our own union constructors... I'll try something.

gumb0 avatar Jan 28 '21 10:01 gumb0

@gumb0 @chfast do we want to specify minimum required compiler versions in cmake/readme? It seems gcc >=8.3 is required unless we want to merge this commit from @gumb0.

axic avatar Mar 29 '21 22:03 axic

@gumb0 @chfast do we want to specify minimum required compiler versions in cmake/readme? It seems gcc >=8.3 is required unless we want to merge this commit from @gumb0.

No.

chfast avatar Mar 30 '21 07:03 chfast

Why not? This PR showed that fizzy was not compiling with gcc<8.3.

axic avatar Mar 31 '21 21:03 axic

Why not? This PR showed that fizzy was not compiling with gcc<8.3.

If you want to do this, you need to create CI jobs with the discovered compiler version. I don't think this is worth the time right now.

chfast avatar Mar 31 '21 21:03 chfast