Re: Broken ./configure checks for __cpuid() and __cpuidex() - Mailing list pgsql-hackers
| From | Lukas Fittl |
|---|---|
| Subject | Re: Broken ./configure checks for __cpuid() and __cpuidex() |
| Date | |
| Msg-id | CAP53PkxoZNHG-uEOrZ2NharmfDgBBdGL41eK=mWifyFsiM6rcA@mail.gmail.com Whole thread Raw |
| In response to | Re: Broken ./configure checks for __cpuid() and __cpuidex() (Michael Paquier <michael@paquier.xyz>) |
| Responses |
Re: Broken ./configure checks for __cpuid() and __cpuidex()
|
| List | pgsql-hackers |
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.
Thanks,
Lukas
Lukas Fittl
pgsql-hackers by date: