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

Fix bad configure tests that breaks homebrew/clang #42737

Closed
wants to merge 1 commit into from

Conversation

omajid
Copy link
Member

@omajid omajid commented Sep 25, 2020

The clang script that's included with homebrew gets confused with the -c flag and returns a wrong result for some configure tests. The -c flag is not needed for this check.

See dotnet/source-build#1744 (comment) and dotnet/source-build#1744 (comment) for more details.

@janvorli found the bug and came up with the fix. I am just trying to get it merged here.

The clang script that's included with homebrew gets confused with the -c
flag and returns a wrong result for some configure tests. The -c flag is
not needed for this check.

See
dotnet/source-build#1744 (comment)
and
dotnet/source-build#1744 (comment)
for more details.
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@janvorli
Copy link
Member

There seems to be an unexpected problem with this change. The ARM/ARM64 test legs for coreclr and libraries are failing in huge amount of tests with an assert in gcinfo and looking at the test failure history, it seems it was not happening before.

@janvorli
Copy link
Member

Looking at the output of the configure step, there is a difference for arm now:

From this PR:
2020-09-25T15:32:31.9773595Z -- Performing Test UNWIND_CONTEXT_IS_UCONTEXT_T
2020-09-25T15:32:32.0428058Z -- Performing Test UNWIND_CONTEXT_IS_UCONTEXT_T - Failed
2020-09-25T15:32:32.0429588Z -- Performing Test HAVE_UNW_GET_SAVE_LOC
2020-09-25T15:32:32.1488253Z -- Performing Test HAVE_UNW_GET_SAVE_LOC - Failed
2020-09-25T15:32:32.1490687Z -- Performing Test HAVE_UNW_GET_ACCESSORS
2020-09-25T15:32:32.2555597Z -- Performing Test HAVE_UNW_GET_ACCESSORS - Failed

From another PR:
2020-09-23T12:01:11.8846230Z -- Performing Test UNWIND_CONTEXT_IS_UCONTEXT_T
2020-09-23T12:01:11.9852929Z -- Performing Test UNWIND_CONTEXT_IS_UCONTEXT_T - Failed
2020-09-23T12:01:11.9854767Z -- Performing Test HAVE_UNW_GET_SAVE_LOC
2020-09-23T12:01:12.0818957Z -- Performing Test HAVE_UNW_GET_SAVE_LOC - Success
2020-09-23T12:01:12.0819996Z -- Performing Test HAVE_UNW_GET_ACCESSORS
2020-09-23T12:01:12.1788800Z -- Performing Test HAVE_UNW_GET_ACCESSORS - Success

I'll run the cross build for arm locally to check why those checks are failing, as they should succeed.

@janvorli
Copy link
Member

I've found the problem. On arm and arm64, the check_c_source_compiles fails to link the resulting executable due to some missing symbols that the methods we check for call (they are likely implemented in the header files for arm/arm64). I have forgotten that check_c_source_compiles also links and not just compiles. The problem with fixing that though is that for the case when we use our copy of libunwind, we do not have the library at the configure time to pass to the linker, hence we have a chicken and egg problem.
Fortunately, we can use check_symbol_exists for the two symbols which doesn't do any linking. Let me create a new PR with that fix. I've verified it works on Linux arm and on OSX too.

@omajid
Copy link
Member Author

omajid commented Sep 25, 2020

Thanks for digging into this!

Let me create a new PR with that fix. I've verified it works on Linux arm and on OSX too.

Shall I close this PR then?

@janvorli
Copy link
Member

I will close it.

@janvorli janvorli closed this Sep 25, 2020
@asbjornu
Copy link
Member

asbjornu commented Oct 1, 2020

I feel like I've come to a dead-end here as I can't find the "new PR with that fix". Can you please link me to it?

@janvorli
Copy link
Member

janvorli commented Oct 1, 2020

@asbjornu here you go: #42756

@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants