Fix LTO on linux

[springmeyer]

This is a fix on top of the cache-split branch. It should fix LTO on linux by:

• ensuring we're using a consistent compiler (clang++ via mason, setup via the scripts/setup.sh script)
  • NOTE: I have dreams of a mbxcxx command that we use to do this in the future (rather than this clumsy scripts/setup.sh copied everywhere)
• ensure that the same compile is used to compile and link all binaries (important for being able to blend LTO binaries with other libraries).

Should fix #117 (comment)

[springmeyer]

This is now working - the LTO job is fixed. But a new one is failing: the g++ one:

g++-6: internal compiler error: Killed (program cc1plus) Please submit a full bug report, with preprocessed source if appropriate.

This is because g++ uses more memory than clang++, mason is compiling in parallel, and the machine is overwhelmed by the memory usage and decides to kill g++

[apendleton]

@springmeyer awesome, this looks great. Sorry for rushing it, as I know you had originally proposed this for post-refactor. My thinking about going for it now was that the next stuff on our roadmap involves pivoting away from this library for awhile and I wasn't sure when we'd next be back to it.

Any idea why there's a one-line coverage change? I'm kind of inclined to ignore it...

[springmeyer]

@apendleton no idea on the one line of coverage - I'm also okay ignoring for now.

This branch should noe be 🍏 and is ready to merge into your cache-split if you'd like to get the LTO working on linux.

The one difficulty here that I'll document for my future self is that the g++-6 job, which we have just for wider coverage of compilers and to leverage the -Weffc++ warning, does not play nicely with the upgraded binutils. I'm not sure why but I was able to fix by only upgrading binutils for clang++ (which makes sense given the latest clang++ and binutils from mason where built against each other and are very recent versions). So this was the g++ error:

/usr/bin/ld: /home/travis/build/mapbox/carmen-cache/mason_packages/.link/lib/librocksdb.a(db_info_dumper.o): unrecognized relocation (0x2a) in section `.text'
/usr/bin/ld: final link failed: Bad value
collect2: error: ld returned 1 exit status

Fixed by disabling binutils install from mason and falling back to the old binutils from the system.

[apendleton]

Yeah, GCC is also much fussier about warnings (partly because we only do Clang warning pragmas at the moment), and as far as I know we're the only consumer of this library and are an all-LLVM shop, so I have no particular attachment to the GCC tests. That said, happy to defer to core tech's assessment of their utility if there are extra classes of error they catch.

Anyhow, thanks again.

[springmeyer]

g++ is included since it's -Weffc++ flag is able to warn on uninitialized class members which are often harmless but can be major trouble (llvm/clang's support for -Weffc++ does not catch this). It is the learning from mapbox/wagyu#70 and reflected in mapbox/cpp#37 (comment). That said, it looks like clang-tidy can now catch this (untested): https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-pro-type-member-init.html and if so that would mean I'd be happy to drop g++ in the future if it is annoying.

[apendleton]

We did have to fight with it a little bit because of some of the newly recommended warning flags we added in this refactor; we use the __int128 type, which clang is fine with but causes gcc compilation to fail with the -pedantic-errors flag added. We switched to just -pedantic (across the board) for now, but would likely switch back to -pedantic-errors if that build target were dropped. It was only momentarily a frustration though and now that we've figured it out I don't expect it to come up again, so it's not a huge deal either way.

[springmeyer]

Thanks for that feedback.

We did have to fight with it a little bit because of some of the newly recommended warning flags we added in this refactor
It was only momentarily a frustration though and now that we've figured it out I don't expect it to come up again, so it's not a huge deal either way.

By design per mapbox/cpp#37. Glad it was not too much of a battle. 👍 then on keeping the g++ job and keeping -pedantic-errors disabled/removed. The intention of those default flags is to help catch inadvertent errors, but fine to disable if you need.