Skip to content

cmrtlib/linux/CMakelists.txt: respect MEDIA_BUILD_FATAL_WARNINGS#1203

Merged
intel-mediadev merged 1 commit into
intel:masterfrom
ffontaine:master
Aug 11, 2021
Merged

cmrtlib/linux/CMakelists.txt: respect MEDIA_BUILD_FATAL_WARNINGS#1203
intel-mediadev merged 1 commit into
intel:masterfrom
ffontaine:master

Conversation

@ffontaine
Copy link
Copy Markdown
Contributor

Respect MEDIA_BUILD_FATAL_WARNINGS to avoid the following build failure when the user provides _FORTIFY_SOURCE:

In file included from /home/buildroot/autobuild/instance-0/output-1/host/x86_64-buildroot-linux-gnu/sysroot/usr/include/dlfcn.h:22,
                 from /home/buildroot/autobuild/instance-0/output-1/build/intel-mediadriver-19.4.0r/cmrtlib/linux/../linux/share/cm_include.h:30,
                 from /home/buildroot/autobuild/instance-0/output-1/build/intel-mediadriver-19.4.0r/cmrtlib/agnostic/share/cm_printf_host.cpp:23:
/home/buildroot/autobuild/instance-0/output-1/host/x86_64-buildroot-linux-gnu/sysroot/usr/include/features.h:397:4: error: #warning _FORTIFY_SOURCE requires compiling with optimization (-O) [-Werror=cpp]
  397 | #  warning _FORTIFY_SOURCE requires compiling with optimization (-O)
      |    ^~~~~~~

Fixes:

Signed-off-by: Fabrice Fontaine fontaine.fabrice@gmail.com

Respect MEDIA_BUILD_FATAL_WARNINGS to avoid the following build failure
when the user provides _FORTIFY_SOURCE:

In file included from /home/buildroot/autobuild/instance-0/output-1/host/x86_64-buildroot-linux-gnu/sysroot/usr/include/dlfcn.h:22,
                 from /home/buildroot/autobuild/instance-0/output-1/build/intel-mediadriver-19.4.0r/cmrtlib/linux/../linux/share/cm_include.h:30,
                 from /home/buildroot/autobuild/instance-0/output-1/build/intel-mediadriver-19.4.0r/cmrtlib/agnostic/share/cm_printf_host.cpp:23:
/home/buildroot/autobuild/instance-0/output-1/host/x86_64-buildroot-linux-gnu/sysroot/usr/include/features.h:397:4: error: #warning _FORTIFY_SOURCE requires compiling with optimization (-O) [-Werror=cpp]
  397 | #  warning _FORTIFY_SOURCE requires compiling with optimization (-O)
      |    ^~~~~~~

Fixes:
 - http://autobuild.buildroot.org/results/52638d95312e464626d1c4047b3b26d4f57a1cd2

Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
@XinfengZhang XinfengZhang requested a review from HeFan2017 June 7, 2021 02:55
@XinfengZhang
Copy link
Copy Markdown
Contributor

from my understanding, we should resolve warning instead of remove the Werror option. could you help to explain why we could not fix the warning directly?
@HeFan2017 could you help to review it.


# Set up compile options that will be used for the Linux build
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CPP_STANDARD_OPTION} -fPIC -fpermissive -fstack-protector-all -Werror")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CPP_STANDARD_OPTION} -fPIC -fpermissive -fstack-protector-all")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why need to remove -Werror instead of fixing in the code.

@ffontaine
Copy link
Copy Markdown
Contributor Author

ffontaine commented Jun 9, 2021

Hardcoding -Werror is generally seen as a bad practice from a downstream user perspective. Indeed, this will break your build with old or new gcc. This will also break the build if the user provides some kind of options that were not foreseen (e.g. if the user passes -D_FORTIFY_SOURCE=1 in CFLAGS, you'll get a warning because some parts of intel-mediadriver hardcodes -D_FORTIFY_SOURCE=2. Due to -Werror, this warning will become an error). You can read more info about it here: https://embeddedartistry.com/blog/2017/05/22/werror-is-not-your-friend.

Copy link
Copy Markdown
Contributor

@dvrogozh dvrogozh left a comment

Choose a reason for hiding this comment

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

Warning should be fixed, no doubt.

However, I fully agree that having -Werror by default is a bad practice. Instead, in our CI we should add -Werror passing custom build flags, something like:

cmake -DCMAKE_C_FLAGS="-Werror" ...

This advise goes to majority of the build flags by the way. Imho, cmake setup should be minimal and if we have any additional requirements these should be handled in CI setup, not in the cmake setup.

As of now media-drviver does not follow that, but it has MEDIA_BUILD_FATAL_WARNINGS cmake variable to allow disabling of -Werror. By default, we force warnings as errors. That's good compromise. So, I believe this PR is a good one since it actually won't affect our CI builds, but allow community make experiments if needed (on their own risk). So, I approve the request.

@ffontaine
Copy link
Copy Markdown
Contributor Author

Thanks for your feedback, it should also be noted that we're carrying a patch in buildroot to drop hardening flags because some embedded toolchains don't support it: https://git.buildroot.net/buildroot/tree/package/intel-mediadriver/0001-Drop-hardening-related-flags.patch. It would be great if those kind of flags could also be disabled through a dedicated option even if stack-protection should be enabled by default for obvious security reasons.

@dvrogozh dvrogozh added the verifying PR: fix ready and verifying with build/test label Jul 20, 2021
@intel-mediadev intel-mediadev merged commit 6a52497 into intel:master Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

verifying PR: fix ready and verifying with build/test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants