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

[release/6.0] Define __cpuid{ex} only when there's no builtin one (backport of #73065) #77507

Merged

Conversation

ayakael
Copy link
Contributor

@ayakael ayakael commented Oct 26, 2022

Customer impact

Without this, build with clang15 fails.

Testing

  • unit tests in ci
  • source-build build within Alpine environment passes

Risk

Low, as it is already in main, and only activates when these variables aren't defined


Backport of #73065 and #73905

Per @am11:

Fix clang 15 RC1 build: error: definition of builtin function '__cpuid'.
Also add clang 15 autodetection; upstream PR https://github.com/dotnet/arcade/pull/10199.
* gcc < v10 does not define the __has_builtin macro which I missed in unixstubs.cpp when making the previous change: https://github.com/dotnet/runtime/commit/992cf8c97cc71d4ca9a0a11e6604a6716ed4cefc. Some community legs like illumos use gcc v8.4 which were failing.
* gcc v10+ supports __has_builtin macro which was being skipped due to version based conditions and we were forcing the fallback code path.
This PR moves __has_builtin macro check in two root headers which cover all the sources that are using it (pal.h and gcenv.base.h)

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 26, 2022
@ghost
Copy link

ghost commented Oct 26, 2022

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #73065

Per @am11:

Fix clang 15 RC1 build: error: definition of builtin function '__cpuid'.
Also add clang 15 autodetection; upstream PR https://github.com/dotnet/arcade/pull/10199.
Author: ayakael
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@am11
Copy link
Member

am11 commented Oct 27, 2022

There was a problem in this patch with old gcc versions. It was fixed in #73905.

@ayakael ayakael force-pushed the backport/pr-73065-to-release/6.0 branch from b763366 to b94f3b5 Compare November 2, 2022 15:41
@carlossanlop
Copy link
Member

@jkotas I see you approved the main PR. When/if this is ready, can you please add the servicing-consider label and send an email to Tactics requesting merge approval?

@ghost
Copy link

ghost commented Nov 4, 2022

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #73065 and #73905

Per @am11:

Fix clang 15 RC1 build: error: definition of builtin function '__cpuid'.
Also add clang 15 autodetection; upstream PR https://github.com/dotnet/arcade/pull/10199.
* gcc < v10 does not define the __has_builtin macro which I missed in unixstubs.cpp when making the previous change: https://github.com/dotnet/runtime/commit/992cf8c97cc71d4ca9a0a11e6604a6716ed4cefc. Some community legs like illumos use gcc v8.4 which were failing.
* gcc v10+ supports __has_builtin macro which was being skipped due to version based conditions and we were forcing the fallback code path.
This PR moves __has_builtin macro check in two root headers which cover all the sources that are using it (pal.h and gcenv.base.h)

Customer impact

Without this, build with clang15 fails.

Testing

  • unit tests in ci
  • source-build build within Alpine environment passes

Risk

Low, as it is already in main, and only activates when these variables aren't defined

Author: ayakael
Assignees: -
Labels:

area-Infrastructure-libraries, area-Infrastructure-coreclr, community-contribution

Milestone: 6.0.x

@jkotas jkotas added the Servicing-consider Issue for next servicing release review label Nov 4, 2022
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. we will take for consideration in 6.0.x

@jeffschwMSFT jeffschwMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Nov 7, 2022
@carlossanlop
Copy link
Member

The CI failure is weird, because the test is supposed to be disabled (it was disabled here):

[ActiveIssue("https://github.com/dotnet/runtime/issues/64153")]

Aside from that, the CI is green, it was approved by Tactics, signed off by area owner, and does not need OOB package authoring changes (it's native code). Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit df01243 into dotnet:release/6.0 Nov 7, 2022
@carlossanlop
Copy link
Member

The CI failure is weird, because the test is supposed to be disabled (it was disabled here):

Ah, this is 6.0, I thought this was 7.0, that's why we're hitting it here too. I'll backport the ActiveIssue change.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-coreclr community-contribution Indicates that the PR has been added by a community member Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants