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

Abseil fails to compile on ARM with CMake, because it forces x86_64 flags #365

Closed
lilianmoraru opened this issue Aug 12, 2019 · 10 comments
Closed
Assignees

Comments

@lilianmoraru
Copy link

absl/copts/AbseilConfigureCopts.cmake checks for GCC "${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU" and then forcibly adds msse: set(ABSL_RANDOM_RANDEN_COPTS "${ABSL_RANDOM_HWAES_X64_FLAGS}"), even though it does not compile on x86_64.
That last line adds this:

list(APPEND ABSL_RANDOM_HWAES_X64_FLAGS
    "-maes"
    "-msse4.1"
)

Note that the ARM flags(from the generated file with compiler flags) also makes some assumptions.
-mfpu=neon is buggy with the version of the compiler we have and the hardware we run the software on.

@pizzard
Copy link
Contributor

pizzard commented Aug 20, 2019

Same issue here.
The build system just checks fur GCC and then immediately assumes it is and X64 arch. But that itself is never checked, there is plenty of gcc usage on ARM.

The CMAKE crosscompile flags should be checked and if there is crosscompile setup, it is not safe to assume its x64.

@asoffer
Copy link
Contributor

asoffer commented Sep 11, 2019

Thanks for the report, and sorry for the delayed response.
Could you explain further how -mfpu=neon is buggy? That is, on which platforms does this not suffice, and how should they be detected with CMAKE_SYSTEM_PROCESSOR?

@lilianmoraru
Copy link
Author

@asoffer I've found it to be buggy on GCC 6.1 and older. It works most of the times but we had some issues with the rapidjson library. I added this check(it was a quick fix, would not recommend it for anybody else) in the toolchain file:

if (   (CMAKE_C_COMPILER_ID  STREQUAL  "GNU"    AND  NOT CMAKE_C_COMPILER_VERSION VERSION_LESS 7.1)
    OR (CMAKE_C_COMPILER_ID  STREQUAL  "Clang"  AND  NOT CMAKE_C_COMPILER_VERSION VERSION_LESS 5.0))
    set(target_fpu_cflag       "-mfpu=neon-vfpv4")
else()
    set(target_fpu_cflag       "-mfpu=vfpv4")
endif()

if (   (CMAKE_CXX_COMPILER_ID  STREQUAL  "GNU"    AND  NOT CMAKE_CXX_COMPILER_VERSION VERSION_LESS 7.1)
    OR (CMAKE_CXX_COMPILER_ID  STREQUAL  "Clang"  AND  NOT CMAKE_CXX_COMPILER_VERSION VERSION_LESS 5.0))
    set(target_fpu_cxxflag     "-mfpu=neon-vfpv4")
else()
    set(target_fpu_cxxflag     "-mfpu=vfpv4")
endif()

Inside QEMU, it worked fine. It(rapidjson) was crashing(SIGBUS) on Qualcomm Snapdragon X5 LTE modem(9x28) and Qualcomm Snapdragon X12 LTE modem(9x40).

Note: abseil could keep neon and add checks only if people have issues with some specific versions of the compiler for example.
I personally would still prefer for it to allow people to pass custom toolchain files and take the configurations from there and not overwrite them.

if ("${CMAKE_TOOLCHAIN_FILE}" STREQUAL "")
  # sets its own flags
endif("${CMAKE_TOOLCHAIN_FILE}" STREQUAL "")

@lilianmoraru
Copy link
Author

If you want, I could work on this but I will do it when I find some free time(most probably this weekend).
I did not touch the files since it seems that most of the configs are generated from Blaze.

@powderluv
Copy link

powderluv commented Oct 7, 2019

Same issue. According to AbseilConfigureCopts.cmake it is because of this section that assumes if you are using a GNU toolchain you are on x86_64

if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
set(ABSL_DEFAULT_COPTS "${ABSL_GCC_FLAGS}")
set(ABSL_TEST_COPTS "${ABSL_GCC_FLAGS};${ABSL_GCC_TEST_FLAGS}")
set(ABSL_RANDOM_RANDEN_COPTS "${ABSL_RANDOM_HWAES_X64_FLAGS}")
elseif("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")
MATCHES so we get both Clang and AppleClang

Im building abseil on aarch64 Gentoo on RPi4 for EdgeTPU access and it fails because the default compiler is gcc

@lilianmoraru
Copy link
Author

Sorry, I am not able to work on this, completely overflowed with work on weekends...

absl-federation-github pushed a commit that referenced this issue Oct 24, 2019
--
e54b9c7bbb0c58475676c268e2e19c69f4bce48a by Jorg Brown <jorg@google.com>:

Tweak ABSL_PREDICT_TRUE slightly, for better code on some platforms and/or
optimization levels.  "false || (x)" is more verbose than "!!(x)", but
ultimately more efficient.

For example, given this code:

void InitIfNecessary() {
  if (ABSL_PREDICT_TRUE(NeedsInit())) {
    SlowInitIfNecessary();
  }
}

Clang with default optimization level will produce:

Before this CL              After this CL
InitIfNecessary:            InitIfNecessary:
  push rbp                    push rbp
  mov  rbp, rsp               mov  rbp, rsp
  call NeedsInit              call NeedsInit
  xor  al, -1
  xor  al, -1
  test al, 1                  test al, 1
  jne  .LBB2_1                jne  .LBB3_1
  jmp  .LBB2_2                jmp  .LBB3_2
.LBB2_1:                    .LBB3_1:
  call SlowInitIfNecessary    call SlowInitIfNecessary
.LBB2_2:                    .LBB3_2:
  pop  rbp                    pop  rbp
  ret                         ret
PiperOrigin-RevId: 276401386

--
0a3c4dfd8342bf2b1b11a87f1c662c883f73cab7 by Abseil Team <absl-team@google.com>:

Fix comment nit: sem_open => sem_init.

The code calls sem_init, not sem_open, to initialize an unnamed semaphore.
(sem_open creates or opens a named semaphore.)

PiperOrigin-RevId: 276344072

--
b36a664e9459057509a90e83d3482e1d3a4c44c7 by Abseil Team <absl-team@google.com>:

Fix typo in flat_hash_map.h: exchaged -> exchanged

PiperOrigin-RevId: 276295792

--
7bbd8d18276eb110c8335743e35fceb662ddf3d6 by Samuel Benzaquen <sbenza@google.com>:

Add assertions to verify use of iterators.

PiperOrigin-RevId: 276283300

--
677398a8ffcb1f59182cffe57a4fe7ff147a0404 by Laramie Leavitt <lar@google.com>:

Migrate distribution_impl.h/cc to generate_real.h/cc.

Combine the methods RandU64To<Float,Double> into a single method:
GenerateRealFromBits().

Remove rejection sampling from absl::uniform_real_distribution.

PiperOrigin-RevId: 276158675

--
c60c9d11d24b0c546329d998e78e15a84b3153f5 by Abseil Team <absl-team@google.com>:

Internal change

PiperOrigin-RevId: 276126962

--
4c840cab6a8d86efa29b397cafaf7520eece68cc by Andy Soffer <asoffer@google.com>:

Update CMakeLists.txt to address #365.
This does not cover every platform, but it does at least address the
first-order issue of assuming gcc implies x86.

PiperOrigin-RevId: 276116253

--
98da366e6b5d51afe5d7ac6722126aca23d85ee6 by Abseil Team <absl-team@google.com>:

Internal change

PiperOrigin-RevId: 276097452
GitOrigin-RevId: e54b9c7bbb0c58475676c268e2e19c69f4bce48a
Change-Id: I02d84454bb71ab21ad3d39650acf6cc6e36f58d7
@amrox
Copy link

amrox commented Oct 25, 2019

This is working for me as of 078b89b 🎉

@lilianmoraru
Copy link
Author

Looking at the changes, I think in my case it will throw the warning Value of CMAKE_SYSTEM_PROCESSOR (${CMAKE_SYSTEM_PROCESSOR}) is unknown and cannot be used to set ABSL_RANDOM_RANDEN_COPTS and not set any flags, which is fine with me.
I might set manually CMAKE_SYSTEM_PROCESSOR to arm, for the optimizations.
I will try to test it today and see if it compiles.

@lilianmoraru
Copy link
Author

Yes, I saw the warning message, but it works.
Closing this since it is fixed in the master branch.
Thank you!

@lilianmoraru
Copy link
Author

I observed that the check on CMAKE_SYSTEM_PROCESSOR is a bit "flaky".
People usually set it like this: set(CMAKE_SYSTEM_PROCESSOR ARM), which this check elseif("${CMAKE_SYSTEM_PROCESSOR}" STREQUAL "arm") would fail, because it is case sensitive and it would expect STREQUAL "ARM".
This is usually fixed with string(TOUPPER or string(TOLOWER and then checking on the new variable.

rongjiecomputer pushed a commit to rongjiecomputer/abseil-cpp that referenced this issue Oct 8, 2020
--
e54b9c7bbb0c58475676c268e2e19c69f4bce48a by Jorg Brown <jorg@google.com>:

Tweak ABSL_PREDICT_TRUE slightly, for better code on some platforms and/or
optimization levels.  "false || (x)" is more verbose than "!!(x)", but
ultimately more efficient.

For example, given this code:

void InitIfNecessary() {
  if (ABSL_PREDICT_TRUE(NeedsInit())) {
    SlowInitIfNecessary();
  }
}

Clang with default optimization level will produce:

Before this CL              After this CL
InitIfNecessary:            InitIfNecessary:
  push rbp                    push rbp
  mov  rbp, rsp               mov  rbp, rsp
  call NeedsInit              call NeedsInit
  xor  al, -1
  xor  al, -1
  test al, 1                  test al, 1
  jne  .LBB2_1                jne  .LBB3_1
  jmp  .LBB2_2                jmp  .LBB3_2
.LBB2_1:                    .LBB3_1:
  call SlowInitIfNecessary    call SlowInitIfNecessary
.LBB2_2:                    .LBB3_2:
  pop  rbp                    pop  rbp
  ret                         ret
PiperOrigin-RevId: 276401386

--
0a3c4dfd8342bf2b1b11a87f1c662c883f73cab7 by Abseil Team <absl-team@google.com>:

Fix comment nit: sem_open => sem_init.

The code calls sem_init, not sem_open, to initialize an unnamed semaphore.
(sem_open creates or opens a named semaphore.)

PiperOrigin-RevId: 276344072

--
b36a664e9459057509a90e83d3482e1d3a4c44c7 by Abseil Team <absl-team@google.com>:

Fix typo in flat_hash_map.h: exchaged -> exchanged

PiperOrigin-RevId: 276295792

--
7bbd8d18276eb110c8335743e35fceb662ddf3d6 by Samuel Benzaquen <sbenza@google.com>:

Add assertions to verify use of iterators.

PiperOrigin-RevId: 276283300

--
677398a8ffcb1f59182cffe57a4fe7ff147a0404 by Laramie Leavitt <lar@google.com>:

Migrate distribution_impl.h/cc to generate_real.h/cc.

Combine the methods RandU64To<Float,Double> into a single method:
GenerateRealFromBits().

Remove rejection sampling from absl::uniform_real_distribution.

PiperOrigin-RevId: 276158675

--
c60c9d11d24b0c546329d998e78e15a84b3153f5 by Abseil Team <absl-team@google.com>:

Internal change

PiperOrigin-RevId: 276126962

--
4c840cab6a8d86efa29b397cafaf7520eece68cc by Andy Soffer <asoffer@google.com>:

Update CMakeLists.txt to address abseil#365.
This does not cover every platform, but it does at least address the
first-order issue of assuming gcc implies x86.

PiperOrigin-RevId: 276116253

--
98da366e6b5d51afe5d7ac6722126aca23d85ee6 by Abseil Team <absl-team@google.com>:

Internal change

PiperOrigin-RevId: 276097452
GitOrigin-RevId: e54b9c7bbb0c58475676c268e2e19c69f4bce48a
Change-Id: I02d84454bb71ab21ad3d39650acf6cc6e36f58d7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants