Make it compile on *BSD

[reezer]

With this change compilation on the BSDs will work.

[saper]

did you test it on all of the BSDs? On FreeBSD 15 and NetBSD 10 it needs bswap16, bswap32, bswap64. But even with that it place, it fails the test (after rebase to 25ea924)

/home/saper/src/kaitai_struct_cpp_stl_runtime/tests/unittest.cpp:36: Failure
Expected equality of these values:
  ks.read_f4le()
    Which is: -9.6157696e+09
  3.14159f
    Which is: 3.1415901

[GreyCat]

@reezer @saper I've actually took a stab at adding FreeBSD to our CI pipeline, and here's what I see:

https://github.com/kaitai-io/kaitai_struct_cpp_stl_runtime/actions/runs/9252769532/job/25451063405#step:3:1575

This one is running FreeBSD 14.0 + Clang 16.0.6. There are no problems with compilation observed at all, but there is one problem in runtime related to how iconv is implemented on FreeBSD:

  [ RUN      ] KaitaiStreamTest.bytes_to_str_invalid_seq_gb2312_two_bytes
  /home/runner/work/kaitai_struct_cpp_stl_runtime/kaitai_struct_cpp_stl_runtime/tests/unittest.cpp:383: Failure
  Failed
  Expected illegal_seq_in_encoding exception
  
  [  FAILED  ] KaitaiStreamTest.bytes_to_str_invalid_seq_gb2312_two_bytes (0 ms)


...

  [==========] 46 tests from 1 test suite ran. (216 ms total)
  [  PASSED  ] 45 tests.
  [  FAILED  ] 1 test, listed below:
  [  FAILED  ] KaitaiStreamTest.bytes_to_str_invalid_seq_gb2312_two_bytes

Could you take a look and see how your environment where bswap* etc are needed are different from the one in CI?

[saper]

Big big thanks for looking into this. I have pushed my code for NetBSD/FreeBSD to https://github.com/saper/kaitai_struct_cpp_stl_runtime/tree/some-bsd-build

I get the iconv failure as well.

Your branch works on FreeBS 15 too (sans iconv problem)

NetBSD does to seem to have <byteswap.h>

[ 25%] Building CXX object CMakeFiles/kaitai_struct_cpp_stl_runtime.dir/kaitai/kaitaistream.cpp.o
/home/saper/src/kaitai_struct_cpp_stl_runtime/kaitai/kaitaistream.cpp:32:10: fatal error: byteswap.h: No such file or directory
32 | #include <byteswap.h>
| ^~~~~~~~~~~~

Looks like there something very wrong with the approach in the PR as originally posted

[saper]

Maybe there is ready to use cmake test for things like this?

quick search reveals they might have something similar for autoconf https://www.gnu.org/software/gnulib/manual/html_node/byteswap_002eh.html

[GreyCat]

@saper My point on this is very simple: if somebody cares about support of specific OS/platform combination, let's reproduce it in CI. Otherwise, as development progresses, it's a matter of time when it will break again.

So, if FreeBSD 14 generally works with the CI test I've started introducing, let's focus on understanding the iconv problem and finish it first (or at least understand that it's impossible to finish, document this as known quirk and disable that test on FreeBSD 14).

For NetBSD, we'll need similar thing: some new CI flow (likely based off this GH Action) that proves that we're ok compiling in that environment. Would you want to contribute it?

Maybe there is ready to use cmake test for things like this?

quick search reveals they might have something similar for autoconf https://www.gnu.org/software/gnulib/manual/html_node/byteswap_002eh.html

A good idea, might worth checking. What you refer to, actually, is not autoconf, but gnulib — and, to certain extent, it's taking whole different approach: https://github.com/coreutils/gnulib/blob/master/lib/byteswap.in.h

It's not really checking for specific OS/platform macros, but really just relies on existence of __has_builtin macro function and uses that function to check if bswap_XX exist directly. It's a relatively new thing, as far as I can tell:

• Clang has it since v3 (~2017)
• gcc has it since v10 (~2020)
• MSVC has it since VS 2019 (so, ~2018)

Everything older, it basically downgrades to implementing it fully manually with splitting bytes and reordering with shifts, so not ideal.

[generalmimon]

@GreyCat:

Everything older, it basically downgrades to implementing it fully manually with splitting bytes and reordering with shifts, so not ideal.

Well, I agree that it might be suboptimal in terms of performance (at least in theory; it's somewhat baseless to claim this without benchmarks), but I think that in general manual bit shifts are the best fallback for platforms that we don't recognize. Using dedicated built-in functions to swap endianness instead of a manual approach is an optimization, not a necessity. And some think it's not even worth the trouble, see roelschroeven@9b92677, https://commandcenter.blogspot.com/2012/04/byte-order-fallacy.html and https://justine.lol/endian.html.

The current policy "treat anything we don't recognize as Linux":

kaitai_struct_cpp_stl_runtime/kaitai/kaitaistream.cpp

Lines 30 to 33 in 25ea924

	#else // !__APPLE__ or !_MSC_VER or !__QNX__
	#include <endian.h>
	#include <byteswap.h>
	#endif

... is just wrong, and it often means that getting the KS C++ runtime to even compile on other than the few platforms we recognize becomes a DIY experience.

[GreyCat]

@generalmimon

The current policy "treat anything we don't recognize as Linux":
... is just wrong, and it often means that getting the KS C++ runtime to even compile on other than the few platforms we recognize becomes a DIY experience.

I'm not sure if I fully agree. In principle, yes, it's a good thing to have gradual fallbacks and everything. In practice, I'm so far more inclined to believe that vast majority of systems outside of mainstream (e.g. embedded C++ implementations, RTOS, game consoles, etc), would emulate Linux approach as "standard" rather than anything else. Same goes for gnulib, if anything.

"DIY experience" is not bad per se. Yes, it's a bit more involving, but in the end you're pretty sure you've added correct implementation (and you can contribute it back to upstream). Sweeping things under the rug, putting "works everywhere but potentially inefficient" implementation is making things much harder in the long run, when you end up with performance problems.

[saper]

Maybe in this case we do not need to support X systems on Y platforms. What we really care is a couple of source-level interfaces such as <endian.h> ,<byteswap.h>, <libkern/OSByteOrder.h>, <gulliver.h> for QNX and something from <stdlib.h> for Windows.

If most of those are source-level macros, they should be easy to test even without having a CI on a target platform. The only thing worth checking may be actual behaviour of some compliers.

If some system is not covered by the above, it can be added if anyone cares.

If we are lucky we might not need too many of defined(__FreeBSD__) and the likes.

[generalmimon]

@GreyCat:

Sweeping things under the rug, putting "works everywhere but potentially inefficient" implementation is making things much harder in the long run, when you end up with performance problems.

BTW, as hinted at https://justine.lol/endian.html, the manual bit shifting approach is optimized to the same assembly as the platform-specific way of swapping bytes since GCC 5.1 (released in 2015) and Clang 5.0 (released in 2017). This also applies to the icx 2021.1.2 compiler (which is the oldest version of this compiler in Compiler Explorer). However, it doesn't apply to msvc v19.39 (which is the latest version at the moment) or icc 2021.10.0, which is apparently deprecated in favor of its successor icx, though.

See https://godbolt.org/z/eEWehPhq4:

[GreyCat]

@generalmimon I get it, thanks for thorough checking again :) Still, my point is that in this specific dilemma, I'm clearly on the side of "let's rather explode in build-time, but not risk potential undetectable perf regression". Relying on a feature in the compilers which is around since ~2015-2017 is kind of "very modern" in C++ world.

Folks, I've more or less solved everything but iconv problem, which I propose to basically detect and disable — let's continue discussion in #74?

Specifically this PR, I believe we're ok to abandon: it's out of date, has conflicts, and, moreover, it seems to not actually solve the problem.

[generalmimon]

@GreyCat:

Still, my point is that in this specific dilemma, I'm clearly on the side of "let's rather explode in build-time, but not risk potential undetectable perf regression".

I see your point and thanks for the explanation. You're right that the perf regression would be much harder to detect than a build-time error on an unknown platform, which is annoying for users in the short term but at least they're not unknowingly using a subpar implementation that could cause problems in the future.

I did the comparison in Compiler Explorer mainly out of my own curiosity. The result is kind of interesting, but since we want to have good performance even on old compilers, it rather proves that we (unfortunately) still need the platform-specific handling.