Page MenuHomePhabricator

[Sanitizer][MIPS] Fix stat struct size for the O32 ABI
ClosedPublic

Authored by dmilosevic141 on Jul 14 2022, 4:01 AM.

Details

Reviewers
djtodoro
Summary

This patch adds the correct value for the struct stat's size (144), for the O32 MIPS ABI.
Until now, the value 160 was used (correct for the N32 MIPS ABI), which resulted in build failures.

Diff Detail

Event Timeline

dmilosevic141 created this revision.Jul 14 2022, 4:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2022, 4:01 AM
dmilosevic141 requested review of this revision.Jul 14 2022, 4:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2022, 4:01 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
djtodoro accepted this revision.Wed, Aug 31, 1:28 AM
This revision is now accepted and ready to land.Wed, Aug 31, 1:28 AM
djtodoro closed this revision.Wed, Aug 31, 1:28 AM

I think this patch has created regression when using largefile support. So try adding -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 to compiler cmdline on mips (o32 ABI) and the size of this struct is bumped to 160 on linux/glibc see definition of stat for mips in glibc

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/mips/bits/struct_stat.h;h=71594e456244d1e4a746c859048793385c5be376;hb=HEAD#l28

I think this patch has created regression when using largefile support. So try adding -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 to compiler cmdline on mips (o32 ABI) and the size of this struct is bumped to 160 on linux/glibc see definition of stat for mips in glibc

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/mips/bits/struct_stat.h;h=71594e456244d1e4a746c859048793385c5be376;hb=HEAD#l28

Hmm, so should we force -D_FILE_OFFSET_BITS=64 in the C compiler (clang and gcc) when -fsanitize=address is used? AFAIK There is nothing we can do to support both cases (with or w/o -D_FILE_OFFSET_BITS=64) in one libsanitizer build.

I think this patch has created regression when using largefile support. So try adding -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 to compiler cmdline on mips (o32 ABI) and the size of this struct is bumped to 160 on linux/glibc see definition of stat for mips in glibc

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/mips/bits/struct_stat.h;h=71594e456244d1e4a746c859048793385c5be376;hb=HEAD#l28

Hmm, so should we force -D_FILE_OFFSET_BITS=64 in the C compiler (clang and gcc) when -fsanitize=address is used? AFAIK There is nothing we can do to support both cases (with or w/o -D_FILE_OFFSET_BITS=64) in one libsanitizer build.

I think it could be turned into a CMake option or somesuch. However, usecase I am dealing with is using largefile support by default and its perhaps more common than the without largerfile settings.

I think this patch has created regression when using largefile support. So try adding -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 to compiler cmdline on mips (o32 ABI) and the size of this struct is bumped to 160 on linux/glibc see definition of stat for mips in glibc

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/mips/bits/struct_stat.h;h=71594e456244d1e4a746c859048793385c5be376;hb=HEAD#l28

Thanks for pointing this out @raj.khem.

As far as I can see, -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 is expected by default for the O32 ABI in LLVM - see https://github.com/llvm/llvm-project/blob/main/compiler-rt/cmake/base-config-ix.cmake#L224. GCC, however, doesn't follow this - please see the following discussion https://gcc.gnu.org/pipermail/gcc-patches/2022-July/597617.html.

Rather than working with CMake, shouldn't we be able to just add a check if _LARGEFILE_SOURCE is defined and if _FILE_OFFSET_BITS is defined and equal to 64? If it is so, struct_kernel_stat_sz would be equal to 160, rather than 144 (for the O32 ABI). This change would then be cherry-picked into GCC. Thus we would cover every possible case for both LLVM and GCC.
That is, unless, LLVM allows only -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 builds for the O32 ABI, and GCC allows only non--D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 builds for the O32 ABI (I'm not 100% sure if this is true), in which case these checks wouldn't be necessary, as we, for sure, know the correct values (160 for LLVM, 144 for GCC). They wouldn't be harmful in any way, however, and would make the source codes in LLVM and GCC not diverge. Also, if non--D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 builds for the O32 ABI were to be enabled in LLVM (or -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 builds for the O32 ABI were to be enabled in GCC) in the future, it would be covered. Thoughts?

I think this patch has created regression when using largefile support. So try adding -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 to compiler cmdline on mips (o32 ABI) and the size of this struct is bumped to 160 on linux/glibc see definition of stat for mips in glibc

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/mips/bits/struct_stat.h;h=71594e456244d1e4a746c859048793385c5be376;hb=HEAD#l28

Thanks for pointing this out @raj.khem.

As far as I can see, -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 is expected by default for the O32 ABI in LLVM - see https://github.com/llvm/llvm-project/blob/main/compiler-rt/cmake/base-config-ix.cmake#L224. GCC, however, doesn't follow this - please see the following discussion https://gcc.gnu.org/pipermail/gcc-patches/2022-July/597617.html.

Rather than working with CMake, shouldn't we be able to just add a check if _LARGEFILE_SOURCE is defined and if _FILE_OFFSET_BITS is defined and equal to 64? If it is so, struct_kernel_stat_sz would be equal to 160, rather than 144 (for the O32 ABI). This change would then be cherry-picked into GCC. Thus we would cover every possible case for both LLVM and GCC.

The problem is we need to ensure libsanitizer functional properly, not just papering over the issue and make it build. That's the point of why we define our own value of struct_kernel_stat_sz and check it with COMPILER_CHECK, instead of just using sizeof(struct stat).

I think this patch has created regression when using largefile support. So try adding -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 to compiler cmdline on mips (o32 ABI) and the size of this struct is bumped to 160 on linux/glibc see definition of stat for mips in glibc

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/mips/bits/struct_stat.h;h=71594e456244d1e4a746c859048793385c5be376;hb=HEAD#l28

Thanks for pointing this out @raj.khem.

As far as I can see, -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 is expected by default for the O32 ABI in LLVM - see https://github.com/llvm/llvm-project/blob/main/compiler-rt/cmake/base-config-ix.cmake#L224. GCC, however, doesn't follow this - please see the following discussion https://gcc.gnu.org/pipermail/gcc-patches/2022-July/597617.html.

Rather than working with CMake, shouldn't we be able to just add a check if _LARGEFILE_SOURCE is defined and if _FILE_OFFSET_BITS is defined and equal to 64? If it is so, struct_kernel_stat_sz would be equal to 160, rather than 144 (for the O32 ABI). This change would then be cherry-picked into GCC. Thus we would cover every possible case for both LLVM and GCC.

The problem is we need to ensure libsanitizer functional properly, not just papering over the issue and make it build. That's the point of why we define our own value of struct_kernel_stat_sz and check it with COMPILER_CHECK, instead of just using sizeof(struct stat).

I'm sorry, I must've misunderstood what the actual issue is then, would you mind elaborating in more detail? Is it mismatching the presence of -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 within the build process of compiler-rt / libsanitizer and within the actual invocation command of the built compiler-rt / libsanitizer through Clang / GCC?

I think this patch has created regression when using largefile support. So try adding -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 to compiler cmdline on mips (o32 ABI) and the size of this struct is bumped to 160 on linux/glibc see definition of stat for mips in glibc

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/mips/bits/struct_stat.h;h=71594e456244d1e4a746c859048793385c5be376;hb=HEAD#l28

Thanks for pointing this out @raj.khem.

As far as I can see, -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 is expected by default for the O32 ABI in LLVM - see https://github.com/llvm/llvm-project/blob/main/compiler-rt/cmake/base-config-ix.cmake#L224. GCC, however, doesn't follow this - please see the following discussion https://gcc.gnu.org/pipermail/gcc-patches/2022-July/597617.html.

Rather than working with CMake, shouldn't we be able to just add a check if _LARGEFILE_SOURCE is defined and if _FILE_OFFSET_BITS is defined and equal to 64? If it is so, struct_kernel_stat_sz would be equal to 160, rather than 144 (for the O32 ABI). This change would then be cherry-picked into GCC. Thus we would cover every possible case for both LLVM and GCC.

The problem is we need to ensure libsanitizer functional properly, not just papering over the issue and make it build. That's the point of why we define our own value of struct_kernel_stat_sz and check it with COMPILER_CHECK, instead of just using sizeof(struct stat).

yes, so it seems we do not support the default builds with -D_LARGEFILE_SOURCE if we do not support` largefile` then we should add some informative diagnostics during build/configure and bail out.