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 the address sanitizer problems - part 1 #2101

Merged
merged 2 commits into from Jan 5, 2023

Conversation

smhdfdl
Copy link
Contributor

@smhdfdl smhdfdl commented Dec 1, 2022

This PR is intended to address the SIMD unit test sanitizer problem identified in issue #2019, and any other sanitizer problems that are being thrown up when running other RapidJSON unit tests under valgrind . #2019 contains the list but in summary:

  1. SIMD.SkipWhitespace
  1. Uri.Parse_UTF16 with std::string
  • Invalid read of size 32

I am seeing all these on master branch (Fedora 36).

@smhdfdl
Copy link
Contributor Author

smhdfdl commented Dec 1, 2022

Commit a7ce844 addresses the main SIMD test sanitizer problem (thanks to @ylavic for the code).

It also changes one of the Uri tests to pass in an allocator where it was missing (note this does not fix the Uri sanitizer problem).

@smhdfdl
Copy link
Contributor Author

smhdfdl commented Dec 1, 2022

@ylavic @miloyip @hcoona are you able to help track down the remaining problems please?

@smhdfdl
Copy link
Contributor Author

smhdfdl commented Dec 1, 2022

Update on the valgrind failures in Uri.Parse_UTF16. The cause is nothing to do with the Uri class. I can recreate the problem using the following test case. The test passes, but valgrind thinks there is a problem with the compare of the underlying memory. It only happens when the string being compared is less than 8 chars.

TEST(Uri, Parse_UTF16_Std) {
    typedef GenericValue<UTF16<> > Value16;
    typedef std::basic_string<Value16::Ch> String;

    String short1 = L"abcd";
    String short2 = L"abcd";
    EXPECT_TRUE(short1 == short2);
}

@miloyip
Copy link
Collaborator

miloyip commented Dec 1, 2022

Update on the valgrind failures in Uri.Parse_UTF16. The cause is nothing to do with the Uri class. I can recreate the problem using the following test case. The test passes, but valgrind thinks there is a problem with the compare of the underlying memory. It only happens when the string being compared is less than 8 chars.

TEST(Uri, Parse_UTF16_Std) {
    typedef GenericValue<UTF16<> > Value16;
    typedef std::basic_string<Value16::Ch> String;

    String short1 = L"abcd";
    String short2 = L"abcd";
    EXPECT_TRUE(short1 == short2);
}

It looks like a false alarm. This piece of code seems just using C++ standard library.

@smhdfdl
Copy link
Contributor Author

smhdfdl commented Dec 2, 2022

Commit 417bf54 suppresses the failure from uritest.cpp. Suspect there's an optimisation going on that is fooling valgrind into thinking there is a real problem.

@smhdfdl
Copy link
Contributor Author

smhdfdl commented Dec 2, 2022

That leaves SIMD.SkipWhitespace other failures ('Conditional jump or move depends on uninitialised value(s)' &
'Use of uninitialised value of size 8'). When I run the tests, it automatically chooses SSE42. I notice that valgrind.supp already contains entries for Cond & Addr8 for SSE2. So @miloyip are we already suppressing the 'same' failure for SSE2?

@smhdfdl
Copy link
Contributor Author

smhdfdl commented Dec 16, 2022

@miloyip Your thoughts please

1 similar comment
@smhdfdl
Copy link
Contributor Author

smhdfdl commented Jan 5, 2023

@miloyip Your thoughts please

@miloyip
Copy link
Collaborator

miloyip commented Jan 5, 2023

I am unsure about the suppression situation.
Adding alignment to input buffer in this PR should help. But not sure if it can fix all valgrind problems.
If this PR can improve some situations that you encountered, I think we may merge it first.

@smhdfdl
Copy link
Contributor Author

smhdfdl commented Jan 5, 2023

@miloyip I agree, let's review & merge this first.

@smhdfdl smhdfdl marked this pull request as ready for review January 5, 2023 11:22
@smhdfdl smhdfdl changed the title Fix the address sanitizer problems Fix the address sanitizer problems - part 1 Jan 5, 2023
@miloyip miloyip merged commit 1ce516e into Tencent:master Jan 5, 2023
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