Add CMake project

[BurningEnlightenment]

Hi,
I just made a CMake script for the project, because I want to include it within another CMake project. The CMake script includes an install target (mostly for windows) and supports SIMD on the x86 platform (supporting NEON shouldn't be hard, but I don't need it). However I had to change #include "config.h" in order to support out of source tree builds. I hope you are ok with my solution.
I successfully built the library on windows with mingw-w64 gcc 4.8.2 x86_64 and Visual Studio 2015 x86/x86_64/ARM.

My script is CC0 licensed and the TargetArch.cmake is distributed under a 2-clause BSD license (please note that no one distributes a cmake script in binary form, so the second clause concerning binary distribution is more or less meaningless).

Fixes #70

Cheers,
Henrik

[aklomp]

Hi! Thanks a lot for this contribution! As you can tell from the other open pull request, this project is definitely in need of a good cross-platform build system. I think that CMake is probably the best candidate, and that your pull request is a good start. I'd like to work with you to tailor it to this project.

I really appreciate your contribution back to this project, and I understand that I have little ground to critique work done by others in their own time. That said, I have some comments, which I will place in the code. It would be nice if you could have a look at them, but you are, of course, under no obligation.

[BurningEnlightenment]

That said, I have some comments, which I will place in the code. It would be nice if you could have a look at them, but you are, of course, under no obligation.

I appreciate your feedback and will work towards an implementation we are both comfortable with. That said this one of my CMake templates with some necessary modifications and it didn't get too much love, because I was in a hurry (i.e. no NEON support etc.). Please note that (as stated in the copyright notice) I'm not the author of the TargetArch.cmake script which I haven't touched unitl now.

I think I'll answer the rest of your questions/address your comments in place.

[BurningEnlightenment]

@aklomp can you estimate when you can answer my questions? I don't want to pressure you, but I'm currently planning my weekend and it would be nice if I can take this into account.

[aklomp]

No problem. I don't think I'll be doing much on the project today as I have other plans this evening, and the same is mostly true for tomorrow. (I might have some time to work on this project, but no guarantees.) On Sunday, I actually have to be in the office for a server migration, so I think it's a safe bet to say that I won't be doing much over the weekend. Also, I planned to "comment" in the form of pushing some example code, which takes a bit more time than just typing text.

[BurningEnlightenment]

Status Update

I reworked and simplified quite some bits. Currently all target platform / instruction set detection code is disabled (no third party code anymore 🎉). I included some SIMD instruction set detection code for SSSE3 and AVX2 and an infrastructure skeleton for it located in TargetSIMDInstructionSet.cmake just in case you want to add those checks in the future.

I did some research regarding NEON, but I need an ARM platform in order to check the cmake behavior on that platform and the implications for cross compiling, i.e. that won't be added before my Odroid-C1+ arrives...

Did I forget something?

Addendum: Just to clarify it: You can build the library for every platform and with every compiler cmake supports, but SSSE3 and AVX2 are only supported with the gcc, clang and msvc compilers...

[BurningEnlightenment]

Just added cmake projects for the test driver and the benchmark.

[aklomp]

Looks good, I'll review it on short notice! Do you consider it fit to merge as-is? Don't worry about the NEON stuff, I have a Raspberry Pi 2B which has NEON32 extensions, and I can run NEON64 in an emulator. We can iterate.

Sorry for all the delay btw. Your pull request has been on my radar for the whole time, but I've also been occupied with implementing a faster algorithm (currently in Master), and working on merging OpenMP support (see Issue #9).

[BurningEnlightenment]

Do you consider it fit to merge as-is?

The scripts work well enough for the plain, SSSE3 and AVX2 use cases to be merged . And as you said we can will iterate 😉. I'll probably make (a) new pull request(s) for added OpenMP support and NEON stuff after I passed my exams.

implementing a faster algorithm (currently in Master), and working on merging OpenMP support

Awesome work! 👍

[aklomp]

Just checked out some of your CMake stuff. I'm definitely new at this, but I'm open to learn. I wanted to build the benchmarks with AVX2 support, so I tried this:

cmake -DBASE64_WITH_AVX2=1 .
make
cd test
cmake .
make

This series of commands fails for me. It looks like the generated Makefiles in test/ want to link with a system-wide -lbase64, while they should actually just include ../lib/libbase64.o as an extra object file. Except that that file is not built anymore. Instead there is a libbase64.a in the project root, which exports way too many global symbols. (The list of "allowed" global symbols is in lib/exports.txt.) If I remove the target_link_libraries call in test/CMakeLists.txt and add ../libbase64.a as an extra source file, things break even more. Am I doing something wrong?

Other things I noticed: the bin/base64 binary is not built by default, and make clean no longer fully works. It doesn't purge generated stuff like CMakeFiles/.

[aklomp]

FWIW, I managed to compile the tests by adding the following to test/CMakeLists.txt:

find_library(LIBBASE64 libbase64.a ..)
target_link_libraries(${TEST_NAME} ${LIBBASE64})

[BurningEnlightenment]

Well, try this (after you completely cleaned the project dir):

cd /path/to/project/src
mkdir build
cd build

cmake -DBASE64_WITH_AVX2=1 ..
make
make test

Explanation

make clean only removes the build output, but not the build files which were generated by cmake. Because of this and because generating a Makefile in the project root will overwrite your Makefile it is advised to make an out of source build, i.e. create a "build" directory where all the temporary build files reside (this doesn't necessarily have to be a subdirectory of the project root). In this scenario you don't do make clean, but rm -rf build. If you use a project subdirectory like build/ I recommend you adding build/ to your global gitignore file.

Furthermore cmake likes hierarchies, so test/CMakeLists.txt isn't really a separate cmake project which can be used/build on its own. Instead it is called from the root CMakeLists.txt providing the necessary context like where our library is located (FindLibrary(...) will find you your system base64 library...). This also means that the cmake ... command generates one master Makefile in the "build" directory which is used for everything.

One more thing to note: make test will not (re-)build the test executables, it'll only execute them and report whether one of them crashed returned a value != 0. Therefore I've included the test executables in the all target, so that they will get rebuild if anything (library sources, executable sources, build files) changes.
Furthermore you probably want to use either ccmake or cmake-gui, because they list all available options and let you easily tweak the compiler arguments.

[BurningEnlightenment]

@aklomp are you still interested in this? If yes I will invest some time updating the build files soon:tm:.

[aklomp]

Sorry about the silence, I didn't intend to string you along. I am definitely still interested in cmake integration (because it's the de-facto cross-platform C buildtool), and I think your work is a very good starting point. I also think you tackled all the comments I made previously, so I should do my share. What's preventing me from merging it directly is mostly this:

• I'm unfamiliar with cmake, but if I merge this, I would have to support it from now on. This is my problem, not yours, but it means that I need to spend some time learning about it before I can commit to it. I'm a man of many projects and great laziness, so I've been putting it off.
• Related: I feel like I do not have the cmake knowledge to grade the quality level of your contribution. I mean, it looks fine, but maybe it's not idiomatic or whatever. I can't really get my finger behind it, which makes me hesitant to accept it. (Again, my problem.)
• I'm not sure I like cmake very much. This is totally irrelevant in the light of it being the de-facto cross-platform C buildtool, but it explains something about the points above. I recognize my own bias here.

Maybe this is how we can proceed:

• I should spend some time learning about cmake, no excuses.
• I can merge the code into a staging branch for the time being, so that it becomes part of the master project, and proceed from there.

[BurningEnlightenment]

I don't like cmake either, but sadly as you said it's a de-facto standard and just works:tm: :smile:

• I should spend some time learning about cmake, no excuses.

👍

• I can merge the code into a staging branch for the time being, so that it becomes part of the master project, and proceed from there.

Yeah, I would like to get this into the upstream project, but I think we should wait with merging until you can maintain the build files yourself. Instead I will watch your repository and keep the cmake files in my fork in sync with the upstream repository. I suggest you add a link in the project description referring to the fork; this way I don't have to open new pull requests every time something changes and people can use the cmake tooling now, so:

• I will close this pull request and rename my base64 to base64-cmake and move the cmake files to the master branch
• I continue to maintain the cmake files for now
• You learn cmake
• New pull request + merge of the build files into the upstream repository
• You maintain the build files

Are you 🆗 with this?

[dotnwat]

Hi everyone.

What's the plan to get cmake support merged? It looks like the latest feedback has been taken into account, and having some support would be nice. It will be easier to iterate on cmake support as the community starts to use it.

[madebr]

To my own projects, I usually add a PROJECT_WERROR option to enable/disable warnings as errors (enabled by default).
That way, users have the possibility to ignore warnings.
This is useful for new compiler releases that introduce new warnings.

option(BASE64_WERROR "Treat warnings as error" ON)
target_compile_options(base64 PRIVATE
    $<$<BOOL:${BASE64_WERROR}>:$<IF:$<C_COMPILER_ID:MSVC>,/WX,-Werror>>
)

Currently, only MSVC has warnings as error behavior.

[BurningEnlightenment]

I concur.
What do you think about the warnings which are explicitly marked as errors? I don't remember adding them 😅

[madebr]

Perhaps add a else() with a warning about an unknown arch?

else()
    message(WARNING "Unknown architecture: ${_TARGET_ARCH}")
endif()

[aklomp]

A warning definitely makes sense.

[BurningEnlightenment]

Does it though? Obviously there won't be SIMD acceleration, because SIMD instruction sets are architecture specific and one chose to compile for an alien one. Or is this more about not providing official support/ not regularly testing the pure C codec on that architecture?

[madebr]

Another comment, shouldn't TARGET_ARCH be HOST_ARCH?
A target architecture only makes sense for a code generator (=compiler).
Here, we're just converting some sources to binaries (on a build machine to some host machine).

[madebr]

Can you take another look at this suggestion?

[madebr]

I think import symbols don't need this.

Suggested change

	#define BASE64_SYMBOL_IMPORT __attribute__ ((visibility ("default")))
	#define BASE64_SYMBOL_IMPORT

[BurningEnlightenment]

I'm not an expert w.r.t. gcc symbol visibility; I looked at the GenerateExportHeader output and noticed that they do define the import macro with said attribute (s. GenereateExportHeader.cmake:310). However, I don't know the rationale.

[madebr]

My interpretation is that adding visibility("default"), you're instrumenting the compiler to export a symbol.
So adding this as a consumer will cause you to re-export the symbol.

[aklomp]

@dotnwat Merging cmake support is with me as the maintainer of this project 😿

As I've written earlier in this thread, I feel conflicted about merging this PR because I have hardly any experience with cmake, I don't really understand the changes being introduced, and I'm not confident I can maintain them. The obvious solution to that problem was for me to start learning cmake so I can converse on equal footing. But I still haven't gotten round to it (made it enough of a priority), and I'm sorry about that.

(It's absolutely been on my ongoing mental to-do list for years and causing me quite some anguish as one of those points that you really need to do one day. I didn't just walk away from this. But that's not doing anyone in this thread any good.)

So at a given moment I said I was willing to merge any cmake config that demonstrably works. That offer still stands. So why am I not merging this PR now?

• There are several open questions in this thread (mainly by @madebr) which I don't feel qualified to answer.
• I can't get cmake to actually compile the library (more below). That makes me back off from merging.
• The PR does not include an update to the README to explain how to compile the code with cmake. OK, that's minor and can be fixed later.

So that's basically why it's in limbo. As ashamed as I now am by all of this, I am seriously contemplating just merging everything sight-unseen and sorting out any breakage in later PRs. I tend to agree with you that any support is better than no support, and that once it's mainlined, it can be iterated upon.

Back to the build not working for me. I'm trying to build the code with this set of commands:

EDIT: It works fine now. I was looking at an older version of the branch. Will play a bit with this then. Hollywood ending after all?

[aklomp]

One large problem with this PR is that everything is compiled without optimization. The result is that the code is incredibly slow. All the tricks that we do to get good performance (SIMD, fancy algorithms, aggressive inlining) are all for nothing: plain is the fastest codec. Here's part of the benchmark output when compiled with cmake -B build; cmake --build build:

Testing with buffer size 100 KB, fastest of 10 * 100
AVX2    encode  146.29 MB/sec
AVX2    decode  137.06 MB/sec
plain   encode  550.22 MB/sec
plain   decode  476.14 MB/sec
SSSE3   encode  135.73 MB/sec
SSSE3   decode  146.43 MB/sec
SSE41   encode  152.59 MB/sec
SSE41   decode  154.20 MB/sec
SSE42   encode  152.85 MB/sec
SSE42   decode  154.21 MB/sec
AVX     encode  158.27 MB/sec
AVX     decode  158.16 MB/sec

Here's what it should look like when the benchmark tool is compiled with the old Makefile, which optimizes at -O3 by default:

Testing with buffer size 100 KB, fastest of 10 * 100
AVX2    encode  13745.55 MB/sec
AVX2    decode  14467.66 MB/sec
plain   encode  3475.35 MB/sec
plain   decode  3259.66 MB/sec
SSSE3   encode  7606.59 MB/sec
SSSE3   decode  8212.31 MB/sec
SSE41   encode  7603.66 MB/sec
SSE41   decode  8401.14 MB/sec
SSE42   encode  7600.30 MB/sec
SSE42   decode  8211.38 MB/sec
AVX     encode  7996.93 MB/sec
AVX     decode  9094.13 MB/sec

This is clearly not a desirable state of affairs. Can cmake be instructed to always optimize to -O3 too?

Edit: the answer is yes, the optimization flags can be added to the block in CMakeLists.txt that sets the error/warning flags.

[aklomp]

If all of these are legitimate issues with the code, I'd prefer to have them fixed rather than ignore the warning.

[BurningEnlightenment]

There is currently one violation in the windows __cpuid macro definition and its usage in codec_choose_x86(). Sadly one can only disable the warning as a whole and not just it being treated as an error.

[aklomp]

Support for these levels are not selectable in MSVC?

Edit: to answer my own question: no, they are not. It's strange though, because what to do then? We can't really use the next best level, which is AVX. But the default architecture is only SSE2, so presumably these codecs would miscompile. Argh.

[BurningEnlightenment]

To the contrary the SIMD intrinsics up to and including SSE42 are always available on the MSVC platform and cannot be disabled. This isn't an issue for MSVC because they don't utilize them for auto vectorization (unlike gcc).

[BurningEnlightenment]

One large problem with this PR is that everything is compiled without optimization.

CMake has four different build types (Debug, MinSizeRel, RelWithDebInfo and Release) of which it defaults to Debug which as you noticed is an unoptimized build. The other build types have the usual optimization flags set which can also be overriden by the user. You can explicitly specify a build type like so:

cmake -B build -DCMAKE_BUILD_TYPE=Release

I recommend using the more graphical ccmake rather than cmake if you want to inspect/ fiddle with the compiler flags.

[aklomp]

@BurningEnlightenment I still think it's reasonable to set -O3 (and the equivalent for MSVC) on all builds, because the library is pretty crap without them. With the default build, you'd be better off using a plain C library than using any of the SIMD codecs. The Makefile also sets -O3 unconditionally, so there's precedence.

I'd like to add it unless it violates a core cmake principal and horrifies the gods.

[dotnwat]

If there are optimizations that are only available at -O3 then it is reasonable to hard code that for all compilers. And you can even do that only for Release builds (which is always useful). I see on my system that -O3 is the default for GCC in cmake (with CMAKE_BUILD_TYPE=Release), but it doesn't look like clang is specifying anything specifically.

You can always use -O3 for everything and let exceptions be handled as they arise.

Happy to help out a bit.

[madebr]

CMake uses cmake toolchains for specifying the host architecture.
See CMAKE_SYSTEM_PROCESSOR.
Perhaps we should first try to parse CMAKE_SYSTEM_PROCESSOR, and then try to run the compiler?
Using that approach, users can override the architecture in the case of a mis-detection.

[dotnwat]

Or you could effectively transliterate the makefile into cmake (ie require users to set options). I'm all about making this as feature rich as possible, but it doesn't seem like it makes sense to wait on the perfect cmake build.

[madebr]

I find the _DEFINE suffix weird: it's a definition anyways.
How about simply using BASE64_STATIC?

[madebr]

@BurningEnlightenment I still think it's reasonable to set -O3 (and the equivalent for MSVC) on all builds, because the library is pretty crap without them. With the default build, you'd be better off using a plain C library than using any of the SIMD codecs. The Makefile also sets -O3 unconditionally, so there's precedence.

I'd like to add it unless it violates a core cmake principal and horrifies the gods.

I think we can keep the cmake gods happy by only doing "-O3" when building with a Release or RelWithDebInfo config.
Then, we can keep the lower optimization level for Debug builds, so debugging remains easy.

Something as follows: (untested)

if(NOT CMAKE_C_COMPILER_ID STREQUAL "MSVC")
    string(REGEX REPLACE "([/-])O[0-2]" "" CMAKE_C_FLAGS_RELEASE "${CMAKE_C_FLAGS_RELEASE}")
    string(REGEX REPLACE "([/-])O[0-2]" "" CMAKE_C_FLAGS_RELWITHDEBINFO "${CMAKE_C_FLAGS_RELWITHDEBINFO}")
    set(CMAKE_C_FLAGS_RELEASE "${CMAKE_C_FLAGS_RELEASE } -O3")
    set(CMAKE_C_FLAGS_RELWITHDEBINFO "${CMAKE_C_FLAGS_RELWITHDEBINFO} -O3")
endif()

[htot]

I just added an issue BurningEnlightenment#4

[htot]

I am writing a Yocto project recipe for this library and tools and are basing it on the cmake. Because the high level of standardization saves me work. It would be nice to base the recipe of this repo instead of @BurningEnlightenment fork. Can we get it in and then optimize later?

I still think it's reasonable to set -O3 (and the equivalent for MSVC) on all builds, because the library is pretty crap without them.

The purpose of the debug build is that you can debug in a sensible way, so optimizations need to be disabled. If you really need to debug optimized code (and willing to suffer the big pain) there is the RelWithDebInfo flag.

Has anybody seen a significant speedup from -O2 to -O3?

[aklomp]

Thanks everyone for your patience, the @BurningEnlightenment cmake branch has finally been merged! I recognize that @htot has a point in asking for the branch to be merged first and incremental improvements to be made later, even though it goes against my perfectionist tendencies :)

Anyway, sorry this took so long. Let's iterate on this in new issues.

[BurningEnlightenment]

I will leave the fork up for a month or two in order to allow people to migrate their CI/ package manager scripts gracefully.

I'd also like to thank everyone involved for the reviews, feedback and contributions 🙂

[dotnwat]

Happy to see this merged. And great timing; I was just about to put up a PR to modify the relative include paths, as we are using Meson to integrate into our build system and it requires out-of-tree builds.