On Mon, Jul 28, 2025 at 10:30 PM Michael Paquier <mich...@paquier.xyz> wrote:
> meson.build does not check for the second pieces if the > first checks pass. configure checks each of these four APIs > individually, and all of them detected in MinGW. So we can simply > mimick in ./configure what meson does like in the attached, which has > been working for some time now. > > The CI is happy with this version for me, with MSVC reporting the second > steps, and MinGW reporting the first steps. That would be for the > buildfarm to decide if it is entirely stable, but from my perspective > I am pretty confident that this patch should be OK as-is. And that's > pretty much what the CRC32 code expects from these checks: we only > want one of these routines. > Nice, happy to hear that makes it work, and if I got my reading of configure.ac correct, the code looks good to me. And agreed on the approach for the back-branches, the code would only use one of the two, and it makes sense to match how Meson already behaved. Its worth noting that __get_cpuid_count and __cpuidex are not fully equivalent (which is part of why GCC added __cpuidex despite already having __get_cpuid_count), but in any case the current code doesn't care about that, since it prefers __get_cpuid_count if available, and I think that difference only applies to special leafs like the VM Hypervisor information (not used yet). > > I'm not sure how to get CI to run MinGW (it appears paused for me?), so I > > can't test this myself easily. > > src/tools/ci/README, "Enabling cirrus-ci in a github repository". > I've enabled it in my own copy of Postgres on github, relying on that > as an extra pre-commit check mostly for patches that are OS-sensitive. > It runs independently on the CI, relying on the OS base images that > Andres has been cooking for the last few years, of course. > Thanks, to be clear, I have CI enabled but the MinGW tasks were always paused (presumably because of the trigger type being manual). But I think "ci-os-only" as noted in the README should do the trick, I'll go investigate that. > > But the relevant change would be to change "defined(HAVE__CPUIDEX)" to > > "(defined(HAVE__CPUIDEX) && defined(_MSC_VER))" for the guard on both > > intrin.h includes. > > _MSC_VER is a flag specific to MSVC, so we'd still get the problem > with MinGW, no? So we'd still have the same problem. Perhaps we'll > need the _MSC_VER piece for your other patch, but for the sake of the > back-branches and what we are doing here it does not seem necessary to > me to do so. > Hmm, yeah, let me do some more testing of MinGW in the context of the other patch. FWIW, I looked again at the MinGW sources and I think you're right that intrin.h is likely also correct for MinGW. I originally thought that cpuid.h would be correct, given that's whats used by GCC/clang, but I think the MinGW source itself is authoritative here (vs the compiler in use), and that has a intrin.h include ([0]) but no cpuid.h. [0]: https://github.com/mingw-w64/mingw-w64/blob/master/mingw-w64-headers/crt/intrin.h Thanks, Lukas -- Lukas Fittl
