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

meson: undefine NDEBUG in the tests #238

Closed
wants to merge 1 commit into from

Conversation

evelikov
Copy link
Collaborator

When using -D b_ndebug=true or inheriting CFLAGS="-DNDEBUG" from the environment, the asserts will be no-op thus the checks in the LD_PRELOADED libraries will be omitted.

Explicitly undefine the macro, so we don't accidentally shoot ourselves in the foot.


As an aside: our tests are fairly inconsistent, namely

  • explicit return/fail vs assert_return(.... EXIT_FAILURE)
  • close/free in error paths vs always leak
  • return 0 vs return EXIT_SUCCESS vs exit(EXIT_SUCCESS)

Any objections if we get them on the same page, any preferences on the individual sub-topics?

When using -D b_ndebug=true or inheriting CFLAGS="-DNDEBUG" from the
environment, the asserts will be no-op thus the checks in the
LD_PRELOADED libraries will be omitted.

Explicitly undefine the macro, so we don't accidentally shoot ourselves
in the foot.

Signed-off-by: Emil Velikov <[email protected]>
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

@stoeckmann
Copy link
Contributor

  • return 0 vs return EXIT_SUCCESS vs exit(EXIT_SUCCESS)

Definitely prefer return because calling exit would skip a stack canary check. This check should be called as often as possible.

lucasdemarchi pushed a commit that referenced this pull request Nov 12, 2024
When using -D b_ndebug=true or inheriting CFLAGS="-DNDEBUG" from the
environment, the asserts will be no-op thus the checks in the
LD_PRELOADED libraries will be omitted.

Explicitly undefine the macro, so we don't accidentally shoot ourselves
in the foot.

Signed-off-by: Emil Velikov <[email protected]>
Link: #238
Signed-off-by: Lucas De Marchi <[email protected]>
@lucasdemarchi
Copy link
Contributor

Applied, thanks

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.

3 participants