-
Notifications
You must be signed in to change notification settings - Fork 111
[CLIENT-3915] Added suppressions for valgrind error noise #871
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
base: dev
Are you sure you want to change the base?
Changes from all commits
3bb2de2
0c7c7e8
586995f
9dfb2bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,63 @@ | ||||||
| # Generating valgrind suppressions | ||||||
|
|
||||||
| ## 1. Run the valgrind action with the following arguments | ||||||
|
|
||||||
| ``` | ||||||
| --error-limit=no --leak-check=full --gen-suppressions=all --num-callers=350 | ||||||
|
|
||||||
| ``` | ||||||
|
|
||||||
| --error-limit=no: Disable the limit of the numbers errors that can be reported | ||||||
|
|
||||||
| --leak-check=full: Show details for each individual memory error | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| --gen-suppressions=all: generate suppressions blocks alongside all memory errors | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| --num-callers=350: include 350 stack frames of context for each error and suppression. | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| **Note:** Additional stack frames decrease the likelihood of over suppression by increasing the specificity of each suppression. | ||||||
| However, more stack frames also reduce the speed of the test runs. | ||||||
| Suppressions may not exceed 500 stack frames or else the valgrind will fail with the following error: | ||||||
|
|
||||||
| ``` | ||||||
| too many callers in stack trace | ||||||
| ``` | ||||||
|
|
||||||
| When --num-callers=500, suppressions may end up having 600+ frames due to inline frames not counting towards the total number of callers. | ||||||
|
|
||||||
| Setting --num-callers=350 forces the total number of suppressions frames to be below 500 and avoids this error | ||||||
|
|
||||||
| ## 2. Download the results of the valgrind run step | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
? |
||||||
|
|
||||||
| Valgrind action runs available [here]https://github.com/aerospike/aerospike-client-python/actions/workflows/valgrind.yml | ||||||
|
|
||||||
| ## 3. Extract suppressions from logs | ||||||
|
|
||||||
| Run `extract_suppressions.py` from the `valgrind-suppressions` directory to extract any unique expressions from the valgrind logs. | ||||||
|
|
||||||
| Script usage: | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| ``` | ||||||
| python3 extract_suppressions.py log_input.txt new_suppressions.supp | ||||||
| ``` | ||||||
|
|
||||||
| For the full-suite, name the output file new_tests.supp | ||||||
|
|
||||||
| For a single file, name the output file after the name of the test (test_log.py.supp) | ||||||
|
|
||||||
| ## 4. Add suppression file to valgrind argument | ||||||
|
|
||||||
| Add the following argument to the valgrind arguments: | ||||||
|
|
||||||
| ``` | ||||||
| --suppressions=./valgrind-suppressions/new_suppressions.supp | ||||||
| ``` | ||||||
|
|
||||||
| **Note:** If the suppressions are not able to be parsed when running valgrind, look for timestamps in the suppression file. | ||||||
| The script should strip them out, but if the logs are corrupted, they might not be stripped out correctly | ||||||
|
|
||||||
| ## 5. Add any additional suppressions from additional valgrind runs | ||||||
|
|
||||||
| While the suppressions generated should cover most memory issues, some errors show up intermittendly. | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| When new false positive error cases pop up in valgrind logs, simply add the newly generated suppressions to the suppression file. | ||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,64 @@ | ||||||
| import argparse | ||||||
| import re | ||||||
|
|
||||||
| parser = argparse.ArgumentParser() | ||||||
| parser.add_argument("input_file", help="path to input file") | ||||||
| parser.add_argument("output_file", help="path to output file") | ||||||
| args = parser.parse_args() | ||||||
|
|
||||||
| input_file = args.input_file | ||||||
| output_file = args.output_file | ||||||
|
|
||||||
| unique_blocks = set() | ||||||
| current_block = [] | ||||||
| inside_block = False | ||||||
| total_blocks = 0 | ||||||
|
|
||||||
| # Matches the time stamp | ||||||
| ts_re = re.compile(r"^\d{4}-\d{2}-\d{2}T\S+\s+(.*)$") | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Might be better to rename |
||||||
|
|
||||||
| with open(input_file) as f: | ||||||
| for raw_line in f: | ||||||
| line = raw_line.rstrip() | ||||||
|
|
||||||
| # Strip timestamp prefix if present | ||||||
| m = ts_re.match(line) | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Rename |
||||||
| if m: | ||||||
| line = m.group(1) | ||||||
|
|
||||||
| if line.startswith("{"): | ||||||
| inside_block = True | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| current_block = [line] | ||||||
| elif line.startswith("}"): | ||||||
| current_block.append(line) | ||||||
| inside_block = False | ||||||
| total_blocks += 1 | ||||||
|
|
||||||
| # Only keep if 2nd line matches required suppression name | ||||||
| REQUIRED = "<insert_a_suppression_name_here>" | ||||||
|
|
||||||
| if len(current_block) > 1 and current_block[1].strip() != REQUIRED: | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we pass in a suppressions file when running valgrind, and the suppressions file contains a block with a named suppression, in the logs does valgrind print a suppressions block with that name? I'm not sure when this condition would be true |
||||||
| continue # skip this block | ||||||
|
|
||||||
| block_str = "\n".join(l.rstrip() for l in current_block) | ||||||
| unique_blocks.add(block_str) | ||||||
|
|
||||||
| elif inside_block: | ||||||
| current_block.append(line) | ||||||
|
|
||||||
| # Write unique blocks with tabs for inner lines only | ||||||
| with open(output_file, "w") as f: | ||||||
| for block in sorted(unique_blocks): | ||||||
| lines = block.split("\n") | ||||||
| # Keep first line `{` as is, indent all lines in between, last line `}` as is | ||||||
| if len(lines) > 2: | ||||||
| indented_block = "\n".join( | ||||||
| [lines[0]] + ["\t" + line for line in lines[1:-1]] + [lines[-1]] | ||||||
| ) | ||||||
| else: | ||||||
| # Blocks with only {} or {} plus one line | ||||||
| indented_block = "\n".join(lines) | ||||||
| f.write(indented_block + "\n\n") | ||||||
|
|
||||||
| print(f"Original number of suppressions: {total_blocks}") | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| print(f"Number of unique suppressions: {len(unique_blocks)}") | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.