Skip to content

Commit

Permalink
fix broken compilation under ARM. tested with my Raspberry PIV and Ra…
Browse files Browse the repository at this point in the history 
…spbian 64bits.
  • Loading branch information
@cgilles @piponazo
cgilles authored and piponazo committed Jun 9, 2020
1 parent 356f862 commit 1ea63cc
Showing 1 changed file with 5 additions and 1 deletion.
6 changes: 5 additions & 1 deletion cmake/compilerFlags.cmake
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,11 @@ if ( MINGW OR UNIX OR MSYS ) # MINGW, Linux, APPLE, CYGWIN
# This fails under Fedora - MinGW - Gcc 8.3
if (NOT MINGW)
if (COMPILER_IS_GCC AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 8.0)
add_compile_options(-fstack-clash-protection -fcf-protection)
if (NOT ${CMAKE_SYSTEM_PROCESSOR} MATCHES "arm")
add_compile_options(-fstack-clash-protection -fcf-protection)
else()
add_compile_options(-fstack-clash-protection)
endif()
endif()

if (COMPILER_IS_GCC OR (COMPILER_IS_CLANG AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 3.7 ))
Expand Down

4 comments on commit 1ea63cc

@tpetazzoni
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really isn't the best fix: other architectures also may not support -fcf-protection

@clanmills
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. This is not correct. It would be much better to use check_c_compiler_flag (or similar) to test flags. We shouldn't use GCC/Clang version numbers in the CMake code.

This was discussed in #1243 and I submitted a PR: #1245 Regrettfully, I am very dyslexic and find it almost impossible to do such a change without introducing new errors. Another concern is that I don't have a test machine for either ARM or PPC.

I would very much appreciate some help to get #1245 sorted out and into the 0.27-maintenance branch. And after that, the changes should be ported and tested on PPC and ARM - and any other new architectures which are being discussed.

@ryandesign @piponazo @tpetazzoni Can you help to get this sorted out please.

@tpetazzoni
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't seen PR #1245, so I have submitted yet another PR doing more of less the same thing.

Please note that you don't need an ARM or PPC machine to reproduce the issue: you can simply use a cross-compiler on your x86/x86-64 machine, and you would get the -fcf-protection build failure. You can find plenty of ready-to-use cross compilers in most Linux distributions, or otherwise at https://toolchains.bootlin.com. It would probably make sense to have a few of these cross-compilers used in your CI to detect such regressions early on.

@clanmills
Copy link
Collaborator

@clanmills clanmills commented on 1ea63cc Jul 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tpetazzoni Can you look at the review of #1243 and see if you've fallen into the traps into which I fell.

I'd like to retire because I'll be 70 in January. I don't want to get sucked into ARM,PPC, cross-compiling and testing cross-compiled code. All unfamiliar territory. I'd like to see this on 0.27-maintenance as well as 'master'. I maintain Exiv2 v0.27 which shipped in December 2018. Since then I've maintained the 0.27-maintenance branch and shipped 3 "dots". The most recent release was 0.27.3 on 2020-06-30.

I am working on writing a book entitled Image Metadata and Exiv2 Architecture. My aim is the write down everything I know about Exiv2 to make it possible to maintain it in future, or even better to write a new metadata library. Here's today's version: https://clanmills.com/exiv2/book/

Please sign in to comment.