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

Use size_t for hostfxr info count members #48276

Merged
merged 1 commit into from
Feb 15, 2021
Merged

Use size_t for hostfxr info count members #48276

merged 1 commit into from
Feb 15, 2021

Conversation

am11
Copy link
Member

@am11 am11 commented Feb 14, 2021

Installer build with GCC failed:

  /runtime/src/native/corehost/fxr/hostfxr.cpp:456:35: error: narrowing conversion of 'environment_sdk_infos.std::vector<hostfxr_dotnet_environment_sdk_info>::size()' from 'std::vector<hostfxr_dotnet_environment_sdk_info>::size_type' {aka 'long unsigned int'} to 'int32_t' {aka 'int'} inside { } [-Werror=narrowing]
           environment_sdk_infos.size(),
           ~~~~~~~~~~~~~~~~~~~~~~~~~~^~
  /runtime/src/native/corehost/fxr/hostfxr.cpp:458:41: error: narrowing conversion of 'environment_framework_infos.std::vector<hostfxr_dotnet_environment_framework_info>::size()' from 'std::vector<hostfxr_dotnet_environment_framework_info>::size_type' {aka 'long unsigned int'} to 'int32_t' {aka 'int'} inside { } [-Werror=narrowing]
           environment_framework_infos.size(),
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~

This PR fixes the build.

@ghost
Copy link

ghost commented Feb 14, 2021

Tagging subscribers to this area: @vitek-karas, @agocke
See info in area-owners.md if you want to be subscribed.

Issue Details

Installer build with GCC failed:

  /runtime/src/native/corehost/fxr/hostfxr.cpp:456:35: error: narrowing conversion of 'environment_sdk_infos.std::vector<hostfxr_dotnet_environment_sdk_info>::size()' from 'std::vector<hostfxr_dotnet_environment_sdk_info>::size_type' {aka 'long unsigned int'} to 'int32_t' {aka 'int'} inside { } [-Werror=narrowing]
           environment_sdk_infos.size(),
           ~~~~~~~~~~~~~~~~~~~~~~~~~~^~
  /runtime/src/native/corehost/fxr/hostfxr.cpp:458:41: error: narrowing conversion of 'environment_framework_infos.std::vector<hostfxr_dotnet_environment_framework_info>::size()' from 'std::vector<hostfxr_dotnet_environment_framework_info>::size_type' {aka 'long unsigned int'} to 'int32_t' {aka 'int'} inside { } [-Werror=narrowing]
           environment_framework_infos.size(),
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~

This PR fixes the build.

Author: am11
Assignees: -
Labels:

area-Host

Milestone: -

@am11
Copy link
Member Author

am11 commented Feb 14, 2021

cc @VSadov

Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

How come this made it through CI - is the warning disabled, or something like that?

@am11
Copy link
Member Author

am11 commented Feb 15, 2021

The gcc leg only builds CoreCLR subset using gcc v9. Build breakage was caught yesterday by downstream CI set up in the fork. With the fix, it passed.

I think Arch Linux and few distros are also using gcc to build the .NET runtime package. Currently all native components (including mono) build just fine with gcc, so IMO expanding the gcc CI coverage to build all components would be great.

@VSadov
Copy link
Member

VSadov commented Feb 15, 2021

I think some warnings are disabled, or maybe we never build host.native with GCC.
I see these warnings as errors in #48254 though.

@VSadov
Copy link
Member

VSadov commented Feb 15, 2021

There are lots of other warnings as well, but those do not seem to be warn-as-error kind.

https://dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_apis/build/builds/994542/logs/2088

@am11
Copy link
Member Author

am11 commented Feb 15, 2021

The warnings configs are mostly shared between coreclr and installer, our gcc build validation leg is set up to only build coreclr subset.

@am11
Copy link
Member Author

am11 commented Feb 15, 2021

other warnings

the most significant one is:

warning: 'emitter::emitAddrMode::amScale' is too small to hold all values of 'enum emitter::opSize'

having 1000+ occurrences. It has no warning code as it is emerging from core compiler which just writes to stderr without failing.

The appropriate warning code is -Woverflow with gcc and -Wbitfield-enum-conversion with clang, when we assign a value to the instance of enum that exceeds its own bitfield width. In this case, we have a value in enum which exceeds its own bitfild's width, but we are not assigning it to any instance (yet). So compiler thinks it's a matter of time and warns.. tracked by #33541 (comment).

@VSadov VSadov merged commit a62ca2d into dotnet:master Feb 15, 2021
@VSadov
Copy link
Member

VSadov commented Feb 15, 2021

Thanks!!!

@am11 am11 deleted the feature/gcc/installer-build branch February 15, 2021 03:16
@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 2021
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.

3 participants