NLnetLabs / simdzone

Fast and standards compliant DNS zone parser
BSD 3-Clause "New" or "Revised" License
63 stars 11 forks source link

Fix problem with building on BSD-based systems (FreeBSD and others) #202

Closed thedix closed 2 months ago

thedix commented 2 months ago

Some systems (e.g. old FreeBSD versions) do not have /usr/include/endian.h resulting the following build errors:

--- src/fallback/parser.o ---
cc -MT src/fallback/parser.o -MMD -MP -MF src/fallback/parser.d -I./include -I./src -I. -O2 -pipe  -fstack-protector-strong -fno-strict-aliasing -o src/fallback/parser.o -c ./src/fallback/parser.c
In file included from ./src/fallback/parser.c:12:
./src/generic/endian.h:77:10: error: 'endian.h' file not found with <angled> include; use "quotes" instead
#include <endian.h>
         ^~~~~~~~~~
         "endian.h"
In file included from ./src/fallback/parser.c:16:
./src/generic/number.h:98:12: warning: implicit declaration of function 'htobe16' is invalid in C99 [-Wimplicit-function-declaration]
  number = htobe16(number);
           ^
./src/generic/number.h:115:12: warning: implicit declaration of function 'htobe32' is invalid in C99 [-Wimplicit-function-declaration]
  number = htobe32(number);
           ^
...

I think we should use system-dependent endian.h like in nsd/lookup3.c:

#if defined(linux) || defined(__OpenBSD__)
#  ifdef HAVE_ENDIAN_H
#    include <endian.h>    /* attempt to define endianness */
#  else
#    include <machine/endian.h> /* on older OpenBSD */
#  endif
#endif
#if defined(__FreeBSD__) || defined(__NetBSD__) || defined(__DragonFly__)
#include <sys/endian.h> /* attempt to define endianness */
#endif

Also the corresponding nsd submodule should be updated in the master branch after the fix. It would be nice to include this fix in nsd 4.10.1.

k0ekk0ek commented 2 months ago

Hi @thedix. Thanks for you work/report! I'll have a look at this in the next couple of days. I think add_definitions is a little to broad and I have some work on separate branch to tackle the same/a similar problem on Solaris. What version of FreeBSD are you using? I have tested on the latest and assumed it would work for everyone. Feel free to keep the commits coming :slightly_smiling_face: if you fix it I'll happily merge, but note that I'm also happy to prepare a vm and make sure it works for that version as well as part of my work.

thedix commented 2 months ago

Hi Jeroen (@k0ekk0ek)!

I removed add_definitions and modified other files to compile using cmake and autotools. Everything seems to compile successfully in my test Ubuntu environment. But I have very little experience with c/c++ build tools. Could you please take a look at this?

Initially I noticed the problems under FreeBSD 12.4, it doesn't have /usr/include/endian.h. I know it is EOL, but we have a few instances in production with 12.4. The latest versions of FreeBSD 13 and 14 already have /usr/include/endian.h and compile without problems.

There is a workaround in nsd to include different paths to endian.h depending on OS, which I mentioned in the first comment (link). I think it's worth using it. The nsd compiles successfully from ports with this patch under FreeBSD 12.4 and 13.2.

thedix commented 2 months ago

Strange thing, Github started only one check (without compiling the code) for the latest commit...

lemire commented 2 months ago

@k0ekk0ek @thedix The modern way to handle this is...

#if defined(__has_include) // virtually always present, part of C23
#if __has_include(<endian.h>)
# include <endian.h>
#elif __has_include(<machine/endian.h>)
# include <machine/endian.h>
#elif __has_include(<sys/endian.h>)
# include <sys/endian.h>
#endif
#endif

See https://en.wikipedia.org/wiki/C23_(C_standard_revision) and https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2799.pdf

This should handle a very wide range of cases.

thedix commented 2 months ago

@lemire

@k0ekk0ek @thedix The modern way to handle this is...

Wow, thanks! This is a very beautiful solution, but it seems that old clang versions don't support __has_include. I think the nsd (which uses this project) should be able to compile on many platforms that may have outdated compilers.

lemire commented 2 months ago

it seems that old clang versions don't support __has_include.

It was present in LLVM 3 (at least as far back as 3.4 it seems, but I can't access older documentation easily).

My proposal was not to blindly assumed that __has_include was present but it is a very decent line of defense as it is almost always supported.

thedix commented 2 months ago

A strange error has occurred in Windows builders:

24/30 Test #24: include_without_origin ...........***Failed    0.01 sec
[==========] syntax: Running 1 test(s).
[ RUN      ] include_without_origin
[  ERROR   ] --- path
[   LINE   ] --- D:\a\simdzone\simdzone\tests\syntax.c:893: error: Failure!
[  FAILED  ] include_without_origin
[==========] syntax: 1 test(s) run.
[  PASSED  ] 0 test(s).
[  FAILED  ] syntax: 1 test(s), listed below:
[  FAILED  ] include_without_origin

 1 FAILED TEST(S)

As I understand the assert for creating temporary file failed:

  char *path = generate_include("foo TXT bar");
  assert_non_null(path);

Is it possible to re-run the build tests?

lemire commented 2 months ago

@thedix I have relaunched the tests but I am not sure it comes from your PR.

thedix commented 2 months ago

@lemire Thank you! Looks like it was a temporary error.

k0ekk0ek commented 2 months ago

Thanks @thedix! It needs a little cleanup, but I'll do that in a separate PR.

k0ekk0ek commented 2 months ago

@thedix, can you give the main branch a try to see if it all works?

thedix commented 2 months ago

@k0ekk0ek, thanks for cleaning up and merging! I have tested old FreeBSD 12, newer FreeBSD 13.2, and Ubuntu 22.04. The main branch compiles successfully both standalone and as a submodule of the nsd code.

k0ekk0ek commented 2 months ago

Thanks for giving it a try @thedix!