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

Fix deflate_state alignment with MS or clang-cl compilers #1663

Merged
merged 2 commits into from
Feb 15, 2024

Conversation

pps83
Copy link
Contributor

@pps83 pps83 commented Feb 11, 2024

When building with clang-cl, compiler produces the following warning:

zlib-ng/deflate.h(287,3): warning : attribute 'align' is ignored, place it after "struct" to apply attribute to type declaration [-Wignored-attributes]
zlib-ng/zbuild.h(196,34): note: expanded from macro 'ALIGNED_'

Repositioning align attribute after "struct" fixes the warning and aligns deflate_state correctly.

When building with clang-cl, compiler produces the following warning:

zlib-ng/deflate.h(287,3): warning : attribute 'align' is ignored, place it after "struct" to apply attribute to type declaration [-Wignored-attributes]
zlib-ng/zbuild.h(196,34): note: expanded from macro 'ALIGNED_'

Repositioning align attribute after "struct" fixes the warning and aligns `deflate_state` correctly.
@pps83
Copy link
Contributor Author

pps83 commented Feb 11, 2024

Here's sample program showing that repositioning align attribute after "struct" doesn't affect alignment with g++:

https://coliru.stacked-crooked.com/a/cc7bafe2614a1b52

If you compile the same code with cl or clang-cl you'll see that align attribute placed at the end of the struct declaration has no effect. In my case, for example, I'm getting this output:

deflate_state1 align: 8
deflate_state2 align: 256

@nmoinvaz
Copy link
Member

Did you check to see if any other struts suffer this same problem?

@pps83
Copy link
Contributor Author

pps83 commented Feb 11, 2024

Did you check to see if any other struts suffer this same problem?

That's the only place, clang-cl has no other warnings also.

Fixing align attribute, makes ms compiler warn: 'internal_state': Alignment specifier is less than actual alignment (16), and will be ignored.
Increasing alignemnt fixes the warning
Copy link

codecov bot commented Feb 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2049832) 83.24% compared to head (f387ac3) 83.24%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1663   +/-   ##
========================================
  Coverage    83.24%   83.24%           
========================================
  Files          135      135           
  Lines        10927    10927           
  Branches      2815     2815           
========================================
  Hits          9096     9096           
  Misses        1129     1129           
  Partials       702      702           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pps83
Copy link
Contributor Author

pps83 commented Feb 11, 2024

Note, I added one more commit to increase alignment to 16 bytes. Fixing attribute made ms compiler understand the request and it started to complain:

3>D:\work-pps\backtest-engine\ext\zlib-ng\deflate.h(115,20): warning C4359: 'internal_state': Alignment specifier is less than actual alignment (16), and will be ignored.
3>(compiling source file '../ext/zlib-ng/zlib.cpp')

@pps83
Copy link
Contributor Author

pps83 commented Feb 14, 2024

@Dead2 ping

@Dead2 Dead2 merged commit 7745c28 into zlib-ng:develop Feb 15, 2024
139 checks passed
@pps83 pps83 deleted the fix-align branch February 15, 2024 16:16
@Dead2 Dead2 mentioned this pull request May 30, 2024
@Dead2 Dead2 mentioned this pull request Jun 12, 2024
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