Page MenuHomePhabricator

[sanitizer] Remove #include <linux/fs.h> to resolve fsconfig_command/mount_attr conflict with glibc 2.36
ClosedPublic

Authored by MaskRay on Jul 11 2022, 1:10 AM.

Details

Summary

It is generally not a good idea to mix usage of glibc headers and Linux UAPI
headers (https://sourceware.org/glibc/wiki/Synchronizing_Headers). In glibc
since 7eae6a91e9b1670330c9f15730082c91c0b1d570 (milestone: 2.36), sys/mount.h
defines fsconfig_command which conflicts with linux/mount.h:

.../usr/include/linux/mount.h:95:6: error: redeclaration of ‘enum fsconfig_command’

Remove #include <linux/fs.h> which pulls in linux/mount.h. Expand its 4 macros manually.

Fix https://github.com/llvm/llvm-project/issues/56421

Diff Detail

Event Timeline

MaskRay created this revision.Jul 11 2022, 1:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2022, 1:10 AM
MaskRay requested review of this revision.Jul 11 2022, 1:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2022, 1:10 AM
marxin added a subscriber: marxin.Jul 11 2022, 2:00 AM

LGTM, the kernel exported value is same for all architectures. Anther possibility would be to split the sanitizer_platform_limits_posix.cpp and add a sanitizer_platform_limits_posix_linux.cpp which only add linux includes.

zatrazz accepted this revision.Jul 11 2022, 4:48 AM
This revision is now accepted and ready to land.Jul 11 2022, 4:48 AM
vitalybuka accepted this revision.Jul 11 2022, 11:24 AM

LGTM, the kernel exported value is same for all architectures. Anther possibility would be to split the sanitizer_platform_limits_posix.cpp and add a sanitizer_platform_limits_posix_linux.cpp which only add linux includes.

If we are going to do this, sanitizer_platform_limits_linux.cpp may be the choice.

Do you think other #include <linux/*.h> in this file may cause a potential conflict in the future?

This revision was landed with ongoing or failed builds.Jul 11 2022, 11:38 AM
This revision was automatically updated to reflect the committed changes.

LGTM, the kernel exported value is same for all architectures. Anther possibility would be to split the sanitizer_platform_limits_posix.cpp and add a sanitizer_platform_limits_posix_linux.cpp which only add linux includes.

If we are going to do this, sanitizer_platform_limits_linux.cpp may be the choice.

Do you think other #include <linux/*.h> in this file may cause a potential conflict in the future?

I am not sure, it has bitten us in the past which required either glibc to fixup by including the kernel header
or the kernel to fixup some internal definitions. I personally prefer to keep glibc header not dependent of the
kernel ones, even at cost of extra maintainability (however I think this stance is not shared with
other glibc maintainers).

I think a potential sanitizer_platform_limits_linux.cpp may be a better approach to disantagle the kernel
include from libc one.

hctim added a subscriber: hctim.Jul 11 2022, 12:36 PM

Looks like this has broken the sanitizer-android bot build, PTAL: https://lab.llvm.org/buildbot/#/builders/77/builds/19507

LGTM, the kernel exported value is same for all architectures. Anther possibility would be to split the sanitizer_platform_limits_posix.cpp and add a sanitizer_platform_limits_posix_linux.cpp which only add linux includes.

If we are going to do this, sanitizer_platform_limits_linux.cpp may be the choice.

Do you think other #include <linux/*.h> in this file may cause a potential conflict in the future?

I am not sure, it has bitten us in the past which required either glibc to fixup by including the kernel header
or the kernel to fixup some internal definitions. I personally prefer to keep glibc header not dependent of the
kernel ones, even at cost of extra maintainability (however I think this stance is not shared with
other glibc maintainers).

musl's stance: it supports the inclusion of the kernel headers after the libc ones
https://git.musl-libc.org/cgit/musl/commit/?id=04983f2272382af92eb8f8838964ff944fbb8258

I prefer that glibc's header not dependent of the kernel ones, too, to support building using older Linux uapi.
And inclusion of libc headers after uapi headers is unsupported.

For this sys/mount.h issue, the commit should be easily cherry picked into older compiler-rt (<15.0.0) to help distributions.

I think a potential sanitizer_platform_limits_linux.cpp may be a better approach to disantagle the kernel
include from libc one.

LG. I can investigate how to place all these linux/*.h headers elsewhere:

#include <linux/if_eql.h>
#include <linux/if_plip.h>
#include <linux/lp.h>
#include <linux/mroute.h>
#include <linux/mroute6.h>
#include <linux/scc.h>
#include <linux/serial.h>

Looks like this has broken the sanitizer-android bot build, PTAL: https://lab.llvm.org/buildbot/#/builders/77/builds/19507

Testing and will revert this and push an amended one in one minute.

MaskRay added a comment.EditedJul 11 2022, 12:52 PM

Looks like this has broken the sanitizer-android bot build, PTAL: https://lab.llvm.org/buildbot/#/builders/77/builds/19507

Testing and will revert this and push an amended one in one minute.

By the way, do you have instructions testing an Android build without an Android device?
For now, I am going to use

+#if SANITIZER_ANDROID
 #include <linux/fs.h>
+#endif

I plan to do some #include refactoring and having a pre-commit test method will be useful...

hctim added a comment.Jul 11 2022, 1:01 PM

Looks like this has broken the sanitizer-android bot build, PTAL: https://lab.llvm.org/buildbot/#/builders/77/builds/19507

Testing and will revert this and push an amended one in one minute.

By the way, do you have instructions testing an Android build without an Android device?
For now, I am going to use

+#if SANITIZER_ANDROID
 #include <linux/fs.h>
+#endif

I plan to do some #include refactoring and having a pre-commit test method will be useful...

Just the build? https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild should help out, use buildbot_android.sh, and you can comment out line 46 (test_android ...).