noloader / cryptopp-cmake

CMake files for Crypto++ project
BSD 3-Clause "New" or "Revised" License
92 stars 68 forks source link

Add conditional compilation of aes-armv4.S #15

Closed noloader closed 6 years ago

noloader commented 6 years ago

Due to Issue 683 and Commit 3ff7d7f0286a we need to add conditional compilation of aes-armv4.S for CMake.

The makefile has the following:

IS_ARM32 := $(shell echo "$(HOSTX)" | $(GREP) -i -c -E 'arm|armhf|arm7l|eabihf')
...

ifeq ($(IS_ARM32),1)
    CRYPTOGAMS_AES_ARCH = -march=armv7-a -marm
    SRCS += aes-armv4.S
endif
...

ifeq ($(IS_ARM32),1)
aes-armv4.o : aes-armv4.S
    $(CC) $(strip $(CXXFLAGS) $(CRYPTOGAMS_AES_ARCH) -mfloat-abi=$(FP_ABI) -c) $<
endif

The recipe should only be in effect for ARM A-32 systems with GNU AS (GAS). ARM Ltd's assembler may be able to assemble it too, but I have not tested it. The recipe should not be in effect for Aarch32, Aarch64, MIPS, x86_64, Windows, etc.

A similar rule was already added to the Autotools project.

This task is open to any takers. I've had enough fun with jobs like this. Someone else will have to {enjoy|suffer} it.

abdes commented 6 years ago

For keeping up some records, to setup the cross compilation env for ARM on Ubuntu trusty:

sudo apt install git cmake
sudo apt install gcc-arm-linux-gnueabi binutils-arm-linux-gnueabi
sudo apt install g++-arm-linux-gnueabi

Use a custom toolchain with cmake (credit to https://github.com/vpetrigo/arm-cmake-toolchains):

set(CMAKE_SYSTEM_NAME Generic)
set(CMAKE_SYSTEM_PROCESSOR ARM)

if(MINGW OR CYGWIN OR WIN32)
    set(UTIL_SEARCH_CMD where)
elseif(UNIX OR APPLE)
    set(UTIL_SEARCH_CMD which)
endif()

#set(TOOLCHAIN_PREFIX arm-none-eabi-)
set(TOOLCHAIN_PREFIX arm-linux-gnueabi-)

execute_process(
  COMMAND ${UTIL_SEARCH_CMD} ${TOOLCHAIN_PREFIX}gcc
  OUTPUT_VARIABLE BINUTILS_PATH
  OUTPUT_STRIP_TRAILING_WHITESPACE
)

get_filename_component(ARM_TOOLCHAIN_DIR ${BINUTILS_PATH} DIRECTORY)
# Without that flag CMake is not able to pass test compilation check
set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)

set(CMAKE_C_COMPILER ${TOOLCHAIN_PREFIX}gcc)
set(CMAKE_ASM_COMPILER ${CMAKE_C_COMPILER})
set(CMAKE_CXX_COMPILER ${TOOLCHAIN_PREFIX}g++)

set(CMAKE_OBJCOPY ${ARM_TOOLCHAIN_DIR}/${TOOLCHAIN_PREFIX}objcopy CACHE INTERNAL "objcopy tool")
set(CMAKE_SIZE_UTIL ${ARM_TOOLCHAIN_DIR}/${TOOLCHAIN_PREFIX}size CACHE INTERNAL "size tool")

set(CMAKE_FIND_ROOT_PATH ${BINUTILS_PATH})
set(CMAKE_FIND_ROOT_PATH_MODE_PROGRAM NEVER)
set(CMAKE_FIND_ROOT_PATH_MODE_LIBRARY ONLY)
set(CMAKE_FIND_ROOT_PATH_MODE_INCLUDE ONLY)

From the build directory, run cmake:

cmake -DCMAKE_TOOLCHAIN_FILE=../arm-gcc-toolchain.cmake -DBUILD_SHARED=OFF ..
cmake --build .

ARM does not support shared libs so we make it simple, avoid a warning and just request static lib.

noloader commented 6 years ago

ARM does not support shared libs so we make it simple, avoid a warning and just request static lib.

Hmmm... I'm not sure about that.

The cross-compile support is good. It gave us a lot of problems in the past.

I will be testing locally on an ARM dev-board, like a BananaPi, CubieTruckv5 or Wandboard. They are my Cortex-A7 through Cortex-A9 test platforms.

If possible, please test Windows Phone to ensure something does not get knocked loose.

abdes commented 6 years ago
-- SRC_DIR: /home/abdes/projects/cryptopp-test/cryptopp
CMake Warning (dev) at cryptopp/CMakeLists.txt:731 (add_library):
  ADD_LIBRARY called with SHARED option but the target platform does not
  support dynamic linking.  Building a STATIC library instead.  This may lead
  to problems.
This warning is for project developers.  Use -Wno-dev to suppress it.

By the way the asm files for aes-armv4.S are not in the 7.0.0 :-)

noloader commented 6 years ago

By the way the asm files for aes-armv4.S are not in the 7.0.0

It was added earlier this week at Issue 683 and Commit 3ff7d7f0286a. As new files are added to Master they get added to Autotools and Cmake, too.

CMake Warning (dev) at cryptopp/CMakeLists.txt:731 (add_library):
  ADD_LIBRARY called with SHARED option but the target platform does not
  support dynamic linking.  Building a STATIC library instead.  This may lead
  to problems.
This warning is for project developers.  Use -Wno-dev to suppress it.

Sorry, I have to admit I don't quite understand the message. ARM surely supports shared libraries. I test it through the GNUmakefile.

Maybe something is cross-pollinating from iOS? (iOS up to about iOS 8 did not allow dynamic libs).

abdes commented 6 years ago

It was added earlier this week at Issue 683 and Commit 3ff7d7f0286a.

still not released though, and the master branch has many new changes that are not ready to compile with the current CMakeLists.txt - If you really want to use it, you're gonna have to cherry pick it in your cryptopp sources, and we will probably need to test for the file presence in the CMakeLists to not break the build of just 7.0.0.

Sorry, I have to admit I don't quite understand the message. ARM surely supports shared libraries. Maybe something is cross-pollinating from iOS? (iOS up to about iOS 8 did not allow dynamic libs).

Maybe this is a limitation of the cross compilation environment. Anyway, I don't have anything to test ARM with, so I will only be able to help you with pure cmake knowledge 😄

abdes commented 6 years ago

Here is what I suggest for using the aes-armv4.S. Note that this file is compiled with the C compiler not the ASM compiler.

diff --git a/CMakeLists.txt b/CMakeLists.txt
index dd18112..07aaae4 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -490,6 +490,11 @@ if (cryptopp_SOURCES_ASM)
   set_source_files_properties(${cryptopp_SOURCES_ASM} PROPERTIES LANGUAGE ASM)
 endif ()

+if ((CRYPTOPP_ARM OR CRYPTOPP_ARMHF) AND EXISTS "${SRC_DIR}/aes-armv4.S")
+  list(APPEND cryptopp_SOURCES ${SRC_DIR}/aes-armv4.S)
+  set_source_files_properties(${SRC_DIR}/aes-armv4.S PROPERTIES LANGUAGE C)
+endif ()
+
 #============================================================================
 # Architecture flags
 #============================================================================
@@ -601,6 +606,7 @@ if (CMAKE_CXX_COMPILER_ID MATCHES "Clang" OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU

     if (CRYPTOPP_ARMV7A_HARD AND CRYPTOPP_ARMV7A_NEON)
       # Need to set floating point ABI to something, like "hard" of "softfp". Most Linux use hard floats ("hard").
+      set_source_files_properties(${SRC_DIR}/aes-armv4.S PROPERTIES COMPILE_FLAGS "-march=armv7-a -mfloat-abi=hard -mfpu=neon")
       set_source_files_properties(${SRC_DIR}/aria-simd.cpp PROPERTIES COMPILE_FLAGS "-march=armv7-a -mfloat-abi=hard -mfpu=neon")
       set_source_files_properties(${SRC_DIR}/blake2-simd.cpp PROPERTIES COMPILE_FLAGS "-march=armv7-a -mfloat-abi=hard -mfpu=neon")
       set_source_files_properties(${SRC_DIR}/cham-simd.cpp PROPERTIES COMPILE_FLAGS "-march=armv7-a -mfloat-abi=hard -mfpu=neon")
@@ -613,6 +619,7 @@ if (CMAKE_CXX_COMPILER_ID MATCHES "Clang" OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU
       set_source_files_properties(${SRC_DIR}/speck-simd.cpp PROPERTIES COMPILE_FLAGS "-mfloat-abi=hard -mfpu=neon")
     elseif (CRYPTOPP_ARMV7A_SOFTFP AND CRYPTOPP_ARMV7A_NEON)
       # Need to set floating point ABI to something, like "hard" of "softfp". Most Linux use hard floats ("hard").
+      set_source_files_properties(${SRC_DIR}/aes-armv4.S PROPERTIES COMPILE_FLAGS "-march=armv7-a -mfloat-abi=softfp -mfpu=neon")
       set_source_files_properties(${SRC_DIR}/aria-simd.cpp PROPERTIES COMPILE_FLAGS "-march=armv7-a -mfloat-abi=softfp -mfpu=neon")
       set_source_files_properties(${SRC_DIR}/blake2-simd.cpp PROPERTIES COMPILE_FLAGS "-march=armv7-a -mfloat-abi=softfp -mfpu=neon")
       set_source_files_properties(${SRC_DIR}/cham-simd.cpp PROPERTIES COMPILE_FLAGS "-march=armv7-a -mfloat-abi=softfp -mfpu=neon")

Then simply pick it up from the commit/master and add it to a 7.0.0 checkout.

noloader commented 6 years ago

still not released though, and the master branch has many new changes that are not ready to compile with the current CMakeLists.txt

As far as I know the only thing out-of-sync is aes-armv4.S. Everything in Master has been added to CmakeList.txt.

If you really want to use it, you're gonna have to cherry pick it in your cryptopp sources, and we will probably need to test for the file presence in the CMakeLists to not break the build of just 7.0.0.

The idea is we keep CmakeList.txt in-sync with Master. It is easier to delete unneeded things than it is to add needed things. For example, a 7.0 user can delete CHAM or LEA. It takes no thought to perform a delete. In fact, they probably won't notice the additional support and won't need to delete.

However, going the other way is much more difficult. Adding CHAM or LEA support takes some learning. We've added so many files like cham-simd.cpp and lea-simd.cpp it only takes minutes to do. Someone unfamiliar with the process will waste a lot of time learning what to do.

I don't have anything to test ARM with

Email me at noloader, gmail address. I'll send you an ARM dev board for testing. I'll need your postal mailing address. A BeagleBone or Banana Pi should fill the ARM A-32 gap.

noloader commented 6 years ago

@abdes,

still not released though, and the master branch has many new changes that are not ready to compile with the current CMakeLists.txt

I added tags to Automake and CMake projects. The tags are the same as Crypto++ but the dates may differ. For example, we may release Crypto++ on a Monday and tag CMake several days later after we test CMake with the release.

You can see an example of it at Crypto++ Wiki | OpenCSW.

noloader commented 6 years ago

Cleared at Commit 3659174b3520.