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

cmake: blacklist GCC 7.0 and GCC 7.1 #1949

Merged
merged 1 commit into from Jun 29, 2017

Conversation

Projects
None yet
3 participants
@gregory38
Contributor

gregory38 commented May 21, 2017

@@ -70,6 +70,11 @@ function(check_compiler_version version_warn version_err)
endif()
endif()
if(GCC_VERSION VERSION_EQUAL "7.0" OR GCC_VERSION VERSION_EQUAL "7.1")
message(FATAL_ERROR "GCC 7.0/7.1 generates invalid code => https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80799\n"
"You can either backport the fix or swith to another version of GCC.")

This comment has been minimized.

@turtleli

turtleli May 22, 2017

Member

swith -> switch.

Is it a good idea to use FATAL_ERROR? I think this would still kill the build even if a patched GCC is used.

This comment has been minimized.

@orbea

orbea May 23, 2017

Contributor

I suggest this be a warning and then it disables the known bad optimizations as the current workaround already does. If a distro does patch it, they probably can also patch pcx2 to remove the workaround until it is no longer needed.

This comment has been minimized.

@gregory38

gregory38 May 23, 2017

Contributor

Nobody looks at the warning. I found it easier if distributions downgrade the ERROR into a warning (or remove it) when they know the GCC fix was back-ported.

Otherwise maybe, we can add a simple testcase to check the GCC behavior.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented May 25, 2017

@orbea I added a small testcase to ensure GCC correctness. Could you confirm me that it is able to detect the gcc bug. Thank you.

@orbea

This comment has been minimized.

Contributor

orbea commented Jun 3, 2017

@gregory38 Sorry for the slow reply, I was out of town.

I tested the PR and unfortunately it does not seem to work with cmake-3.8.2.

Cloning into 'pcsx2'...
remote: Counting objects: 119816, done.
remote: Compressing objects: 100% (62/62), done.
remote: Total 119816 (delta 32), reused 37 (delta 15), pack-reused 119738
Receiving objects: 100% (119816/119816), 87.66 MiB | 1.39 MiB/s, done.
Resolving deltas: 100% (91740/91740), done.
patching file plugins/GSdx/GSdx.cpp
Hunk #1 succeeded at 382 with fuzz 2 (offset 66 lines).
patching file cmake/Pcsx2Utils.cmake
patching file cmake/ApiValidation.cmake
patching file cmake/Pcsx2Utils.cmake
-- The C compiler identification is GNU 7.1.0
-- The CXX compiler identification is GNU 7.1.0
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Error at cmake/Pcsx2Utils.cmake:74 (GCC7_BUG):
  Unknown CMake command "GCC7_BUG".
Call Stack (most recent call first):
  CMakeLists.txt:54 (check_compiler_version)


-- Configuring incomplete, errors occurred!
See also "/tmp/SBo/pcsx2/build/CMakeFiles/CMakeOutput.log".
@gregory38

This comment has been minimized.

Contributor

gregory38 commented Jun 4, 2017

@orbea I moved the code to avoid order issue. Could you test it again? Thanks you

@orbea

This comment has been minimized.

Contributor

orbea commented Jun 5, 2017

I tested the new patch with a now patched gcc and it compiles and renders games correctly.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Jun 6, 2017

@orbea does the patch detect bad GCC ? I.e abort in Cmake because GCC doesn't work as expected.

@orbea

This comment has been minimized.

Contributor

orbea commented Jun 6, 2017

I rebuilt gcc without the patch and then built pcsx2, it unfortunately did not detect the bad GCC. To make sure I removed the previous workaround and tried again, as expected it built a broken package.

diff --git a/plugins/GSdx/CMakeLists.txt b/plugins/GSdx/CMakeLists.txt
index 6cc334dae..c5c7e0713 100644
--- a/plugins/GSdx/CMakeLists.txt
+++ b/plugins/GSdx/CMakeLists.txt
@@ -27,10 +27,6 @@ if(CMAKE_COMPILER_IS_GNUCXX)
 	if (${GCC_VERSION} VERSION_LESS "5.1")
 		set(GSdxFinalFlags ${GSdxFinalFlags} -fabi-version=6)
 	endif()
-	# github issue 1937
-	if (${GCC_VERSION} VERSION_GREATER "7.0")
-		set(GSdxFinalFlags ${GSdxFinalFlags} -fno-gcse-lm -fno-tree-pre)
-	endif()
 endif()
 
 if(XDG_STD)

My time to test further may be limited until early next week.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Jun 12, 2017

I made some improvements. I hope it will be enough. Otherwise I will need your help to get the generated binary to understand what is generated/what is happening.

Operation to get the binary

  • add --debug-trycompile option
  • use Cmake directly (not build.sh, otherwise you need to disable ninja)
  • Get the binary name from build_dev/CMakeFiles/CMakeTmp/Makefile
  • If you're lucky grep -o "cmTC_\w*" build_dev/CMakeFiles/CMakeTmp/Makefile | uniq
  • Send me the binary (example: build_dev/CMakeFiles/CMakeTmp/cmTC_8dad8)
@orbea

This comment has been minimized.

Contributor

orbea commented Jun 13, 2017

That seems better, with the broken gcc I get.

CMake Error at cmake/ApiValidation.cmake:122 (message):
  GCC 7.0/7.1 generates invalid code =>
  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80799

  You can either backport the fix or swith to another version of GCC.
Call Stack (most recent call first):
  cmake/SearchForStuff.cmake:199 (GCC7_BUG)
  CMakeLists.txt:61 (include)


-- Configuring incomplete, errors occurred!
See also "/tmp/SBo/pcsx2/build/CMakeFiles/CMakeOutput.log".

And with a working gcc I can compile a working package.

@orbea

This comment has been minimized.

Contributor

orbea commented Jun 13, 2017

I also suggest removing the previous work around in this PR as seen in this patch.

diff --git a/plugins/GSdx/CMakeLists.txt b/plugins/GSdx/CMakeLists.txt
index 6cc334dae..c5c7e0713 100644
--- a/plugins/GSdx/CMakeLists.txt
+++ b/plugins/GSdx/CMakeLists.txt
@@ -27,10 +27,6 @@ if(CMAKE_COMPILER_IS_GNUCXX)
 	if (${GCC_VERSION} VERSION_LESS "5.1")
 		set(GSdxFinalFlags ${GSdxFinalFlags} -fabi-version=6)
 	endif()
-	# github issue 1937
-	if (${GCC_VERSION} VERSION_GREATER "7.0")
-		set(GSdxFinalFlags ${GSdxFinalFlags} -fno-gcse-lm -fno-tree-pre)
-	endif()
 endif()
 
 if(XDG_STD)

That is how I tested this PR. Again my time to test may be limited again for several days.

@gregory38 gregory38 force-pushed the greg/blacklist-bad-gcc branch from a705e10 to ea2489c Jun 29, 2017

@gregory38 gregory38 merged commit 7d75a73 into master Jun 29, 2017

0 of 2 checks passed

continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
@gregory38

This comment has been minimized.

Contributor

gregory38 commented Jun 29, 2017

@orbea thanks you for all the tests 👍

@gregory38 gregory38 deleted the greg/blacklist-bad-gcc branch Jul 2, 2017

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