From 93b866dee88c9cd6273b964ef444ccceb2e12b12 Mon Sep 17 00:00:00 2001 From: Ulya Trofimovich Date: Sat, 7 Nov 2015 22:48:42 +0000 Subject: [PATCH] Added [-Wcondition-order] description. --- .../warnings/condition_order/how_it_works.rst | 12 +++ .../warnings/condition_order/real_world.rst | 89 +++++++++++++++++++ .../condition_order/simple_example.rst | 86 ++++++++++++++++++ .../condition_order/wcondition_order.re | 34 +++++++ .../condition_order/wcondition_order.rst | 10 +++ src/manual/warnings/warnings.rst | 2 +- src/manual/warnings/wcondition_order.rst | 5 -- 7 files changed, 232 insertions(+), 6 deletions(-) create mode 100644 src/manual/warnings/condition_order/how_it_works.rst create mode 100644 src/manual/warnings/condition_order/real_world.rst create mode 100644 src/manual/warnings/condition_order/simple_example.rst create mode 100644 src/manual/warnings/condition_order/wcondition_order.re create mode 100644 src/manual/warnings/condition_order/wcondition_order.rst delete mode 100644 src/manual/warnings/wcondition_order.rst diff --git a/src/manual/warnings/condition_order/how_it_works.rst b/src/manual/warnings/condition_order/how_it_works.rst new file mode 100644 index 00000000..bd820c7e --- /dev/null +++ b/src/manual/warnings/condition_order/how_it_works.rst @@ -0,0 +1,12 @@ +How it works +~~~~~~~~~~~~ + +The warning is triggered if at the same time: + +* Conditions are enabled with ``-c``. +* Neither ``/*!types:re2c*/``, nor ``-t, --type-header`` is used. +* Initial dispatch on conditions in sensitive to condition order: + it is either an ``if`` statement or a jump table. + Note that the number of conditions must be greater than one, + otherwise dispatch shrinks to a simple unconditional jump. + diff --git a/src/manual/warnings/condition_order/real_world.rst b/src/manual/warnings/condition_order/real_world.rst new file mode 100644 index 00000000..11f663d5 --- /dev/null +++ b/src/manual/warnings/condition_order/real_world.rst @@ -0,0 +1,89 @@ +Real-world examples +~~~~~~~~~~~~~~~~~~~ + +The best real-world example is a story of how ``[-Wcondition-order]`` was added to re2c. + +One day I decided to change condition numbering scheme. +It was only natural: re2c assigns numbers to conditions in the order they appear in code. +This is not very convenient, because elsewhere in the code condition names (not numbers) are used as unique identifiers. +Names are sorted lexicographically, so the original condition order is not preserved. +It takes extra care to remember the mapping of names to numbers. +So why not just drop numbers and sort conditions by their names? +No one cares for condition order anyway: since re2c-generated code uses condition names, +they must be defined, and why would anyone define them by hand if they can do it automatically. + +Or wait. +Almost occasionally I found a real-world program that does use fixed condition numbers. +Here is the carcass of this program: + +.. code-block:: cpp + :number-lines: + + #define NORMAL 0 + #define PRE_RAW 1 + #define RAW 2 + #define INITIAL 3 + + /*!re2c + re2c:yyfill:check = 0; + + T_TRUE 'true' + T_YES 'yes' + T_ON 'on' + T_ENABLED 'enabled' + T_FALSE 'false' + T_NO 'no' + T_OFF 'off' + T_DISABLED 'disabled' + T_EVAL 'ev' + T_SHELL 'sh' + T_IF 'if' + T_RUN 'run' + T_RUN_SHORT "r" + WS [ \r\n\t]+ + DIGITS [-]?[0-9\.]+ + ID [^ \r\n\t:#\000]+ + ADDR [0][x][a-fA-F0-9]+ + OPCODE (ZEND_|zend_)([A-Za-z])+ + INPUT [^\n\000]+ + + {} + <*>{WS}?[\n\000] {} + [-][r]{WS}?{DIGITS} {} + {T_IF}{WS} {} + {ID}[:]{1}[//]{2} {} + [#]{1} {} + [:]{2} {} + [:]{1} {} + ({T_YES}|{T_ON}|{T_ENABLED}|{T_TRUE}){WS} {} + ({T_NO}|{T_OFF}|{T_DISABLED}|{T_FALSE}){WS} {} + {DIGITS} {} + {ADDR} {} + {OPCODE} {} + {ID} {} + {INPUT} {} + <*>{WS} {} + {T_EVAL}{WS} {} + {T_SHELL}{WS} {} + ({T_RUN}|{T_RUN_SHORT}){WS} {} + . {} + . {} + */ + +Curiously, the program somehow avoids using re2c-generated condition names: +even ``YYSETCONDITION`` is called manually instead of using ``=>`` in rules. +The program uses ``-g`` option, so the initial dispatch does depend on condition order: + +.. code-block:: cpp + + static void *yyctable[4] = { + &&yyc_NORMAL, + &&yyc_PRE_RAW, + &&yyc_RAW, + &&yyc_INITIAL, + }; + goto *yyctable[YYGETCONDITION()]; + +Of course, re2c will keep to the old condition numbering scheme (at least in the nearest future). +Who wants to break the old code? ``:)`` + diff --git a/src/manual/warnings/condition_order/simple_example.rst b/src/manual/warnings/condition_order/simple_example.rst new file mode 100644 index 00000000..10da472b --- /dev/null +++ b/src/manual/warnings/condition_order/simple_example.rst @@ -0,0 +1,86 @@ +A simple example +~~~~~~~~~~~~~~~~ + +The following lexer consists of two conditions: ``a`` and ``b``. +It starts in condition ``a``, which expects a sequence of letters ``'a'`` followed by a comma. +Comma causes transition to condition ``b``, which expects a sequence of letters ``'b'`` followed by an exclamation. +Anything else is an error. +Nothing special, except that instead of generating condition names with ``/*!types:re2c*/`` directive +or ``-t, --type-header`` option we hardcoded them manually: + +`[wcondition_order.re] `_ + +.. include:: wcondition_order.re + :code: cpp + :number-lines: + +Condition order is controlled by ``REVERSED_CONDITION_ORDER`` define. +Let's compile and run it: + +.. code-block:: + + $ re2c -c -o example.c -Wcondition-order wcondition_order.re + $ + $ gcc -o example example.c + $ ./example + aaaa,bbb! + $ + $ gcc -o example -DREVERSED_CONDITION_ORDER example.c + $ ./example + aaaa,bbb! + +Everything works fine: we get ``aaaa,bbb!`` in both cases. +However, if we use ``-s`` re2c option, lexer becomes sensitive to condition order: + +.. code-block:: + + $ re2c -cs -o example.c -Wcondition-order wcondition_order.re + re2c: warning: line 31: looks like you use hardcoded numbers instead of autogenerated condition names: better add + '/*!types:re2c*/' directive or '-t, --type-header' option and don't rely on fixed condition order. [-Wcondition-order] + $ + $ gcc -o example example.c + $ ./example + aaaa,bbb! + $ + $ gcc -o example -DREVERSED_CONDITION_ORDER example.c + $ ./example + error + +And we also get a warning from re2c. +The same behaviour (re2c warning and error with ``-DREVERSED_CONDITION_ORDER``) remains if we use ``-g`` option +(or any option that implies ``-s`` or ``-g``). +Why is that? +A look at the generated code explains everything. +Normally the inital dispatch on conditions is a ``switch`` statement: + +.. code-block:: cpp + + switch (c) { + case yyca: goto yyc_a; + case yycb: goto yyc_b; + } + +Dispatch uses explicit condition names and works no matter what numbers are assigned to them. +However, with ``-s`` option re2c generates an ``if`` statement instead of a ``switch``: + +.. code-block:: cpp + + if (c < 1) { + goto yyc_a; + } else { + goto yyc_b; + } + +And with ``-g`` option it uses jump table (computed ``goto``): + +.. code-block:: cpp + + static void *yyctable[2] = { + &&yyc_a, + &&yyc_b, + }; + goto *yyctable[c]; + +Clearly, the last two cases are sensitive to condition order. +The fix is easy: as the warning suggests, use ``/*!types:re2c*/`` directive or ``-t, --type-header`` option. + diff --git a/src/manual/warnings/condition_order/wcondition_order.re b/src/manual/warnings/condition_order/wcondition_order.re new file mode 100644 index 00000000..40a3c734 --- /dev/null +++ b/src/manual/warnings/condition_order/wcondition_order.re @@ -0,0 +1,34 @@ +#include + +#ifdef REVERSED_CONDITION_ORDER +# define yyca 1 +# define yycb 0 +#else +# define yyca 0 +# define yycb 1 +#endif + +int main() +{ + const char * YYCURSOR = "aaaa,bbb!"; + int c = yyca; + for (;;) { + /*!re2c + re2c:define:YYCTYPE = char; + re2c:yyfill:enable = 0; + re2c:define:YYSETCONDITION = "c = @@;"; + re2c:define:YYSETCONDITION:naked = 1; + re2c:define:YYGETCONDITION = c; + re2c:define:YYGETCONDITION:naked = 1; + + <*> * { printf ("error\n"); break; } + + "a" { printf ("a"); continue; } + "," => b { printf (","); continue; } + + "!" { printf ("!\n"); break; } + "b" { printf ("b"); continue; } + */ + } + return 0; +} diff --git a/src/manual/warnings/condition_order/wcondition_order.rst b/src/manual/warnings/condition_order/wcondition_order.rst new file mode 100644 index 00000000..18898307 --- /dev/null +++ b/src/manual/warnings/condition_order/wcondition_order.rst @@ -0,0 +1,10 @@ +[-Wcondition-order] +-------------------------- + +.. include:: ../home.rst +.. include:: ../../../contents.rst + +.. include:: simple_example.rst +.. include:: how_it_works.rst +.. include:: real_world.rst + diff --git a/src/manual/warnings/warnings.rst b/src/manual/warnings/warnings.rst index e59fc5db..3577d2bd 100644 --- a/src/manual/warnings/warnings.rst +++ b/src/manual/warnings/warnings.rst @@ -8,7 +8,7 @@ Warnings * `[-Wundefined-control-flow] `_ * `[-Wunreachable-rules] `_ -* `[-Wcondition-order] `_ +* `[-Wcondition-order] `_ * `[-Wuseless-escape] `_ * `[-Wswapped-range] `_ * `[-Wempty-character-class] `_ diff --git a/src/manual/warnings/wcondition_order.rst b/src/manual/warnings/wcondition_order.rst deleted file mode 100644 index 5d6e7cdb..00000000 --- a/src/manual/warnings/wcondition_order.rst +++ /dev/null @@ -1,5 +0,0 @@ -[-Wcondition-order] --------------------------- - -.. include:: home.rst - -- 2.40.0