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

Test Unix x86 with asan/ubsan in CI #49

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

iluuu1994
Copy link
Contributor

No description provided.

Comment on lines 19 to 21
ifeq (debug, $(BUILD))
CFLAGS += -O0 -g -DIR_DEBUG=1
override CFLAGS += -O0 -g -DIR_DEBUG=1
endif
Copy link
Owner

Choose a reason for hiding this comment

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

I've never known about the "override" before.
I think, it may be better to use a different BUILD type. e.g. "asan" in addition to "release" and "debug".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://www.gnu.org/software/make/manual/html_node/Override-Directive.html

If a variable has been set with a command argument (see Overriding Variables), then ordinary assignments in the makefile are ignored. If you want to set the variable in the makefile even though it was set with a command argument, you can use an override directive, which is a line that looks like this:

override variable = value

I think override is a good flag either way, because otherwise users can still set CFLAGS via make CFLAGS=... but local CFLAGS in the Makefile are completely ignored. This can lead to unexpected behavior. I.e. in this case it will cancel the -Wall -Wextra -Wno-unused-parameter, and -O2 -g with the release build, etc. This is not immediately obvious when the removal of the flags doesn't break the build.

Copy link
Owner

Choose a reason for hiding this comment

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

OK

@dstogov dstogov merged commit 9a8cbdf into dstogov:master Sep 26, 2023
6 checks passed
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.

2 participants