Thread

Re: Broken ./configure checks for __cpuid() and __cpuidex()

Lukas Fittl <lukas@fittl.com> — 2025-07-29T07:21:32Z

On Mon, Jul 28, 2025 at 10:30 PM Michael Paquier <michael@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