Update testsuite compilers and OSs for Jammy #5415

Noordfrees · 2022-05-12T07:16:05Z

No description provided.

…e-jammy

… g++12 wants this anyway)

…e-jammy

This reverts commit 663ec08.

Noordfrees · 2022-05-14T08:17:57Z

g++12 Debug builds work now. g++12 Release builds fail with various warnings from included system headers (std_function.h, atomic_base.h), does anyone know why this happens?

matthiakl · 2022-05-14T17:02:17Z

I do not get these errors with gcc 12.1.0 (CI uses 12.0.1). Instead I get two errors which I only could resolve by using local GCC_DIAG_OFF, but then it compiled (both debug and release):

[ 89%] Building CXX object src/logic/map_objects/CMakeFiles/logic_map_objects.dir/tribes/market.cc.o
In file included from /usr/include/c++/12.1.0/bits/shared_ptr_atomic.h:33,
                 from /usr/include/c++/12.1.0/memory:78,
                 from /mnt/storage/Programme/widelands-dev/src/logic/map_objects/tribes/market.h:22,
                 from /mnt/storage/Programme/widelands-dev/src/logic/map_objects/tribes/market.cc:19:
In member function ‘_PTp* std::__atomic_base<_PTp*>::load(std::memory_order) const [with _PTp = Widelands::Player]’,
    inlined from ‘std::__atomic_base<_PTp*>::operator __pointer_type() const [with _PTp = Widelands::Player]’ at /usr/include/c++/12.1.0/bits/atomic_base.h:694:20,
    inlined from ‘std::atomic<_Tp*>::operator __pointer_type() const [with _Tp = Widelands::Player]’ at /usr/include/c++/12.1.0/atomic:433:16,
    inlined from ‘Widelands::Player* Widelands::MapObject::get_owner() const’ at /mnt/storage/Programme/widelands-dev/src/logic/map_objects/map_object.h:252:10,
    inlined from ‘void Widelands::Market::traded_ware_arrived(int, Widelands::DescriptionIndex, Widelands::Game*)’ at /mnt/storage/Programme/widelands-dev/src/logic/map_objects/tribes/market.cc:254:41:
/usr/include/c++/12.1.0/bits/atomic_base.h:820:31: error: ‘long unsigned int __atomic_load_8(const volatile void*, int)’ writing 8 bytes into a region of size 0 overflows the destination [-Werror=stringop-overflow=]
  820 |         return __atomic_load_n(&_M_p, int(__m));
      |                ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors

and

[ 98%] Building CXX object src/economy/CMakeFiles/economy.dir/idleworkersupply.cc.o
In file included from /mnt/storage/Programme/widelands-dev/src/economy/supply.h:22,
                 from /mnt/storage/Programme/widelands-dev/src/economy/economy.h:26,
                 from /mnt/storage/Programme/widelands-dev/src/economy/economy.cc:19:
In member function ‘void Trackable::Tracker::deref()’,
    inlined from ‘BaseTrackPtr::~BaseTrackPtr()’ at /mnt/storage/Programme/widelands-dev/src/economy/trackptr.h:110:19,
    inlined from ‘TrackPtr<Widelands::Supply>::~TrackPtr()’ at /mnt/storage/Programme/widelands-dev/src/economy/trackptr.h:164:27,
    inlined from ‘Widelands::RequestSupplyPair::~RequestSupplyPair()’ at /mnt/storage/Programme/widelands-dev/src/economy/economy.cc:766:8,
    inlined from ‘void std::push_heap(_RAIter, _RAIter, _Compare) [with _RAIter = __gnu_cxx::__normal_iterator<Widelands::RequestSupplyPair*, vector<Widelands::RequestSupplyPair> >; _Compare = Widelands::RequestSupplyPair::Compare]’ at /usr/include/c++/12.1.0/bits/stl_heap.h:217:5,
    inlined from ‘void std::priority_queue<_Tp, _Sequence, _Compare>::push(const value_type&) [with _Tp = Widelands::RequestSupplyPair; _Sequence = std::vector<Widelands::RequestSupplyPair>; _Compare = Widelands::RequestSupplyPair::Compare]’ at /usr/include/c++/12.1.0/bits/stl_queue.h:741:16,
    inlined from ‘void Widelands::Economy::process_requests(Widelands::Game&, Widelands::RSPairStruct*)’ at /mnt/storage/Programme/widelands-dev/src/economy/economy.cc:845:27:
/mnt/storage/Programme/widelands-dev/src/economy/trackptr.h:58:27: error: pointer may be used after ‘void operator delete(void*)’ [-Werror=use-after-free]
   59 |                         --refcount_;
      |                           ^~~~~~~~~
In member function ‘void Trackable::Tracker::deref()’,
    inlined from ‘void Trackable::Tracker::deref()’ at /mnt/storage/Programme/widelands-dev/src/economy/trackptr.h:56:8,
    inlined from ‘BaseTrackPtr::~BaseTrackPtr()’ at /mnt/storage/Programme/widelands-dev/src/economy/trackptr.h:110:19,
    inlined from ‘TrackPtr<Widelands::Supply>::~TrackPtr()’ at /mnt/storage/Programme/widelands-dev/src/economy/trackptr.h:164:27,
    inlined from ‘Widelands::RequestSupplyPair::~RequestSupplyPair()’ at /mnt/storage/Programme/widelands-dev/src/economy/economy.cc:766:8,
    inlined from ‘void std::push_heap(_RAIter, _RAIter, _Compare) [with _RAIter = __gnu_cxx::__normal_iterator<Widelands::RequestSupplyPair*, vector<Widelands::RequestSupplyPair> >; _Compare = Widelands::RequestSupplyPair::Compare]’ at /usr/include/c++/12.1.0/bits/stl_heap.h:215:23,
    inlined from ‘void std::priority_queue<_Tp, _Sequence, _Compare>::push(const value_type&) [with _Tp = Widelands::RequestSupplyPair; _Sequence = std::vector<Widelands::RequestSupplyPair>; _Compare = Widelands::RequestSupplyPair::Compare]’ at /usr/include/c++/12.1.0/bits/stl_queue.h:741:16,
    inlined from ‘void Widelands::Economy::process_requests(Widelands::Game&, Widelands::RSPairStruct*)’ at /mnt/storage/Programme/widelands-dev/src/economy/economy.cc:845:27:
/mnt/storage/Programme/widelands-dev/src/economy/trackptr.h:60:40: note: call to ‘void operator delete(void*)’ here
   61 |                                 delete this;
      |                                        ^~~~

andy5995 · 2022-05-15T19:34:35Z

Using Ubuntu 22.04 and g++12.0.1 in a docker container, here's the err output. I think it matches what the CI is doing (build stops at 76%).

I'm configuring with CC=gcc-12 CXX=g++-12 cmake .. -DCMAKE_BUILD_TYPE:STRING=Release -DOPTION_BUILD_TRANSLATIONS="ON" -DOPTION_BUILD_WEBSITE_TOOLS="ON" -DOPTION_ASAN="ON" -DOPTION_BUILD_CODECHECK="OFF"

gcc12_err_out.txt

g++12 Debug builds work now. g++12 Release builds fail with various warnings from included system headers (std_function.h, atomic_base.h), does anyone know why this happens?

I don't see any references to atomic_base.h but I do for std_function.h. Seems to be most related to the warnings about using variables that may be uninitialized.

This may be a situation where using fanalyzer might be useful to help track down why the variables may be uninitialized

andy5995 · 2022-05-15T19:41:46Z

Here's the output when fanalyzer is used:

gcc12_fanalyzer_err_02.log

andy5995 · 2022-05-15T20:06:53Z

I do not get these errors with gcc 12.1.0 (CI uses 12.0.1). Instead I get two errors which I only could resolve by using local GCC_DIAG_OFF, but then it compiled (both debug and release):

When I tried on Arch in a docker container, using 12.1.0, the release build fails at 76%

arch_gcc12.1.0_01_err.txt
stdout_arch_gcc12.1.0_01.log

andy5995 · 2022-05-15T20:18:39Z

I know my knowledge of cpp is limited, but I've not a clue as to why when I initialize s to "" (on L33) in this example, the error still happens:

/wl/src/base/md5.cc: In member function ‘std::string Md5Checksum::str() const’:
/wl/src/base/md5.cc:41:16: error: use of uninitialized value ‘<unknown>’ [CWE-457] [-Werro
r=analyzer-use-of-uninitialized-value]
   41 |         return s;
      |                ^
  ‘std::string Md5Checksum::str() const’: events 1-3
    |
    |   32 | std::string Md5Checksum::str() const {
    |      |             ^~~~~~~~~~~        ~~~~~
    |      |             |                  |
    |      |             |                  (2) region created on stack here
    |      |             (1) entry to ‘Md5Checksum::str’
    |   33 |         std::string s = "";
    |      |                         ~~
    |      |                         |
    |      |                         (3) calling ‘std::__cxx11::basic_string<char>::basic_
string’ from ‘Md5Checksum::str’

andy5995 · 2022-05-15T20:49:29Z

If it makes you feel any better, I just added a test for gcc-12 on Jammy for one of my small projects. I've had the fanalyzer flag enabled for a while and it's been clean. With 12, not so much.

https://github.com/theimpossibleastronaut/rmw/runs/6443383022?check_suite_focus=true#step:3:239

andy5995 · 2022-05-16T06:11:27Z

After reviewing the new warnings for my own project, I've concluded they're false positives.

So I disabled them in theimpossibleastronaut/rmw@998a405

@Noordfrees As long as widelands is checking with clang and other versions of g++, it would probably be safe to do the same for the warnings about possible use of uninitialized values, using a condition to check if the compiler is gcc && version 12 or greater. And then try enabling the warnings again when gcc-12.2.0 is released.

andy5995 · 2022-05-16T06:18:09Z

I do not get these errors with gcc 12.1.0 (CI uses 12.0.1). Instead I get two errors which I only could resolve by using local GCC_DIAG_OFF, but then it compiled (both debug and release):

Looks like that can be done by adding a pragma to a header but in this example would affect all versions of gcc https://stackoverflow.com/questions/29320555/how-to-disable-wmaybe-uninitialized-warning

…e-jammy

andy5995

Just some minor stuff I suggested really. Seems like @Noordfrees has this all under control. LGTM!

.github/workflows/build.yaml

andy5995 · 2022-05-18T05:50:51Z

.github/scripts/install_deps.sh

+  libsdl2-ttf-dev\
+  python3\
+  zlib1g-dev \
+  ${ADD_PKG_LIST}


Suggested change

${ADD_PKG_LIST}

${ADD_PKG_LIST}

exit 0

Good practice to include an exit statement?

We don't do this in any other script, and it doesn't add any value IMHO since it's implicit

andy5995 · 2022-05-18T06:11:03Z

src/base/log.cc

@@ -177,7 +177,7 @@ void do_log(const LogType type, const Time& gametime, const char* const fmt, ...
 	assert(logger != nullptr);

 	// message type and timestamp
-	char buffer_prefix[32];
+	char buffer_prefix[256];


Suggested change

char buffer_prefix[256];

char buffer_prefix[sizeof "[24:00:00.000 xxxx] XXXXXXX:"];

If we want to change the format in the future, even for something small, then we'd need to update two places instead of one. It's just one stack-allocated array with short lifespan so this doesn't have any impact resources-wise; also the gametime can have 3 or more digits in the hour in which case this could overflow. So IMHO better to allow some extra space.

.github/workflows/build.yaml

…e-jammy

matthiakl

Looks ok for now. Let's have it, so everyone can compile again.

This compiler is known to cause false-positive warnings.

Do you know of already reported similar issues? Is this something the compiler devs are already aware of?

Noordfrees · 2022-05-21T14:52:10Z

Thanks for the review :)

The GCC C++ bug tracker lists several recent regressions around warnings as well as several reports about spurious warnings in v12 so this seems to be a known (and common) problem.

svenstaro · 2022-07-09T01:37:40Z

Can we get a new release with this? Widelands doesn't currently compile in Arch because of this and the patch doesn't apply cleanly.

matthiakl · 2022-07-09T08:04:27Z

I'm not sure what you need. The current development version does compile fine (there is also an PKGBUILD for it in the AUR). The last release (1.0) is provided as prebuilt package for many platforms and should not be needed to be compiled. If you still want to compile it, simply install and use gcc11 package.

svenstaro · 2022-07-09T11:43:30Z

Distributions don't usually use prebuilt binaries if it can at all be helped. We like to compile things from source so that we can be sure the all binaries are using the same binary hardening, for instance.

Also in Arch Linux, we don't really have a way to go back to old compilers usually. As such, I'd still appreciate a new release. Otherwise if that is not possible, I could look into building a recent commit and shipping that but I'd rather not if it can be helped.

Noordfrees · 2022-07-09T13:40:43Z

We're already in feature freeze for the v1.1 release. There are still some open PRs and issues targeted to v1.1 that need testing and review. I expect the release to happen in late summer or early autumn this year.

What exactly is the problem when building v1.0 with latest compilers? Would patching the CMakeLists.txt to remove the -Werror flag fix things for you?

svenstaro · 2022-07-09T16:14:09Z

Yeah that'd work and that's what I would have gone with anyway in case you couldn't get a release out short term. We try not to mess with upstream compile options if it can be helped. :)

Update testsuite compilers and OSs for Jammy

c64a639

Noordfrees added the testing Automated testing, unit tests, test suite. label May 12, 2022

Noordfrees added this to the v1.1 milestone May 12, 2022

Noordfrees self-assigned this May 12, 2022

Noordfrees and others added 5 commits May 12, 2022 12:54

Use actions/checkout@v3

389bc69

Merge branch 'master' into testsuite-jammy

18627bd

Merge branch 'master' of github.com:widelands/widelands into testsuit…

26f6af3

…e-jammy

Fix a very unlikely use-after-free found by g++12

7bb375d

Make a buffer larger to prevent truncation (not likely to happen, but…

f3ccb24

… g++12 wants this anyway)

Noordfrees linked an issue May 13, 2022 that may be closed by this pull request

Does not compile using gcc version 12.1.0-1 #5418

Closed

Noordfrees mentioned this pull request May 13, 2022

Does not compile using gcc version 12.1.0-1 #5418

Closed

Noordfrees added 4 commits May 13, 2022 18:31

Merge branch 'master' of github.com:widelands/widelands into testsuit…

9e6ab69

…e-jammy

Fix another one from the issue

417d03a

g++ really likes its false-positive warnings

663ec08

Revert "g++ really likes its false-positive warnings"

89abf13

This reverts commit 663ec08.

andy5995 mentioned this pull request May 14, 2022

combine dependency installation into script #5422

Merged

Combine dependency installation into script (#5422)

a94f593

Noordfrees added 2 commits May 16, 2022 10:17

Merge branch 'master' of github.com:widelands/widelands into testsuit…

e696010

…e-jammy

Disable -Werror on g++12

37839d2

Noordfrees marked this pull request as ready for review May 16, 2022 12:25

andy5995 reviewed May 18, 2022

View reviewed changes

.github/workflows/build.yaml Show resolved Hide resolved

Noordfrees added 5 commits May 18, 2022 11:51

Use bracketed lists in matrix

3634214

Merge branch 'master' of github.com:widelands/widelands into testsuit…

0a2dc77

…e-jammy

Fix matrix syntax

1155323

Merge branch 'master' of github.com:widelands/widelands into testsuit…

e4e36c5

…e-jammy

Turns out we can not use lists in matrix includes after all

3563821

andy5995 mentioned this pull request May 19, 2022

Integrate AppImage building into CI #5377

Merged

matthiakl approved these changes May 21, 2022

View reviewed changes

Noordfrees merged commit e4da6d6 into master May 21, 2022

Noordfrees deleted the testsuite-jammy branch May 21, 2022 14:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update testsuite compilers and OSs for Jammy #5415

Update testsuite compilers and OSs for Jammy #5415

Noordfrees commented May 12, 2022

Noordfrees commented May 14, 2022

matthiakl commented May 14, 2022

andy5995 commented May 15, 2022

andy5995 commented May 15, 2022

andy5995 commented May 15, 2022

andy5995 commented May 15, 2022

andy5995 commented May 15, 2022 •

edited

Loading

andy5995 commented May 16, 2022

andy5995 commented May 16, 2022

andy5995 left a comment •

edited

Loading

andy5995 May 18, 2022

Noordfrees May 18, 2022

andy5995 May 18, 2022

Noordfrees May 18, 2022

matthiakl left a comment

Noordfrees commented May 21, 2022

svenstaro commented Jul 9, 2022

matthiakl commented Jul 9, 2022

svenstaro commented Jul 9, 2022

Noordfrees commented Jul 9, 2022

svenstaro commented Jul 9, 2022

	char buffer_prefix[256];
	char buffer_prefix[sizeof "[24:00:00.000 xxxx] XXXXXXX:"];

Update testsuite compilers and OSs for Jammy #5415

Update testsuite compilers and OSs for Jammy #5415

Conversation

Noordfrees commented May 12, 2022

Noordfrees commented May 14, 2022

matthiakl commented May 14, 2022

andy5995 commented May 15, 2022

andy5995 commented May 15, 2022

andy5995 commented May 15, 2022

andy5995 commented May 15, 2022

andy5995 commented May 15, 2022 • edited Loading

andy5995 commented May 16, 2022

andy5995 commented May 16, 2022

andy5995 left a comment • edited Loading

Choose a reason for hiding this comment

andy5995 May 18, 2022

Choose a reason for hiding this comment

Noordfrees May 18, 2022

Choose a reason for hiding this comment

andy5995 May 18, 2022

Choose a reason for hiding this comment

Noordfrees May 18, 2022

Choose a reason for hiding this comment

matthiakl left a comment

Choose a reason for hiding this comment

Noordfrees commented May 21, 2022

svenstaro commented Jul 9, 2022

matthiakl commented Jul 9, 2022

svenstaro commented Jul 9, 2022

Noordfrees commented Jul 9, 2022

svenstaro commented Jul 9, 2022

andy5995 commented May 15, 2022 •

edited

Loading

andy5995 left a comment •

edited

Loading