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.
Details
Details
- Reviewers
djtodoro
Diff Detail
Diff Detail
Event Timeline
Comment Actions
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
Comment Actions
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.
Comment Actions
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.
Comment Actions
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?
Comment Actions
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).
Comment Actions
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?
Comment Actions
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.
Comment Actions
Gentle ping. :)
If I understood correctly, there are two possible solutions:
- Force GCC to also use -D_LARGEFILE_SOURCE, and revert this change back, while also matching this change in GCC.
- Prevent builds of compiler-rt with -D_LARGEFILE_SOURCE, and keep this change as is.
If I'm right, what would be the way to go?
Comment Actions
hi d,
i wanted to say this fix helped, but i wanted to mention the problems i had in sanitiser_linux.cpp that resulted in the failure to build the final shared mulitilib toolchain:
/tools/include/bits/struct_stat.h:190:8: error: redefinition of 'struct stat64'
190 | struct stat64
| ^~~~~~
In file included from ../../../../gcc-12.2.0/libsanitizer/sanitizer_common/sanitizer_linux.cpp:49:
/tools/include/asm/stat.h:51:8: note: previous definition of 'struct stat64'
51 | struct stat64 {
| ^~~~~~
In file included from ../../../../gcc-12.2.0/libsanitizer/sanitizer_common/sanitizer_linux.cpp:184:
../../../../gcc-12.2.0/libsanitizer/sanitizer_common/sanitizer_linux.cpp: In function '__sanitizer::uptr __sanitizer::internal_mmap(void*, uptr, int, int, int, u64)':
../../../../gcc-12.2.0/libsanitizer/sanitizer_common/sanitizer_syscall_generic.inc:19:24: error: '__NR_mmap2' was not declared in this scope
19 | # define SYSCALL(name) __NR_ ## name
| ^~~~~
../../../../gcc-12.2.0/libsanitizer/sanitizer_common/sanitizer_linux.cpp:198:27: note: in expansion of macro 'SYSCALL'
198 | return internal_syscall(SYSCALL(mmap2), addr, length, prot, flags, fd,
| ^~~~~~~
../../../../gcc-12.2.0/libsanitizer/sanitizer_common/sanitizer_linux.cpp: In function 'void __sanitizer::stat64_to_stat(stat64*, stat*)':
../../../../gcc-12.2.0/libsanitizer/sanitizer_common/sanitizer_linux.cpp:279:23: error: 'struct stat64' has no member named 'st_atim'; did you mean 'st_atime'?
279 | out->st_atime = in->st_atime;
| ^~~~~~~~
../../../../gcc-12.2.0/libsanitizer/sanitizer_common/sanitizer_linux.cpp:280:23: error: 'struct stat64' has no member named 'st_mtim'; did you mean 'st_mtime'?
280 | out->st_mtime = in->st_mtime;
| ^~~~~~~~
../../../../gcc-12.2.0/libsanitizer/sanitizer_common/sanitizer_linux.cpp:281:23: error: 'struct stat64' has no member named 'st_ctim'; did you mean 'st_ctime'?
281 | out->st_ctime = in->st_ctime;
| ^~~~~~~~
../../../../gcc-12.2.0/libsanitizer/sanitizer_common/sanitizer_linux.cpp: In function '__sanitizer::uptr __sanitizer::internal_stat(const char*, void*)':
../../../../gcc-12.2.0/libsanitizer/sanitizer_common/sanitizer_syscall_generic.inc:19:24: error: '__NR_stat64' was not declared in this scope
19 | # define SYSCALL(name) __NR_ ## name
| ^~~~~
../../../../gcc-12.2.0/libsanitizer/sanitizer_common/sanitizer_linux.cpp:353:30: note: in expansion of macro 'SYSCALL'
353 | int res = internal_syscall(SYSCALL(stat64), path, &buf64);
| ^~~~~~~
../../../../gcc-12.2.0/libsanitizer/sanitizer_common/sanitizer_linux.cpp: In function '__sanitizer::uptr __sanitizer::internal_lstat(const char*, void*)':
../../../../gcc-12.2.0/libsanitizer/sanitizer_common/sanitizer_syscall_generic.inc:19:24: error: '__NR_lstat64' was not declared in this scope
19 | # define SYSCALL(name) __NR_ ## name
| ^~~~~
../../../../gcc-12.2.0/libsanitizer/sanitizer_common/sanitizer_linux.cpp:378:30: note: in expansion of macro 'SYSCALL'
378 | int res = internal_syscall(SYSCALL(lstat64), path, &buf64);
| ^~~~~~~
../../../../gcc-12.2.0/libsanitizer/sanitizer_common/sanitizer_linux.cpp: In function '__sanitizer::uptr __sanitizer::internal_fstat(fd_t, void*)':
../../../../gcc-12.2.0/libsanitizer/sanitizer_common/sanitizer_syscall_generic.inc:19:24: error: '__NR_fstat64' was not declared in this scope
19 | # define SYSCALL(name) __NR_ ## name
| ^~~~~
../../../../gcc-12.2.0/libsanitizer/sanitizer_common/sanitizer_linux.cpp:397:30: note: in expansion of macro 'SYSCALL'
397 | int res = internal_syscall(SYSCALL(fstat64), fd, &buf64);
| ^~~~~~~
../../../../gcc-12.2.0/libsanitizer/sanitizer_common/sanitizer_linux.cpp: At global scope:
../../../../gcc-12.2.0/libsanitizer/sanitizer_common/sanitizer_linux.cpp:303:13: warning: 'void __sanitizer::kernel_stat_to_stat(kernel_stat*, stat*)' defined but not used [-Wunused-function]
303 | static void kernel_stat_to_stat(struct kernel_stat *in, struct stat *out) {the solution i found was to use a portion of this commit (of yours):
https://github.com/gcc-mirror/gcc/commit/ee915c72da2caf92697dbedf0d9d9730ce9aca7a
specifically i made the first two changes verbatim, but then i simply repasted the (fixed) internal_stat function to replace the existing one, as i think the logic is broken.
this resulted in a successful build.
i have no doubt that the code has evolved since this issue originally arose and you have fixed both of the problems, but i wanted to share this anyways because i was building a multilib toolchain (again, two months later after i produced one with the 'fix' shared above) and had to look at my own comment on the gcc mailing list lol.
Comment Actions
Hi @broly,
Thanks for sharing. :)
[0] and [1] should have fixed all of the issues in regards to building the libsanitizer for the N32 ABI. The issues you mentioned specifically were handled by [0].
I tested builds of both libsanitizer and compiler-rt for the N32 ABI and they are both successful. For other ABIs, there are no such issues, as far as I remember.
Just to make sure I understood you correctly, everything works for you as well now (without having to manually change things up)? :) I haven't really tried building a multilib toolchain.
[0] https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=ee915c72da2caf92697dbedf0d9d9730ce9aca7a
[1] https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=1efeaf99bd8bdfe2a350b8a56b88ed6e00594e54
Comment Actions
hi,
yes when i look at the mainline git it seems those strange CANONICAL_LINUX macros have been removed, just not in time for the 12.2.0 release.
assuming this commit makes it in for the next release, then yes we're sitting pretty.
the nice thing about mips multilib toolchains is they cover 3 modes. try building and ARM "multilib" toolchain and you'll see how big a "multilib" compiler can really get.
really opened my eyes about how they could not escape being a 'disposable' processor (imo).
just a question for the MIPS lads: isn't there a way to make a bi-endian toolchain? i mean i have both a mips64el and mips64 multilib toolchain, and it's not too much of a hassle to make them.
but i just thought since all they differ in is endianness, that down the road we could add some kind of command line switch to a newly-named target that wouldn't interfere with legacy builds.
Comment Actions
my apologies, i should open a new ticket if this is indeed an error.
but it may be my fault for not specifying the ARCH parameter when installing the linux headers prior to starting the toolchain.
if it isn't my fault, i will open a new ticket.