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

COMP: Fix data loss warning #1048

Merged
merged 1 commit into from
Oct 13, 2021
Merged

COMP: Fix data loss warning #1048

merged 1 commit into from
Oct 13, 2021

Conversation

jhlegarreta
Copy link
Contributor

Fix data loss warning.

Fixes:

itkzlib-ng/inflate.c(1209,24): warning C4267: '=': conversion from 'size_t' to 'unsigned long', possible loss of data
itkzlib-ng/inflate.c(1210,26): warning C4267: '=': conversion from 'size_t' to 'unsigned long', possible loss of data

@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Oct 10, 2021

Local system: Windows 10
Compiler: MSVC 2019 version 19.29.30133.0

Found when building ITK: https://github.com/InsightSoftwareConsortium/ITK

@mtl1979
Copy link
Collaborator

mtl1979 commented Oct 10, 2021

Using unsigned long instead of size_t can cause unwanted truncation as we declare total_in and total_out as size_t in non-compat header. unsigned long is only 32 bits under 64-bit Windows, but size_t can be either 32 or 64 bits depending on compiler version.

We might want to use explicit cast to z_size_t instead on those two lines.

@jhlegarreta
Copy link
Contributor Author

Thanks for the review @mtl1979 .

We might want to use explicit cast to z_size_t instead on those two lines.

Let me know if you want me to amend the commit that way.

@mtl1979
Copy link
Collaborator

mtl1979 commented Oct 10, 2021

@jhlegarreta I think using cast is better than changing the type of the temporary variables as that avoids all issues when building the non-compat version, which should be the default version for most developers compiling from sources... "Compat" version still truncates, but that is something we can't fix to stay compatible with stock zlib.

@jhlegarreta
Copy link
Contributor Author

@jhlegarreta I think using cast is better than changing the type of the temporary variables as that avoids all issues when building the non-compat version, which should be the default version for most developers compiling from sources... "Compat" version still truncates, but that is something we can't fix to stay compatible with stock zlib.

Changed in ad6171d.

@mtl1979
Copy link
Collaborator

mtl1979 commented Oct 10, 2021

Cast to unsigned long is still wrong, it will truncate on non-compat version.

Fix data loss warning.

Fixes:
```
itkzlib-ng/inflate.c(1209,24): warning C4267: '=': conversion from 'size_t' to 'unsigned long', possible loss of data
itkzlib-ng/inflate.c(1210,26): warning C4267: '=': conversion from 'size_t' to 'unsigned long', possible loss of data
```
@jhlegarreta
Copy link
Contributor Author

Cast to unsigned long is still wrong, it will truncate on non-compat version.

My bad; 977a2e5 casts to z_size_t. Sorry.

@codecov
Copy link

codecov bot commented Oct 12, 2021

Codecov Report

Merging #1048 (977a2e5) into develop (b418e9c) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1048   +/-   ##
========================================
  Coverage    79.48%   79.48%           
========================================
  Files           85       85           
  Lines         8658     8658           
  Branches      1398     1399    +1     
========================================
  Hits          6882     6882           
  Misses        1228     1228           
  Partials       548      548           
Flag Coverage Δ
macos_clang 69.90% <100.00%> (ø)
macos_gcc 68.53% <100.00%> (ø)
ubuntu_clang 75.48% <100.00%> (ø)
ubuntu_clang_debug 75.59% <100.00%> (ø)
ubuntu_clang_inflate_allow_invalid_dist 75.21% <100.00%> (ø)
ubuntu_clang_inflate_strict 75.42% <100.00%> (ø)
ubuntu_clang_mmap 75.44% <100.00%> (ø)
ubuntu_clang_msan 75.48% <100.00%> (ø)
ubuntu_clang_pigz 36.17% <0.00%> (ø)
ubuntu_clang_pigz_no_optim 40.95% <0.00%> (ø)
ubuntu_clang_pigz_no_threads 35.68% <0.00%> (ø)
ubuntu_clang_reduced_mem 75.73% <100.00%> (ø)
ubuntu_gcc 74.88% <100.00%> (ø)
ubuntu_gcc_aarch64 76.77% <100.00%> (ø)
ubuntu_gcc_aarch64_compat_no_opt 74.55% <100.00%> (ø)
ubuntu_gcc_aarch64_no_acle 76.23% <100.00%> (ø)
ubuntu_gcc_aarch64_no_neon 76.54% <100.00%> (ø)
ubuntu_gcc_armhf 76.77% <100.00%> (ø)
ubuntu_gcc_armhf_compat_no_opt 74.52% <100.00%> (ø)
ubuntu_gcc_armhf_no_acle 77.05% <100.00%> (ø)
ubuntu_gcc_armhf_no_neon 77.33% <100.00%> (ø)
ubuntu_gcc_armsf 76.78% <100.00%> (ø)
ubuntu_gcc_armsf_compat_no_opt 74.52% <100.00%> (ø)
ubuntu_gcc_compat_no_opt 75.51% <100.00%> (ø)
ubuntu_gcc_compat_sprefix 74.79% <100.00%> (ø)
ubuntu_gcc_mingw_i686 0.00% <0.00%> (ø)
ubuntu_gcc_mingw_x86_64 0.00% <0.00%> (ø)
ubuntu_gcc_no_avx2 76.84% <100.00%> (ø)
ubuntu_gcc_no_ctz 77.74% <100.00%> (ø)
ubuntu_gcc_no_ctzll 77.41% <100.00%> (ø)
ubuntu_gcc_no_pclmulqdq 73.62% <100.00%> (ø)
ubuntu_gcc_no_sse2 76.10% <100.00%> (ø)
ubuntu_gcc_no_sse4 73.97% <100.00%> (ø)
ubuntu_gcc_o3 74.09% <100.00%> (ø)
ubuntu_gcc_osb 76.85% <100.00%> (ø)
ubuntu_gcc_pigz 36.56% <0.00%> (ø)
ubuntu_gcc_pigz_aarch64 38.54% <0.00%> (-0.07%) ⬇️
ubuntu_gcc_ppc 74.57% <100.00%> (ø)
ubuntu_gcc_ppc64 77.61% <100.00%> (ø)
ubuntu_gcc_ppc64le 76.07% <100.00%> (ø)
ubuntu_gcc_ppc_no_power8 77.37% <100.00%> (ø)
ubuntu_gcc_s390x 75.92% <100.00%> (ø)
ubuntu_gcc_sparc64 78.65% <100.00%> (ø)
ubuntu_gcc_sprefix 74.57% <100.00%> (ø)
win64_gcc ∅ <ø> (∅)
win64_gcc_compat_no_opt ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
inflate.c 88.15% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b418e9c...977a2e5. Read the comment docs.

@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Oct 12, 2021

Build failures seem unrelated:

Error: Could not find Xcode version that satisfied version spec: '12.1.1'

https://github.com/zlib-ng/zlib-ng/actions/runs/1326434359

e.g.
https://github.com/zlib-ng/zlib-ng/pull/1048/checks?check_run_id=3870178563#step:5:16

@Dead2
Copy link
Member

Dead2 commented Oct 12, 2021

@jhlegarreta Yes, the 3 remaining Ubuntu 16.04 checks are also going to fail after maaany hours of waiting. That seems to be a bug in Github Actions regarding that specific distro version, not sure how that works but it has been like that for weeks now.

I have not seen the xcode errors before, but they probably just mean we need to update our CI scripts.

So I am ignoring those checks 😉

@jhlegarreta
Copy link
Contributor Author

@jhlegarreta Yes, the 3 remaining Ubuntu 16.04 checks are also going to fail after maaany hours of waiting. That seems to be a bug in Github Actions regarding that specific distro version, not sure how that works but it has been like that for weeks now.

@Dead2 Does this have to do with the above?
https://github.blog/changelog/2021-04-29-github-actions-ubuntu-16-04-lts-virtual-environment-will-be-removed-on-september-20-2021/

Should those builds transition to 18.04 as the minimum version?

@Dead2 Dead2 merged commit 5d37363 into zlib-ng:develop Oct 13, 2021
@Dead2
Copy link
Member

Dead2 commented Oct 13, 2021

@jhlegarreta Yep, that is the reason. IMO, Github Actions should have failed quickly instead of spending lots of hours in queue before failing though.

@jhlegarreta jhlegarreta deleted the FixDataLossWarning branch October 13, 2021 13:47
Dead2 added a commit that referenced this pull request Dec 13, 2021
 - Fix hangs on macOS #1031
 - Fix minideflate write buffers being overwritten #1060
 - Fix build problems when building outside of source dir #1049
 - Fix build problems on arm2-7 #1030
 - Fixed some compile warnings #1020 #1036 #1037 #1048
 - Improved posix memalign support #888
 - Improvements to testing #637 #1026 #1035 #1051 #1056 #1063 #1067
 - Improvements for integration into other projects #1022 #1042
 - Code style fixes #637 #1040 #1050
@Dead2 Dead2 mentioned this pull request Dec 13, 2021
Dead2 added a commit that referenced this pull request Dec 20, 2021
 - Fix hangs on macOS #1031
 - Fix minideflate write buffers being overwritten #1060
 - Fix deflateBound and compressBound returning too small size estimates #1071
 - Fix build problems when building outside of source dir #1049
 - Fix build problems on arm2-7 #1030
 - Fixed some compile warnings #1020 #1036 #1037 #1048
 - Improved posix memalign support #888
 - Improvements to testing #637 #1026 #1032 #1035 #1051 #1056 #1063 #1067
 - Improvements for integration into other projects #1022 #1042
 - Code style fixes #637 #1040 #1050 #1075
Dead2 added a commit that referenced this pull request Dec 20, 2021
 - Fix hangs on macOS #1031
 - Fix minideflate write buffers being overwritten #1060
 - Fix deflateBound and compressBound returning too small size estimates #1071
 - Fix build problems when building outside of source dir #1049
 - Fix build problems on arm2-7 #1030
 - Fixed some compile warnings #1020 #1036 #1037 #1048
 - Improved posix memalign support #888
 - Improvements to testing #637 #1026 #1032 #1035 #1051 #1056 #1063 #1067
 - Improvements for integration into other projects #1022 #1042
 - Code style fixes #637 #1040 #1050 #1075
Dead2 added a commit that referenced this pull request Dec 20, 2021
 - Fix hangs on macOS #1031
 - Fix minideflate write buffers being overwritten #1060
 - Fix deflateBound and compressBound returning too small size estimates #1071
 - Fix build problems when building outside of source dir #1049
 - Fix build problems on arm2-7 #1030
 - Fixed some compile warnings #1020 #1036 #1037 #1048
 - Improved posix memalign support #888
 - Improvements to testing #637 #1026 #1032 #1035 #1051 #1056 #1063 #1067
 - Improvements for integration into other projects #1022 #1042
 - Code style fixes #637 #1040 #1050 #1075
Dead2 added a commit that referenced this pull request Dec 20, 2021
 - Fix hangs on macOS #1031
 - Fix minideflate write buffers being overwritten #1060
 - Fix deflateBound and compressBound returning too small size estimates #1049 #1071
 - Fix incorrect function declaration warning #1080
 - Fix build problems when building outside of source dir #1049
 - Fix build problems on arm2-7 #1030
 - Fixed some compile warnings #1020 #1036 #1037 #1048
 - Improved posix memalign support #888
 - Improvements to testing #637 #1026 #1032 #1035 #1049 #1051 #1056 #1063 #1067
 - Improvements for integration into other projects #1022 #1042
 - Code style fixes #637 #1040 #1050 #1075
Dead2 added a commit that referenced this pull request Dec 20, 2021
 - Fix hangs on macOS #1031
 - Fix minideflate write buffers being overwritten #1060
 - Fix deflateBound and compressBound returning too small size estimates #1049 #1071
 - Fix incorrect function declaration warning #1080
 - Fix build problems when building outside of source dir #1049
 - Fix build problems on arm2-7 #1030
 - Fixed some compile warnings #1020 #1036 #1037 #1048
 - Improved posix memalign support #888
 - Improvements to testing #637 #1026 #1032 #1035 #1049 #1051 #1056 #1063 #1067 #1079
 - Improvements for integration into other projects #1022 #1042
 - Code style fixes #637 #1040 #1050 #1075
Dead2 added a commit that referenced this pull request Dec 24, 2021
 - Fix hangs on macOS #1031
 - Fix minideflate write buffers being overwritten #1060
 - Fix deflateBound and compressBound returning too small size estimates #1049 #1071
 - Fix incorrect function declaration warning #1080
 - Fix build problems when building outside of source dir #1049
 - Fix build problems on arm2-7 #1030
 - Fixed some compile warnings #1020 #1036 #1037 #1048
 - Improved posix memalign support #888
 - Improvements to testing #637 #1026 #1032 #1035 #1049 #1051 #1056 #1063 #1067 #1079
 - Improvements for integration into other projects #1022 #1042
 - Code style fixes #637 #1040 #1050 #1075
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants