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

Pass -fstack-protector-strong to linker #1915

Merged
merged 1 commit into from
Sep 20, 2021
Merged

Pass -fstack-protector-strong to linker #1915

merged 1 commit into from
Sep 20, 2021

Conversation

evanmiller
Copy link
Contributor

Fixes some older compiler / platform combinations (e.g. GCC7 on PPC Mac)

Fixes some older compiler / platform combinations (e.g. GCC7 on PPC Mac)
Copy link
Collaborator

@clanmills clanmills left a comment

Choose a reason for hiding this comment

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

This is an interesting find. I had trouble with -fstack-clash-protection on the new "Apple Silicon" which is the M1/arm64 chip. Using add_link_options fixes this correctly. The hack I applied was to evade -fstack-clash-protection on the M1. See: #1857

I've retired from Exiv2 and don't want to get sucked into deciding what to do about this on the 0.27-maintenance branch. I'll leave somebody else to approve your PR or request changes. We expect to release 0.27.5 on 2021-09-30, so let's avoid changing 0.27-maintenance until 0.27.5 has shipped.

558 rmills@rmillsm1:~/gnu/github/exiv2/0.27-maintenance/build $ git diff
diff --git a/cmake/compilerFlags.cmake b/cmake/compilerFlags.cmake
index 57fb575..713e4ad 100644
--- a/cmake/compilerFlags.cmake
+++ b/cmake/compilerFlags.cmake
@@ -25,21 +25,19 @@ if ( MINGW OR UNIX OR MSYS ) # MINGW, Linux, APPLE, CYGWIN
     if (COMPILER_IS_GCC OR COMPILER_IS_CLANG)
         # This fails under Fedora - MinGW - Gcc 8.3 and macOS/M1
         if (NOT (MINGW OR CYGWIN OR CMAKE_HOST_SOLARIS))
-            # macOS M1 will set ARCHITECTURE == arm64
-            EXECUTE_PROCESS( COMMAND uname -m COMMAND tr -d '\n' OUTPUT_VARIABLE ARCHITECTURE )
-            if ( NOT ${ARCHITECTURE} STREQUAL arm64 )
-                check_cxx_compiler_flag(-fstack-clash-protection HAS_FSTACK_CLASH_PROTECTION)
-            endif()
             check_cxx_compiler_flag(-fcf-protection HAS_FCF_PROTECTION)
             check_cxx_compiler_flag(-fstack-protector-strong HAS_FSTACK_PROTECTOR_STRONG)
             if(HAS_FSTACK_CLASH_PROTECTION)
                 add_compile_options(-fstack-clash-protection)
+                add_link_options(-fstack-clash-protection)^M
             endif()
             if(HAS_FCF_PROTECTION)
                 add_compile_options(-fcf-protection)
+                add_link_options(-fcf-protection)^M
             endif()
             if(HAS_FSTACK_PROTECTOR_STRONG)
                 add_compile_options(-fstack-protector-strong)
+                add_link_options(-fstack-protector-strong)^M
             endif()
         endif()
 
559 rmills@rmillsm1:~/gnu/github/exiv2/0.27-maintenance/build $ 

@evanmiller
Copy link
Contributor Author

I had trouble with -fstack-clash-protection on the new "Apple Silicon" which is the M1/arm64 chip. Using add_link_options fixes this correctly.

What's old is new again... interesting! Glad this insight might help to clean up the CMake script. It's been a recurring issue with CMake-based projects on my test machine, so I've learned to diagnose it quickly.

@codecov
Copy link

codecov bot commented Sep 19, 2021

Codecov Report

Merging #1915 (7700f90) into main (d92b0a6) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1915   +/-   ##
=======================================
  Coverage   60.87%   60.87%           
=======================================
  Files          96       96           
  Lines       19043    19043           
  Branches     9727     9727           
=======================================
  Hits        11593    11593           
  Misses       5138     5138           
  Partials     2312     2312           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d92b0a6...7700f90. Read the comment docs.

@clanmills
Copy link
Collaborator

There's another arm64 oddity. #1903. As you can see, I closed it and blamed Apple. Maybe you can suggest a fix.

Your PR concerning libproc refers to the time when dinosaurs roamed the earth. I have a 2003 32-bit PPC MBP with GCC 4.0. The last time it built Exiv2, it took about 30 minutes (v0.26, 2017 I think). The m1 takes 30 seconds.

@evanmiller
Copy link
Contributor Author

There's another arm64 oddity. #1903. As you can see, I closed it and blamed Apple. Maybe you can suggest a fix.

The linked symbols don't look like stock iconv symbols, which should be _iconv, _iconv_open, etc – unless this is some kind of C++ symbol mangling I'm unaware of. I've had no issue linking to the SDK iconv on an M1 in other projects. Do you have another libiconv installed locally that's interfering with your build?

Your PR concerning libproc refers to the time when dinosaurs roamed the earth.

Some of us choose to hunt game in Jurassic Park :-). MacPorts has GCC7.5 and GTK+3 running on 32-bit PowerPC, so it's possible to get all kinds of OSS running on that platform. I've been slowly building up a 700MHz G3 system from 2001... it's almost usable with TenFourFox and an SSD drive in there. (Assuming you don't mind the occasional overnight compile...)

@clanmills
Copy link
Collaborator

Thanks for such a quick response. That's interesting that you don't have trouble with M1/libiconv with other libraries.

Yes, I do have libiconv locally (in /usr/local/lib). cmake know this and declares it in the transcript. make uses libiconv from the SDK. For reasons that I have forgotten, README.md advocates to build/install libiconv-1.16 from source.

@evanmiller
Copy link
Contributor Author

Yes, I do have libiconv locally (in /usr/local/lib).

I would ensure that libiconv is a universal build, or else just delete it and link to Apple's iconv. I had some /usr/local linking issues when I first migrated from x86 to M1. Migration Assistant does not recompile dynamic libraries, alas!

@clanmills
Copy link
Collaborator

You're got that 100% right! I've uninstalled libiconv from /usr/local/lib and now cmake finds and uses Apple's iconv:

-- Found Iconv: /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk/usr/lib/libiconv.tbd  usr/lib/libiconv.tbd  
-- ICONV_INCLUDE_DIR : /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk/usr/include
-- ICONV_LIBRARIES : /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk/usr/lib/libiconv.tbd

When I got the M1, I had a frustrating morning getting up and running. I used the Migration Assistant. I removed all the libraries in /usr/local/lib and rebuilt them from source and everything was working fine. I haven't tried your suggestion to build a universal iconv in /usr/local/lib.

I've looked at the discussion about libiconv in README.md and it recommends building 1.16 from source for visual studio users. I'm not going to modify that. I will update #1903 with our conclusion and leave that closed.

Thank You very much for your contribution and insights. Very helpful.

@kevinbackhouse kevinbackhouse added this to the v1.00 milestone Sep 20, 2021
@kevinbackhouse kevinbackhouse added CMake Configuration issues related with CMake build labels Sep 20, 2021
@kevinbackhouse kevinbackhouse merged commit 66b40d8 into Exiv2:main Sep 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build CMake Configuration issues related with CMake
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants