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

unittests: enable ASAN on native #18099

Merged
merged 3 commits into from
May 13, 2022
Merged

Conversation

kaspar030
Copy link
Contributor

@kaspar030 kaspar030 commented May 13, 2022

Contribution description

This PR wires up the Address Sanitizer for the unittests on native.

Another PR commit fixes the last remaining issue ASAN reported (a scratch buffer was too small in tests-uri_parser).

Testing procedure

This only affects native, and makes it running unittests more likely to fail. CI tests all of it.

Issues/PRs references

#17800

@kaspar030 kaspar030 requested a review from miri64 as a code owner May 13, 2022 09:49
@github-actions github-actions bot added Area: boards Area: Board ports Area: tests Area: tests and testing framework Platform: native Platform: This PR/issue effects the native platform labels May 13, 2022
@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 13, 2022
@@ -48,8 +48,7 @@
} \
} while (0)

#define VEC_MSG_LEN (sizeof("Unexpected userinfo member \"\" for \"\"") + \
64U + 8U)
#define VEC_MSG_LEN (256)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was like ~110 before. tests are executed pretty low on the stack, so this should be ok.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you tell which test exactly tripped the asan? The original VEC_MSG_LEN might be based on a wrong assumption, which in testing is always a warning sign.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(so I'd rather fix the value for the correct assumption than just making it large enough, to account for exactly those kind of wrong assumptions)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah... Just saw, that this is just for the output messages of the assert... so nvm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jup, my debugging went the same way, but it was actually the first test (so first message) where it tripped. Then I tried to parse the macro for why this fails, but that was not very fruitful.

@kaspar030
Copy link
Contributor Author

(unrelated build failure in tests/gnrc_netif_ieee802154)

@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 13, 2022
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. ACK

@miri64 miri64 enabled auto-merge May 13, 2022 12:11
@miri64 miri64 merged commit 9a54e7e into RIOT-OS:master May 13, 2022
miri64 added a commit to miri64/RIOT that referenced this pull request May 23, 2022
@kaspar030 kaspar030 deleted the unittests_native_asan branch May 23, 2022 09:42
miri64 added a commit to miri64/RIOT that referenced this pull request May 23, 2022
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: native Platform: This PR/issue effects the native platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants