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

Silence clang warning -Wtautological-pointer-compare #3005

Merged
merged 5 commits into from
Jul 23, 2019

Conversation

beutlich
Copy link
Member

See this CI run compared to the previous one.

Close #2109.

@beutlich beutlich added the L: C-Sources Issue addresses Modelica/Resources/C-Sources label Jun 25, 2019
@beutlich beutlich added this to the MSL4.0.0 milestone Jun 25, 2019
@beutlich beutlich self-assigned this Jun 25, 2019
Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

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

I'm investigating this would cause null-pointer warnings in our tool-chains - and if so if that can be avoided.

@HansOlsson
Copy link
Contributor

Investigating more I don't think that this is the correct solution, and would either just disable the check - or see if it can be weakened in some other way.

The reason is that as far as I understand:

  • These attributes/SAL only generate warnings, not hard errors, if used incorrectly. Thus removing the "tautological" check just removes defence in depth, and if someone calls with a null-pointer we have changed the code from doing nothing to crashing.
  • These attributes/SAL are optional - so we cannot be sure that users of these will even have warnings enabled.
  • Not all compilers support these attributes/SAL.

Note that Visual Studio C++ (2019 at least) has considered this - and I would say that most of these functions are part of the trust boundary:

"If a function appears at a trust boundary, we recommend that you use the Pre_defensive annotation. The "defensive" modifier modifies certain annotations to indicate that, at the point of call, the interface should be checked strictly, but in the implementation body it should assume that incorrect parameters might be passed. In that case, In Pre_defensive is preferred at a trust boundary to indicate that although a caller will get an error if it attempts to pass NULL, the function body will be analyzed as if the parameter might be NULL, and any attempts to de-reference the pointer without first checking it for NULL will be flagged."

@beutlich beutlich changed the title Remove clang warning -Wtautological-pointer-compare Silence clang warning -Wtautological-pointer-compare Jun 26, 2019
@beutlich
Copy link
Member Author

beutlich commented Jun 26, 2019

OK, I updated the code to only silence the clang warning while not changing any nullptr checks. See here for the CI run.

@beutlich beutlich requested a review from HansOlsson June 26, 2019 07:45
Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

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

Looks good now.

@HansOlsson
Copy link
Contributor

Hmm...

Actually it seems that disabling "-Wtautological-pointer-compare" only disables the warnings, but clang (and gcc) still optimizes away the null-pointer tests, which is kind of dangerous.
It is necessary to add "-fno-delete-null-pointer-checks" to get reliable results.

@beutlich
Copy link
Member Author

beutlich commented Jul 8, 2019

I added -fno-delete-null-pointer-checks here such that this optimization is disabled whenever new binaries are built.

Copy link
Member

@sjoelund sjoelund left a comment

Choose a reason for hiding this comment

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

I added a commit with the compiler flag added for the autotools project (only added if the flag exists for the compiler)

@beutlich beutlich merged commit 4451f22 into modelica:master Jul 23, 2019
@beutlich beutlich deleted the remove-clang-warning branch July 23, 2019 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: C-Sources Issue addresses Modelica/Resources/C-Sources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should external C functions check their external object pointer for non-null?
3 participants