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

Fix all ASAN issues in vectorscan #93

Merged
merged 3 commits into from
Apr 18, 2022
Merged

Fix all ASAN issues in vectorscan #93

merged 3 commits into from
Apr 18, 2022

Conversation

danlark1
Copy link

@danlark1 danlark1 commented Feb 18, 2022

Closes #91
Closes #100

There were lots of problems in small ranges, I tried to fix them all by carefully reading the code.

Also adopt all changes we made to make it compile like setbit unsetting in tests.

We tested with all combinations HS_OPTIMIZE/DEBUG/HASWELL/SSE4.2/ARM/ASAN/MSAN/NOSANITIZERS. Haven't tested AVX512, PPC and SVE2

Resubmitting to a develop branch. I'll take a look at AVX512 failure from FAT_RUNTIME

@markos
Copy link

markos commented Feb 18, 2022

It would be great if you could also share how you enabled and tested ASAN in the builds, I would love to add this to our CI for all platforms even, it might take a bit longer, but we're in the process of adding more cores to our build farm so it will balance out.

@danlark1
Copy link
Author

danlark1 commented Feb 18, 2022

-DSANITIZE=undefined
-DSANITIZE=address
-DSANITIZE=memory

to cmake command

Some might not work with the ifunc attribute so FAT_RUNTIME might be hard to test

And I tested only clang

@danlark1
Copy link
Author

I believe I fixed all tests :)

@danlark1
Copy link
Author

Overall feedback: I might guess I missed in a couple places some mask lowering because of zero byte inputs, however, in general, it took a while lot to comprehend and to match what was missing

Can you share what is the idea behind such major rewrites? I thought Vectorscan just replaced SIMD to portable ones and was very surprised to see ASAN issues and then I realized these major string search algos were rewritten.

Given hyperscan might be supported in the future, it makes new integrations much more complex and even if hyperscan decides not to support ARM for other reasons, merges will become exponentially complex. I saw just a couple optimizations which are missed in hyperscan and presented here. In the end we will have different bugs in two repos and from our view it makes just life more complex

Anyway, thanks for your work on portability, no issues with this, at least

@markos
Copy link

markos commented Feb 18, 2022

well, the idea with the refactoring is that original hyperscan is heavily tailored against Intel SIMD, using movemasks which are extremely expensive to emulate on Arm and Power. My initial fork was indeed just a SIMD port, but performance on Arm suffered heavily. After a lot of searching we decided we care only about API/ABI compatibility and would rather get better performance on other platforms even if it makes it much harder to integrate code back into Vectorscan, when Intel release a new version. In the unlikely event that Hyperscan decides to become more favourable against other architectures in the future, we will still support Vectorscan as I have found multiple places that performance would increase and code size reduced. I will just note that code size in the refactored places was reduced to almost 1/3rd of the original, and keeping more or less the same performance on Intel, but increasing performance on our Arm systems by almost 200% in some microbenchmarks. I could not do that with just SIMD intrinsic implementations without refactoring the algorithms themselves. Furthermore, SIMD engines like SVE2 require much heavier refactorings than even the current because of the major breakthroughs in the logic, especially head and tail of the loops, as they use predicates. The original code, where all the loop functions are implemented for each SIMD engine just does not scale.
Also, by being free from the limitations of the original project we are able to only keep relevant code -eg. Windows support was completely removed as unneeded and irrelevant. I'm actually pondering whether AVX512 support is actually beneficial at all, it makes the code much more complicated and so far the benefit in all but a few benchmarks has been negligible. But that's a decision for the future.
In short, expect more and heavier refactoring, however with the goal to always maintain API/ABI compatibility.

@markos
Copy link

markos commented Feb 18, 2022

Having said that, I should note that I will be adding ASAN to the CI, so such bugs should be caught early on next time. I do honestly appreciate the pointer for that and the PR of course.

@danlark1
Copy link
Author

Thanks

I have also some fuzzing targets, I will try to find all differences between hyperscan and Vectorscan in terms of output

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.

3 participants