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

gh-126433: Fix compiler warning on 32-bit Windows #126448

Closed
wants to merge 1 commit into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Nov 5, 2024

@vstinner
Copy link
Member Author

vstinner commented Nov 5, 2024

Fix the following warning:

C:\victor\python\main\Modules_hacl\Lib_Memzero0.c(39,27): warning C4244: 'function': conversion from 'uint64_t' to 'SIZE_T', possible loss of data [C:\victor\python\main\PCbuild\pythoncore.vcxproj]

@msprotz: Should I fix the issue in HACL upstream first? Or do you want to fix it upstream?

@vstinner
Copy link
Member Author

vstinner commented Nov 5, 2024

SecureZeroMemory() second parameter type is SIZE_T: https://learn.microsoft.com/en-us/previous-versions/windows/desktop/legacy/aa366877(v=vs.85)

@msprotz
Copy link
Contributor

msprotz commented Nov 5, 2024

If you could fix upstream that would be optimal. The file is in hacl-star/lib/c/ I believe. Thanks!!

@vstinner
Copy link
Member Author

vstinner commented Nov 6, 2024

If you could fix upstream that would be optimal. The file is in hacl-star/lib/c/ I believe. Thanks!!

@msprotz: I created hacl-star/hacl-star#1004.

@vstinner
Copy link
Member Author

vstinner commented Nov 7, 2024

@msprotz: I created hacl-star/hacl-star#1004.

It has been merged. Can I run make regen-sbom or should I updated hacl-star in Python somehow?

@msprotz
Copy link
Contributor

msprotz commented Nov 7, 2024

You need to:

  • go into Modules/_hacl, edit refresh.sh to point to the new hash, and run refresh.sh
  • git grep for the old hash in the root cpython directory, replaces all occurrences of the old hash with the new hash (they appear in various json files)
  • make regen-sbom

And then you should be good! Cheers.

@vstinner
Copy link
Member Author

vstinner commented Nov 8, 2024

And then you should be good! Cheers.

I updated lib/c/Lib_Memzero0.c in hacl-start upstream, but I didn't see the different copies:

$ find -name "Lib_Mem*c"
./dist/gcc-compatible/Lib_Memzero0.c
./dist/mozilla/Lib_Memzero0.c
./dist/msvc-compatible/Lib_Memzero0.c
./dist/portable-gcc-compatible/Lib_Memzero0.c
./lib/c/Lib_Memzero0.c

On my upstream PR, the bot said "dist is not automatically re-generated, be sure to do it manually." Oops, I didn't do that!

Should I modify manually the 5 files?

@msprotz
Copy link
Contributor

msprotz commented Nov 8, 2024

The propagation happens via a CI job run weekly on weekends I believe, but I triggered it manually for you to speed things up. Should be good soon. Thanks!

@vstinner
Copy link
Member Author

vstinner commented Nov 8, 2024

Oh ok, I will wait next week for the CI to update these files.

@msprotz
Copy link
Contributor

msprotz commented Nov 8, 2024

vstinner added a commit to vstinner/cpython that referenced this pull request Nov 13, 2024
Run Modules/_hacl/refresh.sh and update SBOM.

Retrieve the change: "Lib_Memzero0.c: Fix compiler warning on 32-bit
Windows".
@vstinner
Copy link
Member Author

I created PR gh-126791 to update hacl-star and retrieve the change fixing the warning. I close this PR.

@vstinner vstinner closed this Nov 13, 2024
@vstinner vstinner deleted the fix_win_warns3 branch November 13, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants