darvid / python-hyperscan

🐍 A CPython extension for the Hyperscan regular expression matching library.
https://python-hyperscan.readthedocs.io/en/latest/
MIT License
165 stars 30 forks source link

Problem with musl and fat runtime? #95

Open Jc2k opened 10 months ago

Jc2k commented 10 months ago

Sorry I haven't had time to dig into this as much as i'd like but i've been getting:

Illegal instruction (core dumped)

When preparing a hyperscan database, on alpine 3.18 / python 3.11 and with latest python-hyperscan tag.

It works fine on an iMac running Linux (Fedora). And also on a newer iMac running macOS. The box where its crashing (CI) does have an older processor and its in an Alpine container. But the processor does still meet the minimum requirements.

I've built a local wheel that uses -march=core2 and disabled FAT_RUNTIME and that works. -march=corei7 also works.

This post (from earlier this year) suggests musl doesn't support the ifunc feature required for fat runtime. Looking through build logs I see

-- Performing Test HAS_C_ATTR_IFUNC
-- Performing Test HAS_C_ATTR_IFUNC - Failed
-- Compiler does not support ifunc attribute, cannot build fat runtime

So the FAT_RUNTIME define is getting ignored in musl builds. I think this means that the wheels only support the arch that github runners happen to use, which is probably AVX2?

Any chance we can lower the target -march in the official wheels?

darvid commented 7 months ago

i can disable fat runtime and set march to core2 for just the musl wheels, but that would mean applications running on alpine on hosts that support AVX2 would still use SSE3, right?

is this still a pain point for you? i'm trying to think of the best universal way to support this, and other instruction sets that are newer (like AVX512, which requires manual config at buildtime and isn't turned on with fat runtime) that might require changing the default build configuration.

my first thought is before changing the build configuration across the board for all musl-based wheels, we can avoid the crash on processors with the minimum required march by testing for libc/ifunc support and turning off fat runtime, and setting march. that way the source dist still builds for the minimum requirements. would that be a good compromise?

Jc2k commented 7 months ago

I have a workaround locally. It's not pretty, but it works.

But, I still wonder if this wheel is technically not valid - aren't cpu features part of the ABI? So if you are following the PEPs you'd have to build for the same minimum cpu that eg alpine uses?

In which case the "right" thing to do by the spec is to make the wheel work everywhere, and make the sdist for people with beefier processors.

darvid commented 7 months ago

ok this will require a change in the underlying docker image i maintain with hyperscan and deps needed for static linking, but correct me if i'm wrong, based on your logs it looks like HS properly turned fat runtime off, so i'm guessing -march=native breaks on musl?

darvid commented 7 months ago

either way i think you're right, i'll set march but only on musl

Jc2k commented 7 months ago

Yes, afaict you are right. fat runtime is turned off so -march=native is a no go, because the GitHub runners have AVX.