Skip to content

[clang][cmake] Don't pass -fno-strict-aliasing for GCC #144222

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

Merged
merged 1 commit into from
Jun 15, 2025

Conversation

thesamesam
Copy link
Member

This was added a long time ago..

  • to the Makefiles in 40fee63;
  • first to CMake in b3ce035;
  • then moved to only apply when building Clang with GCC in c5635a6.

This shouldn't be needed these days. If an issue does arise, it really ought to be documented better and the cause will certainly be different than it was back then.

The two GCC bugs cited in 40fee63 were:

  • https://gcc.gnu.org/PR41874
  • https://gcc.gnu.org/PR41838 and both are long-fixed. Not only that, if those issues did come up again, we'd be better off doing -Wno-strict-aliasing where appropriate if there weren't a real code issue or some suppression that was tighter in scope wasn't appropriate.

@llvmbot llvmbot added the clang Clang issues not falling into any other category label Jun 14, 2025
@thesamesam thesamesam requested a review from nikic June 14, 2025 13:13
@llvmbot
Copy link
Member

llvmbot commented Jun 14, 2025

@llvm/pr-subscribers-clang

Author: Sam James (thesamesam)

Changes

This was added a long time ago..

  • to the Makefiles in 40fee63;
  • first to CMake in b3ce035;
  • then moved to only apply when building Clang with GCC in c5635a6.

This shouldn't be needed these days. If an issue does arise, it really ought to be documented better and the cause will certainly be different than it was back then.

The two GCC bugs cited in 40fee63 were:

  • https://gcc.gnu.org/PR41874
  • https://gcc.gnu.org/PR41838 and both are long-fixed. Not only that, if those issues did come up again, we'd be better off doing -Wno-strict-aliasing where appropriate if there weren't a real code issue or some suppression that was tighter in scope wasn't appropriate.

Full diff: https://github.com/llvm/llvm-project/pull/144222.diff

1 Files Affected:

  • (modified) clang/CMakeLists.txt (-3)
diff --git a/clang/CMakeLists.txt b/clang/CMakeLists.txt
index ab2ac9bc6b9ad..94607a8e8473c 100644
--- a/clang/CMakeLists.txt
+++ b/clang/CMakeLists.txt
@@ -345,9 +345,6 @@ configure_file(
 # Add appropriate flags for GCC
 if (LLVM_COMPILER_IS_GCC_COMPATIBLE)
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-common -Woverloaded-virtual")
-  if (NOT "${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")
-    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-strict-aliasing")
-  endif ()
 
   # Enable -pedantic for Clang even if it's not enabled for LLVM.
   if (NOT LLVM_ENABLE_PEDANTIC)

@thesamesam
Copy link
Member Author

FWIW, there seems to be similar code in Flang which was likely copied from Clang, but I'm not in a position to test removing that right now.

@nikic
Copy link
Contributor

nikic commented Jun 14, 2025

To confirm, you do not get any -Wstrict-aliasing warnings with modern GCC after this change?

Copy link
Member

@MaskRay MaskRay left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for removing this tech debt.

This was added a long time ago..
* to the Makefiles in 40fee63;
* first to CMake in b3ce035;
* then moved to only apply when building Clang with GCC in
  c5635a6.

This shouldn't be needed these days. If an issue does arise, it really
ought to be documented better and the cause will certainly be different
than it was back then.

The two GCC bugs cited in 40fee63 were:
* https://gcc.gnu.org/PR41874
* https://gcc.gnu.org/PR41838
and both are long-fixed. Not only that, if those issues did come up again,
we'd be better off doing -Wno-strict-aliasing where appropriate if there
weren't a real code issue or some suppression that was tighter in scope
wasn't appropriate.
@thesamesam thesamesam merged commit 4ed10db into llvm:main Jun 15, 2025
5 of 7 checks passed
@thesamesam thesamesam deleted the aliasing-gcc branch June 15, 2025 01:35
@thesamesam
Copy link
Member Author

Huh, I thought I'd written a reply to nikic but I don't see it here? Anyway, rewriting it..

To confirm, you do not get any -Wstrict-aliasing warnings with modern GCC after this change?

Yes. Sorry, it's obvious in hindsight I should've stated this explicitly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants