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

Add ASAN coverage for all STL tests #4052

Merged
merged 7 commits into from Sep 29, 2023
Merged

Conversation

CaseyCarter
Copy link
Member

@CaseyCarter CaseyCarter commented Sep 25, 2023

... which isn't enabled in our typical CI, but will be part of a separate CI pipeline run on a different schedule. Probably daily or weekly - it's not terribly expensive - but not on every push into every PR.

The changes are broken down into focused commits:

  1. Teach the test-case-gatherer to parse and filter by RunPL tags. Also change our single existing ctest test stl to only run configurations that do not have the ASAN tag. Add an additonal ctest test stlasan to run only configurations that do have the ASAN tag.
  2. Calling this commit "focused" is a stretch: it adds ASan configurations to every test matrix we use, which is a fairly big lift. Runall is weird: when combining two configs via RUNALL_CROSSLIST, the result has the union of all variables present in either input configuration, with the value of each variable being the concatenation of the values from the inputs, and the intersection of the tags on the inputs. The special tag * is the identity element for tag intersection. So in every sequence of crosslists:
    meow
    woof
    RUNALL_CROSSLIST
    cat
    dog
    RUNALL_CROSSLIST
            seven    # No tag - not an ASAN test
    ASAN    eleven # ASAN tag - is an ASAN test
    
    Every configuration line before the final "leaf" RUNALL_CROSSLIST needs to be tagged with * or there will be no ASAN configurations in the resulting cross-product.
  3. Update the libc++ expected_results.txt file. The :0 config is the same as before, but :1 is now the MSVC-ASAN configuration and :2 is clang-cl. (I think it's less confusing in the long term to change the clang-cl assignment than to have the two MSVC tags be non-consecutive.) This generally looks like "change :1 to :2, and then duplicate the :0 entries with :1", but there are some :0s with no corresponding :1 for failures due to /analyze bugs - we don't run /analyze together with /fsanitize=address.
  4. A specific change to the P0881R7_stacktrace test. The source_location values that this test observes are affected by the presence of /Zi on the command line; since /fsanitize=address is always paired with /Zi, the ASAN configuration(s) HAS_DEBUG_INFO even when it's not specified on the command line.
  5. Add a no-ASAN version of fast_matrix for three <regex> tests that leak memory when compiled with ASAN.
  6. Changes to the Azure Pipelines infrastructure. This adds a new azure-devops/asan-pipeline.yml for the new pipeline which has been running against my fork (https://dev.azure.com/vclibs/STL/_build?definitionId=5&_a=summary) during development, but will be retargeted after this PR merges. The new pipeline looks a lot like the old one, but with a different default value for the testSelection variable which is plumbed through into the run-tests.yml template to select which tests to run.

This is a dual of internal MSVC-PR-500092.

CaseyCarter and others added 6 commits September 25, 2023 10:01
Also teach our compiler option parser about `-fsanitize=address,undefined`
Tell the validator that `.lst` files may be tabby now.
The clang config is now `:2`, `:1` is cl with ASAN enabled.

Skip libc++ `operator new` tests that ASan doesn't like

Skip `basic_string::max_size` test that ASan doesn't like

Avoid VSO-1875597 in std/strings/string.conversions/stol.pass.cpp
... when building with ASan enabled.
In `Dev11_1158803_regex_thread_safety` and `Dev10_814245_regex_character_class_crash`, `regex_match` seems to leak memory with ASAN enabled; OOMs on x86 only.
* Add new `asan-pipeline.yml` entry point
* Properly plumb benchmarkBuildOutputLocationVar through `native-build-test.yml`
@CaseyCarter CaseyCarter added infrastructure Related to repository automation test Related to test code labels Sep 25, 2023
@CaseyCarter CaseyCarter requested a review from a team as a code owner September 25, 2023 23:09
@github-actions github-actions bot added this to Initial Review in Code Reviews Sep 25, 2023
@CaseyCarter CaseyCarter moved this from Initial Review to Final Review in Code Reviews Sep 25, 2023
@StephanTLavavej StephanTLavavej self-assigned this Sep 26, 2023
@StephanTLavavej StephanTLavavej added the ASan Address Sanitizer label Sep 26, 2023
azure-devops/asan-pipeline.yml Show resolved Hide resolved
azure-devops/asan-pipeline.yml Outdated Show resolved Hide resolved
azure-devops/asan-pipeline.yml Outdated Show resolved Hide resolved
azure-devops/asan-pipeline.yml Outdated Show resolved Hide resolved
azure-devops/asan-pipeline.yml Outdated Show resolved Hide resolved
tests/std/tests/fast_matrix.lst Outdated Show resolved Hide resolved
tests/utils/stl/test/tests.py Outdated Show resolved Hide resolved
tests/std/tests/P2693R1_text_formatting_stacktrace/env.lst Outdated Show resolved Hide resolved
tests/std/tests/concepts_latest_matrix.lst Outdated Show resolved Hide resolved
tests/std/tests/usual_matrix.lst Outdated Show resolved Hide resolved
Code Reviews automation moved this from Final Review to Work In Progress Sep 26, 2023
@CaseyCarter CaseyCarter moved this from Work In Progress to Final Review in Code Reviews Sep 28, 2023
@StephanTLavavej
Copy link
Member

There are a couple of places where you fused env.lst lines that had been written somewhat verbosely in an attempt to keep different files looking similar (the header units, modules, and floating-point matrices), but it's not worth resetting testing either, and arguably attempting to keep files looking similar without explicit comments was a lost cause anyways.

@StephanTLavavej StephanTLavavej removed their assignment Sep 28, 2023
@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Sep 28, 2023
@CaseyCarter CaseyCarter self-assigned this Sep 29, 2023
@CaseyCarter CaseyCarter merged commit 4751057 into microsoft:main Sep 29, 2023
35 checks passed
Code Reviews automation moved this from Ready To Merge to Done Sep 29, 2023
@CaseyCarter CaseyCarter deleted the asanpl branch September 29, 2023 16:45
CaseyCarter added a commit to CaseyCarter/STL that referenced this pull request Oct 3, 2023
... in our test matrices.

`RUNALL_CROSSLIST` forms the cross product of all lines from above with all lines from below, and
from each pairing whose tag sets intersect produces a new line tagged with the intersection that
defines all variables defined on either line, with values that are the concatenation of the values
from `x` and `y`. Non-tagged lines are treated as if they are all tagged with the same value, and
`*` is the identity element for tag set intersection. For example:

```
	MEOW=A
X	MEOW=B
Y	MEOW=C
*	MEOW=D
RUNALL_CROSSLIST
	WOOF=1
X,Y	WOOF=2
X	WOOF=3
Y	MEOW=E WOOF=4
```
is equivalent to:
```
	MEOW=A WOOF=1
X	MEOW=B WOOF=2
X	MEOW=B WOOF=3
Y	MEOW=C WOOF=2
Y	MEOW="C E" WOOF=4
	MEOW=D WOOF=1
X,Y	MEOW=D WOOF=2
X	MEOW=D WOOF=3
Y	MEOW="D E" WOOF=4
```

[microsoftGH-4052](microsoft#4052) added `*` tags to all non-leaf lines
so they would properly combine with leaf configurations that have either an `ASAN` or empty tag.
There's a danger of "losing" `ASAN` configurations if we add a crosslist that inadvertently matches
untagged lines with `ASAN` lines. We decided that all lines should be tagged with either `ASAN` or
`*` to lessen the likelihood of such a mistake.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASan Address Sanitizer infrastructure Related to repository automation test Related to test code
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants