Walkthrough

The changes involve the zbuild.h header file, where a fallback definition for the ALIGNED_ macro has been added. This modification ensures that if no specific compiler support is detected, a placeholder definition is provided. The rest of the file remains unchanged, maintaining existing functionality.

Changes

Suggested reviewers

• Dead2

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

@winspool Just wondering, what compiler is hitting this?

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 32.12%. Comparing base (5456966) to head (7b625d2).
Report is 7 commits behind head on develop.

@@             Coverage Diff             @@
##           develop    #1820      +/-   ##
===========================================
- Coverage    32.19%   32.12%   -0.07%     
===========================================
  Files           67       67              
  Lines         5752     5752              
  Branches      1237     1237              
===========================================
- Hits          1852     1848       -4     
  Misses        3644     3644              
- Partials       256      260       +4     

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

@winspool Just wondering, what compiler is hitting this?

OpenWatcom v2 on linux

TODO:

• a patch related to cpuid()
  (Fallback: returning 0 in the variables with the registernames)

• a patch related to xgetbv
  (Fallback: returning 0 should be ok)

• a patch relatet to zng_alloc
  OW does not support posix_memalign() and alligned_alloc() yet
  Using the normal malloc() should work,
  as the intinsics are not supported
  and the higher alignment is probably not required.

Yes, all together the result is a lot slower as possible,
but at a first step, it has to work.

@winspool Just wondering, what compiler is hitting this?

My _build script is here:

https://github.com/winspool/_build.git

I build with autotools, which results in configure --static --32
and for OpenWatcom, i have to overwrite AR and ARFLAGS:

HOST=" "  AR="wlib"  ARFLAGS="-n"  _build  --static  --32

Btw, cross-compiling on linux for _WIN32 fails,
but cross-compiling ( including make test ) works here
for DOS with the causeway extender.
(works also with the dos4gw extender)

Proposed additional portability-patches to allow OpenWatcom and tcc to compile zlib-ng:

Use a fallback for cpuid related functions:
0003-portability-Add-a-fallback-for-cpuid-related-functio.txt

Use a fallback for memalign:
0002-portability-Check-for-memalign-when-available.-Other.txt

When patch 0002 is an acceptable style, i can update configure and CMakeList.txt
in a similar style for the cpuid related functions/includes.

This will also fix cross compiling for windows (tested with i386-win32-tcc and x86_64-win32-tcc)

@winspool Just wondering, what compiler is hitting this?

Forgot to mention, that tcc is also affected

Generally I'm not against adding support for new compilers, but it seems these compilers target "configurations" that are not highest priority for zlib-ng as most of the optimisations get disabled.

Generally I'm not against adding support for new compilers,

Great.
The improved speed look interesting,
but requiring to use one of the 3 mainstream compiler
for the speedup is not needed.

I usually use autotools, but the provided configure script
is simple enough to be easily extended.
And for cmake, i test with gcc and clanng.
I have no MSVC installed, and hope, that zlib-ng is able to extended and test the related parts it in a similar way.

but it seems these compilers target "configurations"
that are not highest priority for zlib-ng as most of the optimisations get disabled.

Get it runing without optimization is only the first step.
I hope to get the optimized code running without depending on a specific toolchain, when possible (thanks to the intrinsics).

The C++ tests won't work, when using tcc (Tiny C Compiler: only C)
or when using OpenWatcom (Missing Many C++11 features),
but the simple C test work on all compiler, which i have tested.

Please give me some infos, what i can do,
to get my suggested portability changes in the main tree?

Is an update to this simple PR needed?
Are the other attached proposed changes OK?
How many portability changes in one PR is acceptable?
What else?

I didn't looked at the CodeCov integration yet,
but i have additional changes to test the different memory allocation strategies locally.

Please give me some infos, what i can do, to get my suggested portability changes in the main tree?

Is an update to this simple PR needed? Are the other attached proposed changes OK? How many portability changes in one PR is acceptable? What else?

Adding support for one compiler per pull request is usually good idea as it makes reviewing the necessary changes easier... If two or more compilers benefit for overlapping changes, then adding the overlapping part first as own commit in PR makes it clear what changes overlap and what are specific to each compiler. If just the overlapping part doesn't result in successful build, then combine it with enough code for one compiler configuration that results in successful build (smaller initial commit is better).

We allow incremental patches that span multiple pull requests, but generally single PR shouldn't break the build for any configuration, or revert changes in previous PR in same patch series. Reverting older changes is acceptable as long as the older change was clearly bad or it is no longer needed due to other changes committed in between.

As far as I know latest stable version of Watcom compiler is quite old (it was released the same day my older girl was born), but I haven't yet checked what newer features are planned for the current beta version or when the feature freeze is for that branch.

I know almost nothing about Tiny C Compiler, so can't comment on that yet...

Patch 3 doesn't look quite right, but the first two have nothing seriously bad that I can notice... Clang as far as I know defines __GNUC__ as it reuses parts of GNU toolchain for binary compatibility. This is why to test for GCC, it is necessary to test that __GNUC__ is defined, but the preprocessor defines for other forked compilers are not defined, including for example Clang and Intel compilers.

In general, we don't support this compiler, but I am not against merging patches like this one that really has no downside.
In this case, I think we can actually call this a bug, the macro should have had a fallback.

In general, we don't support this compiler, but I am not against merging patches like this one that really has no downside.
In this case, I think we can actually call this a bug, the macro should have had a fallback.

Thanks.
What's next for this pull requests?

Curiously this macro was causing problems for me with CMake, GCC, and PowerPC. I never got to the bottom of it, though. We actually do need alignment for VMX, otherwise you get the fun side effect of a load happening at the next alignment boundary, silently.

Curiously this macro was causing problems for me with CMake, GCC, and PowerPC. I never got to the bottom of it, though. We actually do need alignment for VMX, otherwise you get the fun side effect of a load happening at the next alignment boundary, silently.

@KungFuJesus I am a little uncertain about this comment, so I hope you can clarify; Is this a Pro/Con on this PR, or a suggestion for later, or just a comment?

Curiously this macro was causing problems for me with CMake, GCC, and PowerPC. I never got to the bottom of it, though. We actually do need alignment for VMX, otherwise you get the fun side effect of a load happening at the next alignment boundary, silently.

@KungFuJesus I am a little uncertain about this comment, so I hope you can clarify; Is this a Pro/Con on this PR, or a suggestion for later, or just a comment?

Mostly just a comment. The issue likes somewhere in CMake, I think. There's a pending PR for PPC that suddenly breaks the machinery around how we specify this macro: #1690
It's not entirely tangential, it might make things silently fail if that other PR is merged. I really need to get to the bottom of that.

Mostly just a comment. The issue likes somewhere in CMake, I think. There's a pending PR for PPC that suddenly breaks the machinery around how we specify this macro: #1690 It's not entirely tangential, it might make things silently fail if that other PR is merged. I really need to get to the bottom of that.

Ah #1690, now I get it 👍