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

CI: Adding CTest memcheck to CodeBuild #4776

Merged
merged 33 commits into from
Oct 2, 2024
Merged

Conversation

boquan-fang
Copy link
Contributor

@boquan-fang boquan-fang commented Sep 17, 2024

Resolved issues:

Create another buildspec file for CTest memcheck. Following previous tickets instructions #3758 and PR#4369.

Description of changes:

  • Add Valgrind check options for both regular and pedantic valgrind checks into the code base. The CTest memcheck should only triggers pedantic valgrind check if the libcrypto in used is openssl-1-1-1.
  • Create a new codebuild spec file buildspec_valgrind.yml.
  • Append some new valgrind suppressions to handle platform updates from Ubuntu18 to Ubuntu22.

Call-outs:

  • Clang compiler is not included in this PR, because Ubuntu 18 and 22's Valgrind can't check clang-compiled executable. Update Valgrind version in CI docker image #4759
    • Need Valgrind version 20 or more to test Clang compiled executable.
  • “cannot find memory tester output file” message are a result of logging valgrind errors through the --log-fd=2 option. I could not find another approach. Please see this comment for my reasoning.
  • This is an example of how failed CTest memcheck codebuild looks like.
  • For the scope of this PR, pedantic Valgrind check will still run on Ubuntu 18.
  • backtrace function leaks a static amount of memory when it is first loaded, so it is safe to suppress. Read this explanation for further details.

Testing:

  • Create a new CodeBuild job called Valgrind and use this PR as source. The CodeBuild job is successfully.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Boquan Fang added 7 commits September 6, 2024 19:28
* create a buildspec file which contains ctest memcheck command
* append options for both regular and pedantic valgrind check above ASAN
  check
* set up valgrind options
* set up valgrind ci codebuild check
@github-actions github-actions bot added the s2n-core team label Sep 17, 2024
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
codebuild/spec/buildspec_valgrind.yml Outdated Show resolved Hide resolved
tests/unit/valgrind.suppressions Outdated Show resolved Hide resolved
tests/unit/valgrind.suppressions Outdated Show resolved Hide resolved
@lrstewart lrstewart self-requested a review September 17, 2024 17:25
* add comments to `CMakeLists.txt` to explain pedantic valgrind check
  logic
* change variable names to be concise
* use `VALGRIND_DEFAULT` to define `VALGRIND_PEDANTIC`
* reformat the CTest memecheck command to make it more readable
* add explanations and necessary comments to `valgrind.suppressions`
@boquan-fang boquan-fang marked this pull request as ready for review September 17, 2024 17:34
@boquan-fang boquan-fang requested a review from dougch as a code owner September 17, 2024 17:34
boquan-fang and others added 7 commits September 17, 2024 10:53
* reformat `valgrind.suppressions` comments to make them more concise
* move set Valgrind variable up. The next PR will change the location of
  set MEMORYCHECK_COMMAND_OPTIONS
* paste tracking issue's link to the comment
* relocate `include(CTest)` location to above unit testing sections
* explicitly enable `BUILD_TESTING` options in `CMakeLists.txt`
* relocate `MEMORYCHECK_COMMAND_OPTIONS` to the testing sections above
  ASAN
* remove the comments above `include(CTest)`
* update the message to set `BUILD_TESTING=ON`
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
tests/unit/valgrind.suppressions Outdated Show resolved Hide resolved
tests/unit/valgrind.suppressions Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Boquan Fang and others added 3 commits September 18, 2024 20:33
* refactor some comments in `CMakeLists.txt`
* change suppressions names for valgrind and delete some comments
* relocate `include(CTest)` to the end of unit tests set up
Boquan Fang added 3 commits September 18, 2024 22:13
* refactor formats
* add one wildcard to suppress backtrace memory defects for ubuntu22 and
  24 platforms
@boquan-fang
Copy link
Contributor Author

boquan-fang commented Sep 28, 2024

Explanation for "cannot find memory tester output file:" in CTest memcheck:

Problem Statement

CTest memcheck by default hide memory test output to a MemoryTester log file. I need those errors to be printed on terminal.

Study the source code:

CTest memcheck by default add --log-file options to memory check.

Ways that I tried to display memory errors in terminal

  1. Add --log-fd=2 to Valgrind option. This option can overwrite the default setting and print error messages in stderr. However, the source code has a function to print out "cannot find memory tester output file" message, in case the output is not redirected to the output file.
  2. Use CTest --overwrite option. I tried to overwrite MEMORY_COMMAND_OPTIONS. However, that variable doesn't affect the default --log-file set up.
  3. Use CTest -O option. However, this option only duplicate the terminal output to another file, instead of overwrite the --log-file flag.
  4. Write a script to display memory errors in the end. This option will print all error messages in the end, and don't incur the tester file message. However, it's better to display memory error messages directly below failed test cases, so that users can keep track of which error messages is corresponding to which tests.

CMakeLists.txt Show resolved Hide resolved
codebuild/spec/buildspec_valgrind.yml Show resolved Hide resolved
tests/unit/valgrind.suppressions Outdated Show resolved Hide resolved
Boquan Fang added 2 commits October 1, 2024 22:17
* modify valgrind suppression comments for backtrace.
* make suppression comments more precise
@boquan-fang boquan-fang requested a review from lrstewart October 1, 2024 22:39
@boquan-fang boquan-fang requested a review from lrstewart October 1, 2024 23:24
@boquan-fang boquan-fang enabled auto-merge (squash) October 2, 2024 00:59
@boquan-fang boquan-fang merged commit e5ef845 into aws:main Oct 2, 2024
37 checks passed
boquan-fang added a commit to boquan-fang/s2n-tls that referenced this pull request Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants