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

Reenable C4242 and C4244 warnings in libunwind #100241

Merged
merged 6 commits into from
Mar 29, 2024

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Mar 25, 2024

Fixes #99471
Fixes #100207

Big thanks to @filipnavara for doing this.

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

Copy link
Contributor

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

@AaronRobinsonMSFT
Copy link
Member Author

/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

This should also fix #100207.

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

@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
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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants