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 address sanitizer flags #1923

Merged
merged 8 commits into from
Sep 2, 2022
Merged

Conversation

englercj
Copy link
Contributor

@englercj englercj commented Aug 7, 2022

What does this PR do?

Resolves #1595

  1. Implement support for fsanitize=address in gcc, clang, msc, and Visual Studio backends.
  2. Implement support for fsanitize=fuzzer in msc, Visual Studio, and clang backends.

How does this PR change Premake's behavior?

Existing behavior should be unaffected.

Anything else we should know?

There are a number of other fsanitize options that are supported by gcc/clang, but not by msc or visual studio. I did not add support for these, but can if it is desired.

Did you check all the boxes?

  • Focus on a single fix or feature; remove any unrelated formatting or code changes
    • I did make a couple unnecessary format changes, but only in files I was already modifying and nothing major.
  • Add unit tests showing fix or feature works; all tests pass
  • Mention any related issues (put closes #XXXX in comment to auto-close issue when PR is merged)
  • Follow our coding conventions
  • Minimize the number of commits
  • Align documentation to your changes

You can now support Premake on our OpenCollective. Your contributions help us spend more time responding to requests like these!

Copy link
Member

@samsinsane samsinsane left a comment

Choose a reason for hiding this comment

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

We're in the process of deprecating flags and don't allow new values to be added to it, instead can you create a new API? Perhaps sanitize with the type list:string - basically the same as flags just for the sanitizer options.

@englercj
Copy link
Contributor Author

englercj commented Aug 8, 2022

Removed the flags and added a new sanitize API.

@englercj
Copy link
Contributor Author

Anything else need to be changed here?

Copy link
Member

@nickclark2016 nickclark2016 left a comment

Choose a reason for hiding this comment

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

In general, I'd say it looks good. As an aside, I see you added "address" sanitizer to GCC (and by proxy clang). What about fuzzer on Clang? It should be supported as well.

modules/vstudio/vs2010_vcxproj.lua Show resolved Hide resolved
@englercj
Copy link
Contributor Author

Requested changes have been made.

@nickclark2016
Copy link
Member

fuzzer = 2022
asan = 2019

Copy link
Member

@nickclark2016 nickclark2016 left a comment

Choose a reason for hiding this comment

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

LGTM. Leaving open for comments by @starkos or @samsinsane (or anyone else) for a few days, then I'll close. Thanks!

@nickclark2016 nickclark2016 merged commit fb89d40 into premake:master Sep 2, 2022
@englercj englercj deleted the asan-support branch September 3, 2022 15:15
@nickclark2016
Copy link
Member

Something I just noticed, for consistency with the API, I'll add a follow on to make the arguments lower case to be consistent with the rest of the API.

@englercj
Copy link
Contributor Author

englercj commented Sep 5, 2022

Ah, sorry. They were originally flags which use CamelCase so I had followed that pattern. Forgot to change casing when I switched to a new API.

@nickclark2016
Copy link
Member

No worries. I approved the merge, it's on me.

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.

Add sanitizers support
3 participants