search

haskell / alex

A lexical analyser generator for Haskell
https://hackage.haskell.org/package/alex
BSD 3-Clause "New" or "Revised" License
298 stars 82 forks source link

Use byteSwapX# in WORDS_BIGENDIAN #260

Closed sgraf812 closed 4 months ago

sgraf812 commented 4 months ago

Spurred by @Bodigrim in https://github.com/haskell/happy/pull/280#discussion_r1651705321, let's try having a bit more efficient code on WORD_BIGENDIAN architectures.

Mostly uploading this for CI, hoping there will be a big-endian machine to test the new code path.

sgraf812 commented 4 months ago

This seems to work as expected, presuming the MacOS build really tests the WORDS_BIGENDIAN code path. And I couldn't tell how it would work otherwise.

andreasabel commented 4 months ago

Thanks!

Could we get a changelog entry for this?

sgraf812 commented 4 months ago

Done. I say 3.5.2.0 in the CHANGELOG, but I'm inclined to think it's only a PATCH change really. You can always adjust later, I guess.

Bodigrim commented 4 months ago

This seems to work as expected, presuming the MacOS build really tests the WORDS_BIGENDIAN code path.

Unfortunately it does not, Apple hardware is little endian for the past 15 years or so.

To test the change properly one has to setup an emulated job for s390x architecture (aka IBM mainframe). There are several examples of such CI setup around github.com/haskell, the most recent one I worked on is https://github.com/haskell/tar/blob/master/.github/workflows/emulated.yml.

sgraf812 commented 4 months ago

Oh, wow... I somehow thought that iOS was big endian, but it actually isn't. Nevermind then.

sgraf812 commented 4 months ago

Alright, I added an emulated CI pipeline as @Bodigrim suggested. Not pretty, but does the job, and shouldn't break all to often given that alex is pretty stable.

For the job in https://github.com/sgraf812/alex/actions/runs/9912679649/job/27388017292 (569fb3f), I replaced byteSwap with garbage to test that the WORDS_BIGENDIAN code path is covered, so that job is expected to fail.

The job in https://github.com/sgraf812/alex/actions/runs/9912681641/job/27388023583 (36cc05e) does no such replacement and consequently is green.

No idea why the other CI jobs fail cabal check. Edit: It sure is suspicious that all (and only) jobs from the haskell-ci-simple.yml workflow fail. Edit2: I see similar errors in the regenerated CI workflows of happy. Sigh. It really seems that all deps need to announce upper bounds now.

andreasabel commented 4 months ago

It really seems that all deps need to announce upper bounds now.

No, I think only base absolutely needs one now:

Error: [missing-bounds-important] The dependency 'build-depends: base' does not specify an upper bound on the version number.

andreasabel commented 4 months ago

I am fixing Haskell CI in this PR that we should merge first:

andreasabel commented 4 months ago

@Mergifyio rebase

andreasabel commented 4 months ago

@sgraf812 : Do you want this change released now?

sgraf812 commented 4 months ago

Thanks! No need for a release, I just wanted to test the code in https://github.com/haskell/happy/pull/280. Replicating the emulated-CI-without-cabal setup for the multi-component happy project is far too ambitious for my tastes.

Edit: Do press merge when you think this is ready!