Relative computed goto by caffe3 · Pull Request #533 · skvadrik/re2c

caffe3 · 2025-02-20T19:33:07Z

I noticed #531 and I thought I'd give it a go because I've been working on some of my own code generation improvements. This is purely experimental for review/feedback.

I have only done the lightest amount of testing so far (it compiles and the c++98.re seemed to work) and it has some caveats both in my current implementation and compiler support:

I have had to use typeof to keep C++ happy without introducing another type configuration variable to re2c
Clang's heuristics do not work very well with the relative offsets

@crrodriguez would you like to give this a spin to see if it works in your use-case?

To use this, add to a block, and enable -g for re2c:

re2c:cgoto:relative = 1

crrodriguez · 2025-02-20T21:48:42Z

Great I'll give a try..

crrodriguez · 2025-02-21T01:09:13Z

unfortunately gnu AS seems to choke on it.. I tried with the PHP parsers.. chokes on var_unserializer parser..I'll see if it is worth reporting..

caffe3 · 2025-02-21T04:43:15Z

Hi @crrodriguez - thanks, what sort of errors do you see (are they parsing errors or memory errors)? I'll have some time tomorrow to test it further.

I haven't regenerated the test output, there's some output differences with current master, but semantically equivalent - is that what you are seeing when you run the tests?

caffe3 · 2025-02-21T07:05:14Z

This seems to have an interesting effect on executable size (gcc 14.2.0 with -Os):

810592 Feb 21 07:51 uri_rfc3986 (re2c 4.0.2 -g)
101912 Feb 21 07:51 uri_rfc3986_2 (re2c draft-cgoto-relative -g --syntax usc) # replaced yytargetoffset with "static const unsigned short")
146968 Feb 21 07:52 uri_rfc3986_3 (re2c draft-cgoto-relative -g) # yytargetoffset with "static const int")

Maybe this is really worth investigating further?

skvadrik

Thanks for your work @caffe3 ! I have a few comments, see below.

skvadrik · 2025-02-21T06:57:33Z

+        buf.cstr("(typeof(*").cstr(name).cstr("))((char *)&&").str(opts->label_prefix).u32(go->table[i]->label->index);
+        if (opts->computed_gotos_relative) {
+            buf.cstr(" - (char *)&&").str(opts->label_prefix).u32(min_index);
+        }
+        elems[i] = buf.cstr(")").flush();


Don't change code generation for the absolute case (you can see the changes if you run ./run_tests.py or make check).

I need to test this still, so I'll leave this open for tracking.

skvadrik · 2025-02-21T08:46:50Z

This seems to have an interesting effect on executable size (gcc 14.2.0 with -Os):

810592 Feb 21 07:51 uri_rfc3986 (re2c 4.0.2 -g)
101912 Feb 21 07:51 uri_rfc3986_2 (re2c draft-cgoto-relative -g --syntax usc) # replaced yytargetoffset with "static const unsigned short")
146968 Feb 21 07:52 uri_rfc3986_3 (re2c draft-cgoto-relative -g) # yytargetoffset with "static const int")

Maybe this is really worth investigating further?

It is! Since we know all the offsets, we should be able to find the smallest integer type for the table. The change in binary size seems drastic, I'm surprized to see it.

caffe3 · 2025-02-21T09:35:43Z

It is! Since we know all the offsets, we should be able to find the smallest integer type for the table. The change in binary size seems drastic, I'm surprized to see it.

The same thought came to mind but I think that would only work with C++ if at all, unfortunately. But for most small/medium-sized parsers uint16_t should be sufficient if the compiler doesn't decide to rearrange the earliest label, otherwise int16_t is another reasonable choice.

skvadrik · 2025-02-21T09:51:23Z

The same thought came to mind but I think that would only work with C++ if at all, unfortunately. But for most small/medium-sized parsers uint16_t should be sufficient if the compiler doesn't decide to rearrange the earliest label, otherwise int16_t is another reasonable choice.

Ah, right, I forgot we're not on the assembly level and we don't really know the size of the generated code and how far the labels will be. Other options that we can consider:

an estimate of the function size based on the number of states
a configuration to set yytarget type
assertions to make sure yytarget is not overflown

This is all follow-up work that should not get in the way of the initial implementation, though.

BTW why is it different between C and C++?

caffe3 · 2025-02-21T11:23:19Z

BTW why is it different between C and C++?

I was wondering if it was possible to use C++'s constexpr magic to determine the size of the offset values but it isn't possible after some tests - it doesn't treat && as constexpr values.

crrodriguez · 2025-02-21T12:27:53Z

Hi @crrodriguez - thanks, what sort of errors do you see (are they parsing errors or memory errors)? I'll have some time tomorrow to test it further.

I haven't regenerated the test output, there's some output differences with current master, but semantically equivalent - is that what you are seeing when you run the tests?

It simply fails to assemble var_unserializer PHP parser..

ext/standard/var_unserializer.lo -MMD -MF ext/standard/var_unserializer.dep -MT ext/standard/var_unserializer.lo /tmp/ccTSF67I.s: Assembler messages: /tmp/ccTSF67I.s:11293: Error: can't resolve .text.unlikely - .L166

I need to check if it is a thing that happens with one of the CFLAGS or something.
I 'll collect the debug logs and report an assembler bug as soon as I have time to dig into it.

The binary size change is actually pretty nice, what is surprising is the amount.

crrodriguez · 2025-02-21T16:56:24Z

Regenerating the zend_language_parser.l from PHP with this options results in a parser that compiles but crashes. .. needs further investigation.

caffe3 · 2025-02-21T17:06:32Z

Regenerating the zend_language_parser.l from PHP with this options results in a parser that compiles but crashes. .. needs further investigation.

Thanks for testing! Could you give me the version of PHP + compiler you used to build it and any other environment information that could potentially affect it? And some commands that can reproduce the crash - I'm not terribly familiar with php.

crrodriguez · 2025-02-21T17:25:19Z

Regenerating the zend_language_parser.l from PHP with this options results in a parser that compiles but crashes. .. needs further investigation.

Thanks for testing! Could you give me the version of PHP + compiler you used to build it and any other environment information that could potentially affect it? And some commands that can reproduce the crash - I'm not terribly familiar with php.

Im using the latest php-src master branch + this re2c PR + gcc 15

caffe3 · 2025-02-22T22:17:28Z

Some work in progress - I've generalised some of the code, but should be functionally the same so far (barring bugs I've introduced).

I now optimise away when the offset resolves to 0 from the base
I do a naive conversion from the yytarget type to the cast type by stripping storage specifiers.

skvadrik

Looks good, only a few minor issues, plus I think you forgot to include regenerated bootstrap files (syntax files, lexers, docs).

skvadrik · 2025-02-24T19:26:32Z

+            buf.cstr("(int)("); // TODO: port to syntax file
+            buf.cstr("(char*)");
+            buf.cstr("&&").str(opts->label_prefix).u32(go->table[i]->label->index);
+            buf.cstr(" - (char*)");
+            buf.cstr("&&").str(opts->label_prefix).u32(min_index);
+            buf.cstr(")");


Formatting issue: can you just glue it together into one statement, breaking the lines at 100 char limit (the way it's formatted everywhere else)? The comment should go on the line before the statement and say something like TODO: port this to syntax files and replace hardcoded `int` with `code:type_yytarget`.

Are the following lines just indented by 4 or aligned?

Use 4-space indent (the way you've done now is good). I'm not sure what you mean by aligned though.

Great, thanks! I meant if the '.' on each line were aligned.

skvadrik · 2025-02-24T19:32:31Z

+                buf.cstr("(int)("); // TODO: port to syntax file
+                buf.cstr("(char*)");
+                buf.cstr("&&").str(opts->label_prefix).u32(acc[i].state->label->index);
+                buf.cstr(" - (char*)");
+                buf.cstr("&&").str(opts->label_prefix).u32(min_index);
+                buf.cstr(")");


Same comment as for lines 766-771.

skvadrik · 2025-02-24T19:33:43Z

+            buf.cstr("(int)("); // TODO: port to syntax file
+            buf.cstr("(char*)");
+            buf.cstr("&&").str(opts->cond_label_prefix).str(conds[i].name);
+            buf.cstr(" - (char*)").str(opts->cond_label_prefix).str(conds.front().name);
+            buf.cstr(")");


Same comment as for lines 766-771.

Instead of using absolute pointers are used in the cgoto tables, we generate relative offsets from the earliest label offset within the code. This offset type is hardcoded as `int' currently.

skvadrik · 2025-02-25T19:49:26Z

Thanks for your patience addressing all the comments @caffe3 !

crrodriguez · 2025-03-04T18:46:00Z

Hi @crrodriguez - thanks, what sort of errors do you see (are they parsing errors or memory errors)? I'll have some time tomorrow to test it further.
I haven't regenerated the test output, there's some output differences with current master, but semantically equivalent - is that what you are seeing when you run the tests?

It simply fails to assemble var_unserializer PHP parser..

ext/standard/var_unserializer.lo -MMD -MF ext/standard/var_unserializer.dep -MT ext/standard/var_unserializer.lo /tmp/ccTSF67I.s: Assembler messages: /tmp/ccTSF67I.s:11293: Error: can't resolve .text.unlikely - .L166

I took a look at this again.. it is a bad interaction between this code generation and always_inline functions.. alays_inline function gets ..well..always inlined and vanishes from the resulting asm..

skvadrik · 2025-03-05T06:49:20Z

I took a look at this again.. it is a bad interaction between this code generation and always_inline functions.. alays_inline function gets ..well..always inlined and vanishes from the resulting asm..

@crrodriguez Did you try with the final state of this PR that has been merged? I think @caffe3 fixed some error and tested that it works with php.

caffe3 · 2025-03-05T07:20:24Z

I think @caffe3 fixed some error and tested that it works with php.

I haven't found the time to test with PHP yet - PHP's build system looked more complicated than I originally thought rather than just ~~clone & configure & build~~. Edit: Oh, maybe I was looking at the wrong place before - but I might be able to get some testing done today.

But @crrodriguez - if you haven't already pulled the latest, please do, because there was a bug in the initial implementation which would have caused a segfault.

caffe3 · 2025-03-05T08:05:20Z

Hi @crrodriguez - I reproduced your issue. I strongly suspect GCC has a bug where it is still reordering functions into new sections despite being used within relative gotos.

I fixed it by disabling reordering with -fno-reorder-functions:

diff --git a/configure.ac b/configure.ac
index 01d9ded69b..6675e802b7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -155,13 +155,13 @@ PHP_PROG_PHP([7.4])
 PHP_ARG_ENABLE([re2c-cgoto],
   [whether to enable computed goto extension with re2c],
   [AS_HELP_STRING([--enable-re2c-cgoto],
-    [Enable re2c -g flag to optimize conditional jumps using computed goto
+    [Enable re2c --computed-gotos-relative flag to optimize conditional jumps using computed goto
     extension, if supported by the compiler])],
   [no],
   [no])

 AS_VAR_IF([PHP_RE2C_CGOTO], [no],,
-[AC_CACHE_CHECK([whether re2c -g works], [php_cv_have_re2c_cgoto],
+[AC_CACHE_CHECK([whether re2c --computed-gotos-relative works], [php_cv_have_re2c_cgoto],
   [AC_COMPILE_IFELSE([AC_LANG_SOURCE([[
 int main(void)
 {
@@ -176,7 +176,7 @@ label2:
   [php_cv_have_re2c_cgoto=yes],
   [php_cv_have_re2c_cgoto=no])])
 AS_VAR_IF([php_cv_have_re2c_cgoto], [yes],
-  [AS_VAR_APPEND([RE2C_FLAGS], [" -g"])])
+  [AS_VAR_APPEND([RE2C_FLAGS], [" --computed-gotos-relative"])])
 ])

 dnl Platform-specific compile settings.
@@ -201,6 +201,9 @@ esac
 dnl See https://github.com/php/php-src/issues/14140
 AX_CHECK_COMPILE_FLAG([-ffp-contract=off], [CFLAGS="$CFLAGS -ffp-contract=off"])

+dnl Because it breaks re2c --computed-gotos-relative otherwise
+AX_CHECK_COMPILE_FLAG([-fno-reorder-functions], [CFLAGS="$CFLAGS -fno-reorder-functions"])
+
 dnl Mark symbols hidden by default if the compiler (for example, gcc >= 4)
 dnl supports it. This can help reduce the binary size and startup time.
 AX_CHECK_COMPILE_FLAG([-fvisibility=hidden],

I ran make test and got:

=====================================================================
TIME END 2025-03-05 08:03:47

=====================================================================
TEST RESULT SUMMARY
---------------------------------------------------------------------
Exts skipped    :    43 (bcmath, bz2, calendar, com_dotnet, curl, dba, dl_test, enchant, exif, ffi, ftp, gd, gettext, gmp, intl, ldap, mbstring, mysqli, mysqlnd, odbc, openssl, pcntl, pdo_dblib, pdo_firebird, pdo_mysql, pdo_odbc, pdo_pgsql, pgsql, readline, shmop, skeleton, snmp, soap, sockets, sodium, sysvmsg, sysvsem, sysvshm, tidy, xsl, zend_test, zip, zlib)
Exts tested     :    26
---------------------------------------------------------------------

Number of tests : 20449             14604
Tests skipped   :  5845 ( 28.6%) --------
Tests warned    :     0 (  0.0%) (  0.0%)
Tests failed    :     0 (  0.0%) (  0.0%)
Expected fail   :     8 (  0.0%) (  0.1%)
Tests passed    : 14596 ( 71.4%) ( 99.9%)
---------------------------------------------------------------------
Time taken      : 338.225 seconds
=====================================================================

skvadrik · 2025-03-05T08:31:50Z

I strongly suspect GCC has a bug where it is still reordering functions into new sections despite being used within relative gotos.

I thought relative gotos are good precisely because they don't care about the absolute location of the function, as they are relative to the start of the function. So why would function order matter here?

If you suspect a compiler bug, can you get a self-contained C/C++ file so we can compile it with/without -fno-reorder-functions, compare the generated code and hopefully minimize it and report a bug to GCC?

caffe3 · 2025-03-05T09:05:49Z

I thought relative gotos are good precisely because they don't care about the absolute location of the function, as they are relative to the start of the function. So why would function order matter here?

I think the flag is a misnomer - GCC puts different parts of the code into different .text sections to optimise whether its a hot/cold/unlikely path. ~~I speculate that~~ this lets the linker potentially reorder the functions later (edit: it does, and it partitions functions as well!).

The changes in the assembly are very small: https://gist.github.com/caffe3/dccc0f1908dcdc9b2f39c371c102ac46

If you suspect a compiler bug, can you get a self-contained C/C++ file so we can compile it with/without -fno-reorder-functions, compare the generated code and hopefully minimize it and report a bug to GCC?

Yes, I think that is possible - the assembly currently stands at around 40k lines... and I think only gets triggered if the code of the function is big enough though.

caffe3 · 2025-03-05T09:40:22Z

Unfortunately there's no per-function flag that I can see helps with this instance. I tried no_reorder, noclone, noinline, hot and none of them prevent the incorrect assembly, except:

static __attribute__((cold)) int php_var_unserialize_internal(UNSERIALIZE_PARAMETER)

crrodriguez · 2025-03-05T11:58:22Z

I took a look at this again.. it is a bad interaction between this code generation and always_inline functions.. alays_inline function gets ..well..always inlined and vanishes from the resulting asm..

@crrodriguez Did you try with the final state of this PR that has been merged? I think @caffe3 fixed some error and tested that it works with php.

Yes, I tried with the revision that was merged or master branch. not using zend_always_inline worked around the problem for me with gcc15, I also believe this to be a compiler bug.

caffe3 · 2025-03-05T12:06:57Z

The GCC documentation states:

The &&foo expressions for the same label might have different values if the containing function is inlined or cloned. If a program relies on them being always the same, attribute((noinline,noclone)) should be used to prevent inlining and cloning. If &&foo is used in a static variable initializer, inlining and cloning is forbidden.

What did you do to avoid zend_always_inline ? It isn't an attribute on the affected function, php_var_unserialize_internal.

skvadrik · 2025-03-05T22:16:17Z

I think the flag is a misnomer - GCC puts different parts of the code into different .text sections to optimise whether its a hot/cold/unlikely path. I speculate that this lets the linker potentially reorder the functions later (edit: it does, and it partitions functions as well!).

The GCC documentation states:

The &&foo expressions for the same label might have different values if the containing function is inlined or cloned. If a program relies on them being always the same, attribute((noinline,noclone)) should be used to prevent inlining and cloning. If &&foo is used in a static variable initializer, inlining and cloning is forbidden.

We don't rely on &&foo expressions being the same always, we just rely on them being the same within one instance of the re2c-generated function. Is GCC splitting the function, then inlining / cloning some parts of it, so that effectively &&foo expressions become different within the same function? If so, it does look like a bug.

caffe3 · 2025-03-06T11:11:32Z

Removing __attribute__((cold)) from the below makes it compile:

__attribute__((cold)) php_error_docref();
php_var_unserialize() {
  static yytarget_0 = (char *) && yy14 - (char *) && yy1;
  goto *&&yy1 + yytarget_0;
yy1:
yy2:
  goto yy2;
yy14:
  php_error_docref();
}

skvadrik · 2025-04-14T15:54:45Z

Link to the GCC bug report: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=119158 for the record.

skvadrik requested changes Feb 21, 2025

View reviewed changes

skvadrik requested changes Feb 22, 2025

View reviewed changes

skvadrik requested changes Feb 23, 2025

View reviewed changes

Comment thread src/codegen/pass2_generate.cc Outdated

Comment thread test/config/config7c.c Outdated

skvadrik reviewed Feb 23, 2025

View reviewed changes

Comment thread src/options/opt.h Outdated

caffe3 force-pushed the draft-cgoto-relative branch from 06e10ff to 1a92b05 Compare February 24, 2025 19:00

caffe3 changed the title ~~Relative computed goto (experimental)~~ Relative computed goto Feb 24, 2025

caffe3 force-pushed the draft-cgoto-relative branch from 1a92b05 to 9a2c785 Compare February 24, 2025 19:13

skvadrik requested changes Feb 24, 2025

View reviewed changes

caffe3 added 3 commits February 25, 2025 19:32

Add cgoto:relative to enable generating relative offset tables

791e0bb

Instead of using absolute pointers are used in the cgoto tables, we generate relative offsets from the earliest label offset within the code. This offset type is hardcoded as `int' currently.

Update the docs about cgoto:relative and --computed-gotos-relative

b650999

Regenerate bootstrap for relative computed gotos.

e2c84f7

caffe3 force-pushed the draft-cgoto-relative branch from 9a2c785 to e2c84f7 Compare February 25, 2025 18:42

skvadrik marked this pull request as ready for review February 25, 2025 19:26

skvadrik approved these changes Feb 25, 2025

View reviewed changes

skvadrik merged commit f505631 into skvadrik:master Feb 25, 2025

caffe3 deleted the draft-cgoto-relative branch February 26, 2025 18:36

Conversation

caffe3 commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crrodriguez commented Feb 20, 2025

Uh oh!

crrodriguez commented Feb 21, 2025

Uh oh!

caffe3 commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

caffe3 commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skvadrik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skvadrik commented Feb 21, 2025

Uh oh!

caffe3 commented Feb 21, 2025

Uh oh!

skvadrik commented Feb 21, 2025

Uh oh!

caffe3 commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crrodriguez commented Feb 21, 2025

Uh oh!

crrodriguez commented Feb 21, 2025

Uh oh!

caffe3 commented Feb 21, 2025

Uh oh!

crrodriguez commented Feb 21, 2025

Uh oh!

caffe3 commented Feb 22, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skvadrik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

skvadrik commented Feb 25, 2025

Uh oh!

crrodriguez commented Mar 4, 2025

Uh oh!

skvadrik commented Mar 5, 2025

Uh oh!

caffe3 commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

caffe3 commented Feb 20, 2025 •

edited

Loading

caffe3 commented Feb 21, 2025 •

edited

Loading

caffe3 commented Feb 21, 2025 •

edited

Loading

caffe3 commented Feb 21, 2025 •

edited

Loading

caffe3 commented Mar 5, 2025 •

edited

Loading

caffe3 commented Mar 5, 2025 •

edited

Loading

caffe3 commented Mar 5, 2025 •

edited

Loading