pnggroup / libpng

LIBPNG: Portable Network Graphics support, official libpng repository
http://libpng.sf.net
Other
1.28k stars 625 forks source link

Fix CPU architecture match expressions #499

Closed saucecontrol closed 11 months ago

saucecontrol commented 11 months ago

The current expressions for CPU architecture matching use ? and * as if they are wildcards, resulting in incorrect interpretation by CMake's regex matching.

At best, these are benign. For example, ^powerpc* will match powerp and powerpcccccc, which is likely not intended, but it will also match the intended powerpc.

More problematic is ^i?86, which will match i86 and 86, but not the intended i386 or i686.

Additionally, the expressions used do not match the architectures set by some toolchains, including x86, AMD64, and ARM64, which are used by MSVC.

This PR updates the expressions to match their intent and adds handling for the missed architectures, making the matches case-insensitive where appropriate.

With the Arm64 code now enabled for MSVC, the GAS assembly file must be excluded for MSVC. pngpriv.h disables the Arm32 assembly code for all but GCC anyway.

Closes https://github.com/glennrp/libpng/issues/213

ctruta commented 11 months ago

Thank you, Clinton, for your contribution.

You are correct about the incorrect use of ? and * in CMake. I suppose those regexes have been copied verbatim from the configure script, long ago, without anybody noticing the difference in semantics between the shell scripting language and the cmake language.

I applied a bunch of changes of my own on top of your first patch, in order to make those regular expressions more brief and more readable. Moreover, another thing that I noticed that ^mipsel was too strict, excluding plain ^mips, as well as ^mipsisa, so I fixed that one as well.

saucecontrol commented 11 months ago

Thanks, Cosmin. I considered combining the expressions as well but was trying to keep changes minimal.

Your update has notably excluded x86, which is used by MSVC. The x86 expression I included was meant to catch both x86 and x86_64.

I'm not at all familiar with the MIPS code, but I expect that it assumes little-endian byte order and was restricted to mipsel for that reason.

ctruta commented 11 months ago

Whoops!...

You are right, Clinton, not only about x64 vs x86_64, but also about mips vs mipsel. Now, on the topic of MIPS instructions, the code inside filter_msa_intrinsics.c clearly accounts for the classic MIPS(EL?) and for the modern MIPSISAr6(EL?), but neither the configure script nor the CMake file seem to know about any of that. A MIPS expert must have submitted the code to libpng, leaving it up to the rest of us to integrate it. The only way to go forward is by reverting this change, and reapply it later, both in configure.ac and CMakeLists.txt, after I fire up QEMU with all MIPS, MIPSEL, MIPSISAr6, MIPSISAr6EL, 32-bit and 64-bit, and test the heck out of it. Ah, the joys of platform-specific code in a platform-independent project!!

I will make another update, shortly.

ctruta commented 11 months ago

OK, so, I hope I got it right, this time around. Extra-thanks for checking on what I'm doing with your code.

I considered combining the expressions as well but was trying to keep changes minimal.

It was a good thing to consider... and now, look at all those commits! :stuck_out_tongue:

In spite of what happened here, I, also, like the deltas to be as small as possible. However, when it comes to a dispute between the smallest code vs. the smallest commit, for me at least, the smallest code almost always wins.

saucecontrol commented 11 months ago

Looks good, thanks! Your continued maintenance of this project is much appreciated :)