Getting make develop to work with Clang and different GCC versions

[BenHetherington]

As I mentioned in #281, I tried running make develop for the first time recently. However, the current set of options don't seem to be compatible with Clang (specifically, Apple LLVM version 9.1.0 included with Xcode 9.4), so I started looking into ways to make it compatible.

In addition, when trying to run it on my Ubuntu 16.04 VM, neither the default GCC 5.5, nor the more recent GCC 8.0 appeared to support the options in make develop, either.

This took me down a bit of a rabbit hole, in terms of trying to find what options were needed to get it working, and what options were compatible with both. I then realised that some discussion might be need about how best to proceed with this.

Questions

• Should we continue to use -Wpedantic, knowing that the exact warnings may change between compiler versions?
• If so, do we limit our options to warnings that are supported by Clang and most versions of GCC? This would make things easy, but would make the compile-time checking less thorough.
• If no to either of the above, do we give different warning flags to each compiler, to take advantages of the warnings that both are able to provide? I'm not sure what the best way of doing this in make would be.

While I don't mind if we use -Wpedantic or not, I would personally prefer to enable specific warnings for each compiler, so we get the advice of both compilers when running on Travis CI, although I can see why hard-coded compiler checks may be undesirable.

In addition, I would also suggest:

• Use make develop in Travis CI builds, to ensure that incoming PRs don't trigger any new warnings, and to ensure that the flags are compatible with the compilers that are available there.
  • While we're there, I would advocate re-enabling the Mac builds on Travis CI; I think they've got past their blip now.

More detailed changes

These are the specific changes that I had to make in order to use make develop on each of these compilers.

Clang

It requires the following flag changes:

- -Wchkp
- -Wformat-overflow=2
- -Wformat-truncation=1
- -Wstringop-overflow=4
- -Walloc-zero
- -Wduplicated-cond
- -Wlogical-op
- -Wno-aggressive-loop-optimizations
+ -O1                                (needed for -fsanitize=object-size; GCC also recommends optimisations)
+ -Wno-missing-field-initializers    (needed for -Wpedantic; GCC-compatible)
+ -Wno-format-nonliteral             (needed for -Wpedantic; GCC-compatible)

Granted, I've not tried the latest open-source LLVM build, so that may well support some of these flags, unlike the version of LLVM that is included with Xcode.

It also flags a genuine warning in src/link/object.c:

src/link/object.c:286:14: warning: cast from 'uint8_t *' (aka 'unsigned char *')
      to 'struct sSymbol **' increases required alignment from 1 to 8
      [-Wcast-align]
                tSymbols = (struct sSymbol **)&dummymem;
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

I'm not entirely sure why we need dummymem – I'd have thought removing it and simply using NULL would be fine, and it does appear to work on the test repos, but I've not double-checked the code any further.

Aside: We could also take advantage of Clang's runtime sanitisers, to catch any undefined behaviour, memory problems, etc. while running the test repos. Particularly something to think about while running things over Travis! 🙂

GCC 5.5

It requires the following flag changes:

- -Wformat-overflow=2
- -Wformat-truncation=1
- -Wstringop-overflow=4
- -Walloc-zero
- -Wduplicated-cond

(Though apparently I've not configured it properly, so I can't get it to link 😛)

GCC 8.0

It requires the following flag changes:

+ -Wno-strict-overflow    (needed for -Wpedantic; Clang-compatible)

(Again, my VM seems to be borked on the linking front)

…And that's enough of a wall of text 😆 If you need more help with figuring out what does and doesn't work, just let me know and I'll be happy to give things a try.

[AntonioND]

I agree with having different flags per compiler (compiler warnings and runtime sanitisers). I didn't really expect my flags to last forever, but it's just a huge pain to go through all the options of GCC and enable the warnings, so when I started to think about doing that with Clang I decided to leave as it was for the time being.

To be fair, even the Windows binary targets are specific to my system (to a Fedora VM I have set up for this).

However, there is a problem. I think that there is no way to have conditionals in a makefile in a way they are compatible with BSD and GNU make.

By the way, the compiler I use right now (the one that comes in Debian testing, I haven't updated my PC in two weeks) is gcc (Debian 7.3.0-19) 7.3.0.

I don't think it makes sense to remove flags for an older version of GCC (it would make sense for a newer one). I guess that you say this because Travis CI comes with that one by default? There must be some way to use GCC 8 instead. Ideally we would enable all warnings during the CI tests but that's the only place where we need them, so we don't need to keep compatibility with more than the latest versions of GCC and Clang.

Also, that warning that you have pointed out in your message deserves a patch! It is quite weird, and I agree, dummymem should probably be replaced by a NULL in the two places where it is used. But I haven't taken a close look at the code, so I'm not sure.

And yeah, probably the Mac builds can be re-enabled.

[ImperatorStorm]

Testing on archlinux, clang-13.0.0 seems to work fine after changing O0 to -O1
gcc-11.1.0 doesn't recognize ‘-Wno-tautological-constant-out-of-range-compare’ or ‘-Wno-unknown-warning-option’, and errors out where clang doesn't

src/gfx/makepng.c:74:15: error: variable ‘outfile’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
   74 |         char *outfile;
      |               ^~~~~~~

Haven't tested with gcc-5.5 or gcc-8.0, as I don't know where to get binaries for them.

[ISSOtm]

-Og appears to work with Clang and GCC 11; the outfile thing seems like a genuine error—and RGBGFX is a clusterf*k anyway, so I wouldn't be surprised.

I have binaries of GCC 5 to 11 from when Arch distributed packages of those versions. GCC 8 is OK with those flags, GCC 5 not so much:

gcc-5: error: unrecognized command line option ‘-Wformat-overflow=2’
gcc-5: error: unrecognized command line option ‘-Wformat-truncation=1’
gcc-5: error: unrecognized command line option ‘-Wstringop-overflow=4’
gcc-5: error: unrecognized command line option ‘-Walloc-zero’
gcc-5: error: unrecognized command line option ‘-Wduplicated-cond’

We should probably decide on a minimum version of GCC and Clang we wish to be compatible with, at least for development.

[Rangi42]

gcc 8 was released in 2018. For comparison, we currently require bison 3 (released 2013), cmake 3.9 (released 2017), and the C11 standard (from 2011). gcc 5 was released in 2015. I'd be fine with requiring gcc 8, or at least 7.4, which was the default in Ubuntu 18.04 LTS in early 2018.

[ISSOtm]

Yeah, the flags are all OK with GCC 7, but not 6. I think the requirement is acceptable.