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

mprintf: Ignore non-literal format string #8740

Closed
wants to merge 1 commit into from

Conversation

gjasny
Copy link
Contributor

@gjasny gjasny commented Apr 24, 2022

Using the non-literal format string here is intentional and the warning leads to noise in the best case or an error in the worst.

@bagder
Copy link
Member

@bagder bagder commented Apr 24, 2022

How do you to get this warning?

@bagder bagder added the tidy-up label Apr 24, 2022
@gjasny
Copy link
Contributor Author

@gjasny gjasny commented Apr 25, 2022

We have a custom CMake toolchain that sets -Wformat=2 in the CMAKE_C_FLAGS.

@danielgustafsson
Copy link
Member

@danielgustafsson danielgustafsson commented Apr 25, 2022

What if someone has a toolchain or process which relies on trapping that?

@gjasny
Copy link
Contributor Author

@gjasny gjasny commented Apr 25, 2022

AFAIU the compiler warning was added to not pass a user-controlled format string to printf like in this example:

void log(const char* msg) {
  printf(msg); // use printf("%s", msg) instead
}

As heuristic for non-user controlled strings the compiler looks for a string literal (which is always compile-time defined). But here in dprintf_formatf the format string is assembled on-the-fly but still not user-controlled. This renders the compiler warning into a false-positive because the format string is not user-controlled.

To suppress the warning we save all the enabled warnings, disable the format-nonliteral warning and after the function call restore all the warnings.

I cannot think of a case where one would like to see the warning anyways.

bagder
bagder approved these changes May 16, 2022
Copy link
Member

@bagder bagder left a comment

Seems small and innocuous enough as long as we don't have to do this dance on many places.

@bagder bagder closed this in 5367899 May 16, 2022
@bagder
Copy link
Member

@bagder bagder commented May 16, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants