| Message ID | 1478695312-5009-1-git-send-email-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Wed, 9 Nov 2016, Adhemerval Zanella wrote: > It is because sh4 kernel-features.sh is included multiple times > without guards and this patch fixes by adding them. Tested on a > sh4-linux-gnu build. > > Also with this issues, is there are strong reason to *not* have include guards > on kernel-features.h? With current approach, a architecture can't include > Linux default kernel-features.h and redefine it to a different value, only > undefine it (unless it explicit do not include default kernel-features.h, > and I think that's is not the idea). > > * sysdeps/unix/sysv/linux/sh/kernel-features.h: Add include > guards. OK. I think it's appropriate for all these headers to have include guards. The #ifndef, #undef etc. could be avoided (moving this header closer to glibc's macro-name-typo-proof norms in this regard) if the header were split up into smaller pieces for individual macros or groups of related macros whenever a macro's definition needs to vary between architectures. Thus, you'd have e.g. kernel-features-st-ino.h defining __ASSUME_ST_INO_64_BIT to either 0 or 1, the default version of that header would define to 0 and the architectures overriding it would define to 1 (and wouldn't need #include_next). kernel-features.h could end up as an architecture-independent header that just #includes the smaller architecture-specific headers. (This is not a full design, especially for the more complicated cases such as socket syscalls.)
On 10/11/2016 16:07, Joseph Myers wrote: > On Wed, 9 Nov 2016, Adhemerval Zanella wrote: > >> It is because sh4 kernel-features.sh is included multiple times >> without guards and this patch fixes by adding them. Tested on a >> sh4-linux-gnu build. >> >> Also with this issues, is there are strong reason to *not* have include guards >> on kernel-features.h? With current approach, a architecture can't include >> Linux default kernel-features.h and redefine it to a different value, only >> undefine it (unless it explicit do not include default kernel-features.h, >> and I think that's is not the idea). >> >> * sysdeps/unix/sysv/linux/sh/kernel-features.h: Add include >> guards. > > OK. I think it's appropriate for all these headers to have include > guards. Ok, I will push this patch. > > The #ifndef, #undef etc. could be avoided (moving this header closer to > glibc's macro-name-typo-proof norms in this regard) if the header were > split up into smaller pieces for individual macros or groups of related > macros whenever a macro's definition needs to vary between architectures. > Thus, you'd have e.g. kernel-features-st-ino.h defining > __ASSUME_ST_INO_64_BIT to either 0 or 1, the default version of that > header would define to 0 and the architectures overriding it would define > to 1 (and wouldn't need #include_next). kernel-features.h could end up as > an architecture-independent header that just #includes the smaller > architecture-specific headers. (This is not a full design, especially for > the more complicated cases such as socket syscalls.) This could be a feasible approach, although it would require some extensive refactoring. But I think we current approach, where architecture kernel-feature is used from sysdep and it is responsible to include default linux one and undef and redefine value, plus guards should be also feasible for macro-name-typo-proof as well.
diff --git a/sysdeps/unix/sysv/linux/sh/kernel-features.h b/sysdeps/unix/sysv/linux/sh/kernel-features.h index ea4fdbc..d03aafa 100644 --- a/sysdeps/unix/sysv/linux/sh/kernel-features.h +++ b/sysdeps/unix/sysv/linux/sh/kernel-features.h @@ -17,6 +17,9 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#ifndef __KERNEL_FEATURES_SH__ +# define __KERNEL_FEATURES_SH__ + /* SH uses socketcall. */ #define __ASSUME_SOCKETCALL 1 @@ -50,3 +53,5 @@ the kernel interface for p{read,write}64 adds a dummy long argument before the offset. */ #define __ASSUME_PRW_DUMMY_ARG 1 + +#endif
Since 327792c sh4 builds fails with: ../sysdeps/unix/sysv/linux/kernel-features.h:49:0: error: "__ASSUME_ST_INO_64_BIT" redefined [-Werror] #define __ASSUME_ST_INO_64_BIT 1 ^ In file included from ../sysdeps/unix/sysv/linux/sysdep.h:19:0, from ../sysdeps/unix/sysv/linux/sh/sysdep.h:24, from ../sysdeps/unix/sysv/linux/sh/sh4/sysdep.h:4, from <stdin>:1: ../sysdeps/unix/sysv/linux/sh/kernel-features.h:47:0: note: this is the location of the previous definition #define __ASSUME_ST_INO_64_BIT 0 It is because sh4 kernel-features.sh is included multiple times without guards and this patch fixes by adding them. Tested on a sh4-linux-gnu build. Also with this issues, is there are strong reason to *not* have include guards on kernel-features.h? With current approach, a architecture can't include Linux default kernel-features.h and redefine it to a different value, only undefine it (unless it explicit do not include default kernel-features.h, and I think that's is not the idea). * sysdeps/unix/sysv/linux/sh/kernel-features.h: Add include guards. --- ChangeLog | 5 +++++ sysdeps/unix/sysv/linux/sh/kernel-features.h | 5 +++++ 2 files changed, 10 insertions(+)