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

sequential_or: Fixed random order execution of underlying parsers #310

Merged
merged 1 commit into from
Dec 8, 2017

Conversation

Kojoley
Copy link
Collaborator

@Kojoley Kojoley commented Dec 4, 2017

After two hours of fighting with the optimizer, I happily drew attention to
this little nifty bitwise operator in any_if_ns function.

Explanation of the bug: bitwise inclusive OR operator is not a sequence point
(per chapter §5.13 of C++14 standard), that's why at compiling the expression
a() | b() | ... | z() optimizer is allowed to rearrange the execution order
of the functions a, b ... z.

There is high chance that a lot of people were misguided by the bug and chose
not to use sequential_or.

I vaguely remember how about three years ago I thought that I am dumb and/or
the documentation is wrong when I tried to use the sequential_or but ended
with some workaround.

There are three possible fixes:

  • This one
  • Make the original any_ns and any_if_ns strict ordered
    (could potentially make permutations operator slower)
  • Break the any_ns and any_if_ns API and somehow generalize the code

@djowel
Copy link
Member

djowel commented Dec 4, 2017

Very interesting insights! I honestly haven't thought about this until now! Do we have tests for this?

After two hours of fighting with the optimizer, I happily drew attention to
this little nifty bitwise operator in `any_if_ns` function.

Explanation of the bug: bitwise inclusive OR operator is not a sequence point
(per chapter §5.13 of C++14 standard), that's why at compiling the expression
`a() | b() | ... | z()` optimizer is allowed to rearrange the execution order
of the functions `a`, `b` ... `z`.

There is high chance that a lot of people were misguided by the bug and chose
not to use `sequential_or`.

I vaguely remember how about three years ago I thought that I am dumb and/or
the documentation is wrong when I tried to use the `sequential_or` but ended
with some workaround.

There are three possible fixes:
  - This one
  - Make the original `any_ns` and `any_if_ns` strict ordered
    (could potentially make permutations operator slower)
  - Break the `any_ns` and `any_if_ns` API and somehow generalize the code
@Kojoley Kojoley force-pushed the sequential_or-random-order-bug branch from eb44967 to 6a37fde Compare December 4, 2017 15:29
@Kojoley Kojoley changed the title sequental_or: Fixed random order execution of underlying parsers sequential_or: Fixed random order execution of underlying parsers Dec 4, 2017
@Kojoley
Copy link
Collaborator Author

Kojoley commented Dec 4, 2017

There cannot be a test for this particular problem. For me it happened on sequential_or tests. The bug depends on compiler optimization strategy, inlining aggressiveness and phase of the moon .

@Kojoley
Copy link
Collaborator Author

Kojoley commented Dec 4, 2017

Or if someone know a test for i++ + ++i problem - I can adopt it for our case.

Example:

#include <cstdio>

int main()
{
    return printf("Hello") | printf("World");
}

The example produces "WorldHello" for me on MSVC on any optimization strategy except Od and with any inlining strategy, but it prints "HelloWorld" on GCC and Clang with everything I have tried.

From my point of view if such a test exists - it requires some preprocessor pragma or compiler argument. Actually this is a good idea for an in-compiler-fuzzer, but again, the bug will appear on already existing tests and does not require a dedicated test.

@djowel
Copy link
Member

djowel commented Dec 4, 2017

OK, well, I haven't studied your solution yet, but could you give me a quick explanation on how the solution works?

@Kojoley
Copy link
Collaborator Author

Kojoley commented Dec 5, 2017

It is very simple, the crucial part is b1c74d0.

@Kojoley
Copy link
Collaborator Author

Kojoley commented Dec 7, 2017

I cannot setup Appveyor on the repository until the problem is fixed. Should I remove sequence_or tests from the test suit on CI?

@djowel
Copy link
Member

djowel commented Dec 8, 2017

I understand now. Sorry for the delay. Please go ahead.

@Kojoley Kojoley merged commit d3cbd18 into boostorg:develop Dec 8, 2017
@Kojoley Kojoley deleted the sequential_or-random-order-bug branch December 8, 2017 16:48
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.

None yet

2 participants