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

Moved assert macro into its assert header #627

Merged
merged 1 commit into from
Jun 12, 2021
Merged

Moved assert macro into its assert header #627

merged 1 commit into from
Jun 12, 2021

Conversation

zvonimir
Copy link
Member

This is a more meaningful and consistent location
for it.

This is a more meaningful and consistent location
for it.
@shaobo-he
Copy link
Contributor

LGTM.

@shaobo-he
Copy link
Contributor

I have a minor concern: it appears assert.h is coupled with smack.h. This could be a problem when our custom front-ends defined in frontend.py are not used. For example, when a program with assertions is compiled using a makefile and its bc file is intended to be obtained using wllvm. Compilation ignores SMACK's assert.h unless we modify its CFLAGS to include SMACK's header path. But when do it, linking fails because __VERIFIER_assert is not defined.

@zvonimir
Copy link
Member Author

Good point. But how do we deal with this problem when it comes to all other headers we have - like stdlib, stdio, etc.?

@shaobo-he
Copy link
Contributor

Good point. But how do we deal with this problem when it comes to all other headers we have - like stdlib, stdio, etc.?

I think assert.h is unique because assert is a macro while other libraries provide functions. So normally linkers will easily find them.

@shaobo-he
Copy link
Contributor

Another issue with the current implementation manifests in a not-so-realistic case where the users can opt-out assertions in certain files by enabling NDEBUG. SMACK will still check these assertions.

@zvonimir
Copy link
Member Author

We can easily add NDEBUG into assert.h, that is not a problem.
Related to your earlier comment, if a user does not point to SMACK's assert.h, then the bc file we end up containing (most likely) calls to __assert_fail, which we are planning on implementing too.

Btw, I am not set on this solution, and I am open to other proposals. This solution made sense to me since it is consistent with how we deal with other standard C headers.

@zvonimir zvonimir merged commit 030e7af into develop Jun 12, 2021
@zvonimir zvonimir deleted the fix-assert branch June 12, 2021 00:09
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.

None yet

2 participants