-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
gh-96398: Improve accuracy of compiler checks in configure.ac #117815
base: main
Are you sure you want to change the base?
Conversation
cc. @ronaldoussoren and @ned-deily |
This comment was marked as outdated.
This comment was marked as outdated.
In order to solve the WASI error, I propose to introduce a "GCC compatible" check, and use that as a gate to the compiler flag checks. UPDATE: proposed in #117825 |
Introduce a cached variable $ac_cv_gcc_compat and set it to 'yes' if the C preprocessor defines the __GNUC__ macro.
@igalic: I did not spot the question; did it get lost when you posted the comment? |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
a less bleary eyed review reveals that I was reading the LTO code wrong. not sure why my question didn't post at all, tho |
This comment was marked as resolved.
This comment was marked as resolved.
@@ -1845,7 +1847,7 @@ AC_MSG_RESULT([$PROFILE_TASK]) | |||
|
|||
llvm_bin_dir='' | |||
llvm_path="${PATH}" | |||
if test "${CC}" = "clang" | |||
if test "${ac_cv_cc_name}" = "clang" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using $CC_BASENAME here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC_BASENAME may contain a platform triplet, so ac_cv_cc_name seems better.
For the same reason, when the following code does clang_bin=`which clang`
, shouldn't that be clang_bin=$CC
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the same reason, when the following code does
clang_bin=`which clang`
, shouldn't that beclang_bin=$CC
?
I think I'd like to make that change in a follow-up PR.
configure.ac
Outdated
case "$CC_BASENAME" in | ||
gcc) AC_PATH_TOOL([CXX], [g++], [g++], [notfound]) ;; | ||
cc) AC_PATH_TOOL([CXX], [c++], [c++], [notfound]) ;; | ||
clang|*/clang) AC_PATH_TOOL([CXX], [clang++], [clang++], [notfound]) ;; | ||
icc|*/icc) AC_PATH_TOOL([CXX], [icpc], [icpc], [notfound]) ;; | ||
clang) AC_PATH_TOOL([CXX], [clang++], [clang++], [notfound]) ;; | ||
icc) AC_PATH_TOOL([CXX], [icpc], [icpc], [notfound]) ;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to correspond to the definition of AC_PATH_TOOL:
AC_PATH_TOOL (variable, prog-to-check-for, [value-if-not-found], [path = ‘$PATH’])
- variable = CXX
- prog-to-check-for = g++
- value-if-not-found = g++ (?)
- path = notfound (??)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, CC_BASENAME may contain a platform triplet, so shouldn't this use ac_cv_cc_name instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, CC_BASENAME may contain a platform triplet, so shouldn't this use ac_cv_cc_name instead?
Yes, this should use ac_cv_cc_name
, not CC_BASENAME
.
This doesn't seem to correspond to the definition of AC_PATH_TOOL:
Indeed; I suggest we look at this in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, CC_BASENAME may contain a platform triplet, so shouldn't this use ac_cv_cc_name instead?
Addressed with 9900633.
FWIW: @mhsmith pinged me on this PR, because of the potential for clash with the iOS compiler shims (around L400 of configure.ac). I've confirmed that iOS builds still work with these changes. |
This comment was marked as outdated.
This comment was marked as outdated.
It seems there are refleaks on In the mean time, feel free to take another look, if needed. |
🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit d309996 🤖 If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 9900633 🤖 If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
Some checks require a GCC compatible compiler; use $ac_cv_gcc_compat for these.
Some checks require checking the compiler basename; use $CC_BASENAME for these.
For the rest, use $ac_cv_cc_name.