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.

Sign up for GitHub

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

Jump to bottom

cmd: add support for building with address and undefined behavior sanitizers #15059

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

cmd: add support for building with address and undefined behavior sanitizers #15059

wants to merge 6 commits into from
+66 −15

Conversation

bboozzoo
Copy link
Contributor

@bboozzoo bboozzoo commented Feb 11, 2025 •
edited
Loading

The branch builds on #15045 and adds support for building with asan and ubsan. This is only useful for testing local installation or running unit tests, as otherwise we'd have to ship libasan/libubsan in the snapd snap to make it universally useful in all scenarios.

Note, some binaries which are linked statically, snap-gdb*-shim specifically, had to be skipped, since libasan relies on dlopen().

@bboozzoo bboozzoo added the Skip spread Indicate that spread job should not run label Feb 11, 2025
@bboozzoo bboozzoo closed this Feb 11, 2025
@bboozzoo bboozzoo reopened this Feb 11, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 11, 2025 •
edited
Loading

Fri Feb 14 07:13:02 UTC 2025

Spread tests skipped

@codecov Codecov
Copy link

codecov bot commented Feb 11, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (master@1a8abe6). Learn more about missing BASE report.
Report is 213 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff            @@
##             master   #15059   +/-   ##
=========================================
  Coverage          ?   78.06%           
=========================================
  Files             ?     1181           
  Lines             ?   157718           
  Branches          ?        0           
=========================================
  Hits              ?   123130           
  Misses            ?    26942           
  Partials          ?     7646
Flag Coverage Δ
unittests 78.06% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bboozzoo bboozzoo marked this pull request as ready for review February 11, 2025 18:21
@bboozzoo bboozzoo removed the Skip spread Indicate that spread job should not run label Feb 11, 2025
@bboozzoo bboozzoo closed this Feb 11, 2025
@bboozzoo bboozzoo reopened this Feb 11, 2025
alexmurray
alexmurray reviewed Feb 11, 2025
View reviewed changes
cmd/configure.ac
@@ -319,6 +319,20 @@ AS_IF([test "x$with_unit_tests" = "xyes"], [
AX_APPEND_COMPILE_FLAGS([-Werror], [CHECK_CFLAGS])
])

AC_ARG_ENABLE([sanitize],
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is only using asan at the moment, would it be better to just call it asan rather than sanitize?

alexmurray
alexmurray approved these changes Feb 11, 2025
View reviewed changes
Copy link
Contributor

@alexmurray alexmurray left a comment

Choose a reason for hiding this comment

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

LGTM but I think perhaps a better option name should be chosen that is more self-explanatory than just sanitize (plus I expect woke etc will complain about the use of the term sanitize as well)

@bboozzoo
Copy link
Contributor Author

LGTM but I think perhaps a better option name should be chosen that is more self-explanatory than just sanitize (plus I expect woke etc will complain about the use of the term sanitize as well)

Do you think it makes sense to add ubsan too, even if for unit tests only?

@alexmurray
Copy link
Contributor

Do you think it makes sense to add ubsan too, even if for unit tests only?

Sure, the more the merrier particularly if its just the unit tests.

@bboozzoo
github: download-install-debian-deps: install clang
b8282e5 
Signed-off-by: Maciej Borzecki <[email protected]>
@bboozzoo
cmd: add support for building C code with address sanitizer
791e8c2 
Add support for building C code with address
sanitizer (-fsanitize=address).

Signed-off-by: Maciej Borzecki <[email protected]>
@bboozzoo
github: unit-tests: add a step to build with clang and asan
15e76f6 
Signed-off-by: Maciej Borzecki <[email protected]>
@bboozzoo
cmd: make: enable ubsan when building with sanitizers
893bed0 
Signed-off-by: Maciej Borzecki <[email protected]>
@bboozzoo
github: unit-test: tweak step name
98405bb 
Signed-off-by: Maciej Borzecki <[email protected]>
@bboozzoo
woke: ignore cmd/configure.ac cmd/Makefile.am
12c286f 
Signed-off-by: Maciej Borzecki <[email protected]>
@bboozzoo bboozzoo changed the title cmd: add support for building with address sanitizer cmd: add support for building with address and undefined behavior sanitizers Feb 14, 2025
@bboozzoo bboozzoo added the Skip spread Indicate that spread job should not run label Feb 14, 2025
@bboozzoo bboozzoo closed this Feb 14, 2025
@bboozzoo bboozzoo reopened this Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexmurray alexmurray alexmurray approved these changes

@zyga zyga Awaiting requested review from zyga

@valentindavid valentindavid Awaiting requested review from valentindavid

@ernestl ernestl Awaiting requested review from ernestl

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
Skip spread Indicate that spread job should not run
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bboozzoo @alexmurray