Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean arch-dependent guessing, allow cross-compile #36

Closed
thinrope opened this issue Feb 4, 2019 · 3 comments
Closed

Clean arch-dependent guessing, allow cross-compile #36

thinrope opened this issue Feb 4, 2019 · 3 comments

Comments

@thinrope
Copy link

thinrope commented Feb 4, 2019

I have been struggling with getting bro (i.e. zeek) package updated in Gentoo and a random issue caught my eye: use of -msse2 CFLAG. I looked around and it seems part of broker starting at around https://github.com/zeek/broker/blob/master/CMakeLists.txt#L378 :

include(TestBigEndian)
test_big_endian(BROKER_BIG_ENDIAN)

include(CheckIncludeFiles)
set(CMAKE_REQUIRED_FLAGS -msse2)
check_include_files(emmintrin.h HAVE_SSE2)
set(CMAKE_REQUIRED_FLAGS)

if (HAVE_SSE2)
  add_definitions(-DBROKER_USE_SSE2 -msse2)
endif ()

Especially when part of another build (zeek) that makes quite a headache....
It was mentioned in zeek/zeek#249 (comment) that "that code isn't actually used at run-time anymore in latest versions of Bro 2.6.x.", may be this can just be purged?

Packaging software, e.g. in Gentoo, a source-based bistro, means that often flexibility such as choosing the target hardware is needed and used.

@jsiwek
Copy link
Contributor

jsiwek commented Feb 4, 2019

I don't quite get what you're asking, but if you are cross-compiling, you are going to need to look into "CMake toolchains". I outlined an example of how cross-compiling works here:

https://docs.zeek.org/en/latest/install/cross-compiling.html

i.e. I think the SSE2 availability check isn't the real problem if you have not actually set up the right cross-compilation tool chain.

@thinrope
Copy link
Author

thinrope commented Feb 4, 2019

I am just saying that if (HAVE_SSE2) should not be used in an outer layer (i.e. trying to detect what the build host is), because it may not be the running host later. For such cases, it should me more along:

if (USE_SSE2)
    add_definitions(-DBROKER_USE_SSE2 -msse2)
endif ()

where USE_SSE2 is conditional set by the environment (as opposed to guessed from the hardware).
Basically, something along "if CFLAGS are set, don't mess with them, there is probably a good reason".

Since you mentioned that this may actually be not used anymore, I was wondering if this whole block can be dropped at some point.

@jsiwek
Copy link
Contributor

jsiwek commented Feb 5, 2019

I am just saying that if (HAVE_SSE2) should not be used in an outer layer (i.e. trying to detect what the build host is), because it may not be the running host later.

But CMake should understand the difference between build and target hosts if you use a CMake toolchain file like I pointed out?

where USE_SSE2 is conditional set by the environment (as opposed to guessed from the hardware).

Then we'd need such conditions for every platform-specific test? e.g. like also for endianness as you pointed out.

CMake toolchains should be helping for this cross-compiling issue more generally -- so we don't need a special flag/condition that users may need to set for every platform-specific feature test.

Example for this specific case: if you have a cross-compilation toolchain and point CMake to use it, then that SSE2 TEST should correctly fail if emmintrin.h wasn't a part of the cross-compilation sysroot's include files, even if the build host does have that file/capability.

Since you mentioned that this may actually be not used anymore, I was wondering if this whole block can be dropped at some point.

It's not used at the moment because there has not been a performance measurement to judge whether this optimization is worthwhile:

#4

If it is, then we would bring back the code which makes use of SSE2 anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants