This is an archive of the discontinued LLVM Phabricator instance.

We shouldn't need to pass -fno-strict-aliasing when building clang with clang.
ClosedPublic

Authored by beanz on Aug 14 2015, 10:39 AM.

Details

Summary

The code comments in the Makefile indicate this was put in place to support issues when building clang with GCC. Today clang's strict aliasing works, so we shouldn't pass -fno-strict-aliasing when building with clang.

Diff Detail

Repository
rL LLVM

Event Timeline

beanz updated this revision to Diff 32161.Aug 14 2015, 10:39 AM
beanz retitled this revision from to We shouldn't need to pass -fno-strict-aliasing when building clang with clang..
beanz updated this object.
beanz added reviewers: bogner, echristo.
beanz added a subscriber: cfe-commits.
beanz updated this revision to Diff 32163.Aug 14 2015, 10:41 AM

Should have been calling $(CC) not cc in the makefile. Oops.

echristo edited edge metadata.Aug 14 2015, 10:50 AM
echristo added a subscriber: echristo.

This is fine, but would you mind moving it to the cmake and autoconf checks
respectively? Probably near the warning checks where we already check for
compiler.

Have you tested this? I assume there are strict aliasing violations in
Clang/LLVM if we've had this turned on for a while.

(does strict aliasing still have the special case for enum type punning?
Becaues I've certainly seen (& fixed) that in a few places in Clang/LLVM)

dblaikie,

I've run this through check-all with no issues.

echristo,

I can move the makefile check to the autoconf check, but I think setting the flags should stay in the clang CMakeLists/Makefile otherwise it will change behavior. Today we only enable this flag for the clang subdirectory, not all of LLVM.

Thoughts?

thakis added a subscriber: thakis.Aug 14 2015, 11:44 PM
thakis added inline comments.
Makefile
68 ↗(On Diff #32163)

The first of these is marked fixed, the second has a "works on trunk" comment (from 2010). Maybe this isn't needed with gcc anymore either?

beanz updated this revision to Diff 32349.Aug 17 2015, 3:53 PM
beanz edited edge metadata.
  • Adapting to r245255 which exposes a variable in autoconf for which compiler you are building with.
  • Updated the CMake to be more CMake-y

Note: This has been tested by running a check on a stage2 clang built with asan and ubsan.

echristo accepted this revision.Aug 17 2015, 5:43 PM
echristo edited edge metadata.

Sure I guess. I'm still not sure why we can't do this in the configure side of things, but I don't really care that much for now.

This revision is now accepted and ready to land.Aug 17 2015, 5:43 PM
This revision was automatically updated to reflect the committed changes.