Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use ragel to generate machine code #77

Merged
merged 6 commits into from
Aug 25, 2023
Merged

Conversation

notgull
Copy link
Collaborator

@notgull notgull commented Aug 19, 2023

I managed to get the latest master for ragel up and running and got it to generate code... although it looks like tests are failing? Not sure why that's happening.

@RazrFalcon
Copy link
Collaborator

RazrFalcon commented Aug 20, 2023

Nice! Can you run rustfmt on those files (not the whole rustybuzz).

@RazrFalcon
Copy link
Collaborator

As for failing tests, which ragel files did you used? The current master one or 2.7.1 one?

@RazrFalcon
Copy link
Collaborator

I guess you did used the master one, because the 2.7.1 universal is slightly different.

Signed-off-by: John Nunley <[email protected]>
@notgull
Copy link
Collaborator Author

notgull commented Aug 20, 2023

I guess you did used the master one, because the 2.7.1 universal is slightly different.

I've synced with 2.7.1, although tests are still failing, specifically for the Indic tests.

@RazrFalcon
Copy link
Collaborator

Hmm... no idea what could be wrong. Could be a ragel bug.
The suspicious part is that state machine tables are very different compared to what they were. Either newer ragel produces different output or something else.

By the way, what ragel flags have you used? harfbuzz uses -e -F1

I guess the only option is to try to use master ragel on harfbuzz 2.7.1 to make sure it doesn't break harfbuzz tests as well.

@notgull
Copy link
Collaborator Author

notgull commented Aug 24, 2023

I finally figured out how to build harfbuzz. Even after I used master ragel to regenerate the state machines the tests still passed. What?

At this point the only theories I have is that ragel-rust doesn't properly generate state machines (likely) or the tests are set up improperly (less likely). Even after generating state machines with -e -F1 the tests still failed (even though it shouldn't make any change at all, theoretically?). I'll need to investigate this further over the weekend...

@RazrFalcon
Copy link
Collaborator

I finally figured out how to build harfbuzz.

meson builddir
ninja -Cbuilddir

Don't remember how to run tests.

Even after I used master ragel to regenerate the state machines the tests still passed.

Did the output changed or is the same? Does tables have the same values in C and Rust output? It can easily be a ragel bug. Rust support is alpha, afaik.

@behdad
Copy link
Member

behdad commented Aug 24, 2023

Don't remember how to run tests.

ninja -Cbuilddir test

@notgull
Copy link
Collaborator Author

notgull commented Aug 24, 2023

Strangely, it looks like all of the issues come from indic_machine.rs. After I revert my changes to that file, all of the tests pass. So I guess I'll take a run at that particular file in another PR.

@Bobo1239
Copy link

@notgull There was a simple typo in indic_machine.rl:

macro_rules! found_syllable {
    ($kind:expr) => {{
-        found_syllable(ts, ts, &mut syllable_serial, $kind, buffer)
+        found_syllable(ts, te, &mut syllable_serial, $kind, buffer)
    }}
}

The tests pass with this.

@notgull
Copy link
Collaborator Author

notgull commented Aug 25, 2023

notgull code after spending five minutes in production a CI environment

robot fallx299

lmao fixed

@RazrFalcon
Copy link
Collaborator

@Bobo1239 great catch!

@RazrFalcon RazrFalcon merged commit 8cda617 into harfbuzz:master Aug 25, 2023
@RazrFalcon
Copy link
Collaborator

@notgull Thanks! Since it fixes the out-of-bounds bug I will make a release soon.

I also want to write a simple readme of how to use ragel. What exact ragel command did you used to generate Rust files?

@bluebear94 bluebear94 mentioned this pull request Aug 25, 2023
7 tasks
@notgull notgull deleted the ragel branch August 25, 2023 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants