Skip to content

Reenable C4242 and C4244 warnings in libunwind#100241

Merged
AaronRobinsonMSFT merged 6 commits intodotnet:mainfrom
AaronRobinsonMSFT:runtime_99471
Mar 29, 2024
Merged

Reenable C4242 and C4244 warnings in libunwind#100241
AaronRobinsonMSFT merged 6 commits intodotnet:mainfrom
AaronRobinsonMSFT:runtime_99471

Conversation

@AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Mar 25, 2024 •
edited
Loading

Fixes #99471
Fixes #100207

Big thanks to @filipnavara for doing this.

Will create upstream (libunwind) patch after confirmation we are clean.

@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 9.0.0 milestone Mar 25, 2024
@dotnet-policy-service
Copy link
Contributor

dotnet-policy-service bot commented Mar 25, 2024

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

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented Mar 25, 2024

/cc @janvorli @filipnavara @am11

@am11
Copy link
Member

am11 commented Mar 25, 2024

Looks great! :)

Few errors showing up in the CI:

  D:\a\_work\1\s\src\native\external\libunwind\src\aarch64\Gstash_frame.c(57): error C4242: 'function': conversion from 'unw_word_t' to 'long', possible loss of data
  D:\a\_work\1\s\src\native\external\libunwind\src\aarch64\Gstash_frame.c(63): error C4242: 'function': conversion from 'unw_word_t' to 'long', possible loss of data
  D:\a\_work\1\s\src\native\external\libunwind\src\aarch64\Gstash_frame.c(69): error C4242: 'function': conversion from 'unw_word_t' to 'long', possible loss of data
  D:\a\_work\1\s\src\native\external\libunwind\src\aarch64\Gstash_frame.c(75): error C4242: 'function': conversion from 'unw_word_t' to 'long', possible loss of data
  D:\a\_work\1\s\src\native\external\libunwind\src\dwarf\Gexpr.c(369): error C4242: 'function': conversion from 'unw_word_t' to 'int', possible loss of data
  D:\a\_work\1\s\src\native\external\libunwind\src\dwarf\Gparser.c(885): error C4242: '=': conversion from 'unw_word_t' to 'unw_regnum_t', possible loss of data

Will create upstream (libunwind) patch after confirmation we are clean.

When upstream PR is created, please include its reference in src/native/external/libunwind-version.txt in this PR's branch.

@AustinWise
Copy link
Contributor

AustinWise commented Mar 25, 2024

This should also fix #100207.

janvorli
janvorli approved these changes Mar 25, 2024
View reviewed changes
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!

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented Mar 25, 2024 •
edited
Loading

@am11 if you could also take another pass at this, I would be most appreciative.

@AaronRobinsonMSFT AaronRobinsonMSFT marked this pull request as ready for review March 25, 2024 20:21
@AaronRobinsonMSFT AaronRobinsonMSFT requested a review from a team as a code owner March 25, 2024 20:21
filipnavara
filipnavara approved these changes Mar 25, 2024
View reviewed changes
Copy link
Member

@filipnavara filipnavara left a comment

Choose a reason for hiding this comment

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

Thanks for wrapping this up! LGTM

@am11
Copy link
Member

am11 commented Mar 25, 2024

@AaronRobinsonMSFT, looks good but can you please open a PR upstream and reference it in version file? We have been following that pattern for all similar PRs.

@build-analysis build-analysis bot mentioned this pull request Mar 25, 2024
Closed
@AaronRobinsonMSFT AaronRobinsonMSFT mentioned this pull request Mar 25, 2024
Merged
@build-analysis build-analysis bot mentioned this pull request Mar 25, 2024
Closed
This was referenced Mar 29, 2024
Closed
Open
Closed
@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 869dfb7 into dotnet:main Mar 29, 2024
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the runtime_99471 branch March 29, 2024 20:22
@github-actions github-actions bot locked and limited conversation to collaborators Apr 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@janvorli janvorli janvorli approved these changes

+1 more reviewer

@filipnavara filipnavara filipnavara approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@AaronRobinsonMSFT AaronRobinsonMSFT

Labels

area-Diagnostics-coreclr Security

Projects

None yet

Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

C4242 error when compiling CoreCLR on Windows with -msbuild SDL signoff requirements - enable MSVC warnings C4242 and C4244 (libunwind)

5 participants

@AaronRobinsonMSFT @am11 @AustinWise @filipnavara @janvorli