Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc] fix -Wshorten-64-to-32 for 32b arm mmap #77395

Closed
nickdesaulniers opened this issue Jan 9, 2024 · 7 comments · Fixed by #77810
Closed

[libc] fix -Wshorten-64-to-32 for 32b arm mmap #77395

nickdesaulniers opened this issue Jan 9, 2024 · 7 comments · Fixed by #77810
Assignees

Comments

@nickdesaulniers
Copy link
Member

#77350 was an attempted fix that didn't work. Filing a TODO to track a quick workaround to get the build bot green, until we can fix this properly.

@github-actions github-actions bot added the libc label Jan 9, 2024
nickdesaulniers added a commit to nickdesaulniers/llvm-project that referenced this issue Jan 9, 2024
This is still broken after llvm#77350. Disable the warning for now, and fix
properly once the buildbot it back to green.

Link: llvm#77395
nickdesaulniers added a commit that referenced this issue Jan 9, 2024
This is still broken after #77350. Disable the warning for now, and fix
properly once the buildbot it back to green.

Link: #77395
@nickdesaulniers nickdesaulniers self-assigned this Jan 9, 2024
@nickdesaulniers
Copy link
Member Author

/usr/include/arm-linux-gnueabihf/sys/mman.h on the 32b arm buildbot has:

#ifndef __off_t_defined
# ifndef __USE_FILE_OFFSET64
typedef __off_t off_t;
# else
typedef __off64_t off_t;
# endif
# define __off_t_defined
#endif

and preprocessing libc/src/sys/mman/linux/mmap.cpp I see:

# 26 "/usr/include/arm-linux-gnueabihf/sys/mman.h" 2 3 4
typedef __off64_t off_t;

so that's where the 64b off_t is coming from.

@nickdesaulniers
Copy link
Member Author

We have a libc/include/llvm-libc-types/off_t.h, should libc/src/sys/mman/mmap.h be explicitly including that rather than <sys/mman.h>?

@nickdesaulniers
Copy link
Member Author

I see _LARGEFILE_SOURCE getting defined when building this TU, but...I only see that being added to the compile options in
llvm/cmake/modules/HandleLLVMOptions.cmake, and llvm/cmake/config-ix.cmake.

That's not polluting our build flags, is it?

Otherwise, perhaps we need to provide an implementation of the syscall that accepts a long long for that param.

@nickdesaulniers
Copy link
Member Author

if( CMAKE_SIZEOF_VOID_P EQUAL 4 AND NOT LLVM_FORCE_SMALLFILE_FOR_ANDROID)
# FIXME: It isn't handled in LLVM_BUILD_32_BITS.
add_compile_definitions(_LARGEFILE_SOURCE)
add_compile_definitions(_FILE_OFFSET_BITS=64)
endif()
is leaking into our build rules, which is what's forcing off_t to be 64b on 32b targets. I suspect we may want to avoid that, but need to think more about the implications of disabling that.

I suspect it may be a question of (do we want to match the platform's ABI vs do our own ABI).

@nickdesaulniers
Copy link
Member Author

FWICT, it looks like we're inheriting:

-- Found Define: _GNU_SOURCE
-- Found Define: _FILE_OFFSET_BITS=64
-- Found Define: _DEBUG
-- Found Define: _GLIBCXX_ASSERTIONS
-- Found Define: _LARGEFILE_SOURCE
-- Found Define: _FILE_OFFSET_BITS=64
-- Found Define: __STDC_CONSTANT_MACROS
-- Found Define: __STDC_FORMAT_MACROS
-- Found Define: __STDC_LIMIT_MACROS

preprocessor defines. I don't think we want any of those (at least for now).

nickdesaulniers added a commit to nickdesaulniers/llvm-project that referenced this issue Jan 11, 2024
While trying to enable -Werror (llvm#74506), the 32b ARM build bot reported an
error stemming from -Wshorten-64-to-32 related to usages of `off_t`.

I failed to fix these properly in llvm#77350 (the 32b ARM build is not a fullbuild)
and llvm#77396.

It turns out, the preprocessor defines `-D_LARGEFILE_SOURCE` and
`-D_FILE_OFFSET_BITS=64` were being set for llvmlibc when using the cmake build
system. In particular, these preprocessor defines are feature test macros used
by glibc, and which have effects no the corresponding ABI for types like
`off_t` (for instance, should `off_t` be 32b or 64b on 32b targets).

But who was setting these?  Turns out that the use of add_compile_definitions
in llvm/cmake/modules/HandleLLVMOptions.cmake was setting these (and more),
which is then inherited by every subdirectory.  While some of these defines
maybe make sense for host builds, they do not make sense for libraries for the
target. The full list of defines being set prior to this commit:

- `-D_GNU_SOURCE`
- `-D_FILE_OFFSET_BITS=64`
- `-D_DEBUG`
- `-D_GLIBCXX_ASSERTIONS`
- `-D_LARGEFILE_SOURCE`
- `-D_FILE_OFFSET_BITS=64`
- `-D__STDC_CONSTANT_MACROS`
- `-D__STDC_FORMAT_MACROS`
- `-D__STDC_LIMIT_MACROS`

If we desire any of the above, we should manually reset them.

Fix this by resetting COMPILE_DEFINITIONS for the libc/ subdirectory.

Side note: to debug 'directory properties' in cmake, you first need to use
`get_directory_property` to fetch the corresponding value into a variable
first, then that variable can be printed via `message`.

Link: https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fFILE_005fOFFSET_005fBITS
Link: https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fFILE_005fOFFSET_005fBITS

Fixes: llvm#77395
nickdesaulniers added a commit that referenced this issue Jan 16, 2024
While trying to enable -Werror (#74506), the 32b ARM build bot reported
an
error stemming from -Wshorten-64-to-32 related to usages of `off_t`.

I failed to fix these properly in #77350 (the 32b ARM build is not a
fullbuild)
and #77396.

It turns out, the preprocessor defines `-D_LARGEFILE_SOURCE` and
`-D_FILE_OFFSET_BITS=64` were being set for llvmlibc when using the
cmake build
system. In particular, these preprocessor defines are feature test
macros used
by glibc, and which have effects no the corresponding ABI for types like
`off_t` (for instance, should `off_t` be 32b or 64b on 32b targets).

But who was setting these? Turns out that the use of
add_compile_definitions
in llvm/cmake/modules/HandleLLVMOptions.cmake was setting these (and
more),
which is then inherited by every subdirectory. While some of these
defines
maybe make sense for host builds, they do not make sense for libraries
for the
target. The full list of defines being set prior to this commit:

- `-D_GNU_SOURCE`
- `-D_FILE_OFFSET_BITS=64`
- `-D_DEBUG`
- `-D_GLIBCXX_ASSERTIONS`
- `-D_LARGEFILE_SOURCE`
- `-D_FILE_OFFSET_BITS=64`
- `-D__STDC_CONSTANT_MACROS`
- `-D__STDC_FORMAT_MACROS`
- `-D__STDC_LIMIT_MACROS`

If we desire any of the above, we should manually reset them.

Fix this by resetting COMPILE_DEFINITIONS for the libc/ subdirectory.

Side note: to debug 'directory properties' in cmake, you first need to
use
`get_directory_property` to fetch the corresponding value into a
variable
first, then that variable can be printed via `message`.

Link:
https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fFILE_005fOFFSET_005fBITS
Link:
https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fFILE_005fOFFSET_005fBITS

Fixes: #77395
nickdesaulniers added a commit to nickdesaulniers/llvm-project that referenced this issue Jan 16, 2024
This reverts commit 70cea91.

The correct fix was
commit f1f1875 ("[libc][cmake] reset COMPILE_DEFINITIONS (llvm#77810)")

Link: llvm#77395
nickdesaulniers added a commit to nickdesaulniers/llvm-project that referenced this issue Jan 16, 2024
This reverts commit ce1305a.

The correct fix was
commit f1f1875 ("[libc][cmake] reset COMPILE_DEFINITIONS (llvm#77810)")

Link: llvm#77395
@EugeneZelenko
Copy link
Contributor

Patch was reverted.

@EugeneZelenko EugeneZelenko reopened this Jan 16, 2024
@nickdesaulniers
Copy link
Member Author

#78307 will resolve this finally.

justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this issue Jan 28, 2024
This is still broken after llvm#77350. Disable the warning for now, and fix
properly once the buildbot it back to green.

Link: llvm#77395
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this issue Jan 28, 2024
While trying to enable -Werror (llvm#74506), the 32b ARM build bot reported
an
error stemming from -Wshorten-64-to-32 related to usages of `off_t`.

I failed to fix these properly in llvm#77350 (the 32b ARM build is not a
fullbuild)
and llvm#77396.

It turns out, the preprocessor defines `-D_LARGEFILE_SOURCE` and
`-D_FILE_OFFSET_BITS=64` were being set for llvmlibc when using the
cmake build
system. In particular, these preprocessor defines are feature test
macros used
by glibc, and which have effects no the corresponding ABI for types like
`off_t` (for instance, should `off_t` be 32b or 64b on 32b targets).

But who was setting these? Turns out that the use of
add_compile_definitions
in llvm/cmake/modules/HandleLLVMOptions.cmake was setting these (and
more),
which is then inherited by every subdirectory. While some of these
defines
maybe make sense for host builds, they do not make sense for libraries
for the
target. The full list of defines being set prior to this commit:

- `-D_GNU_SOURCE`
- `-D_FILE_OFFSET_BITS=64`
- `-D_DEBUG`
- `-D_GLIBCXX_ASSERTIONS`
- `-D_LARGEFILE_SOURCE`
- `-D_FILE_OFFSET_BITS=64`
- `-D__STDC_CONSTANT_MACROS`
- `-D__STDC_FORMAT_MACROS`
- `-D__STDC_LIMIT_MACROS`

If we desire any of the above, we should manually reset them.

Fix this by resetting COMPILE_DEFINITIONS for the libc/ subdirectory.

Side note: to debug 'directory properties' in cmake, you first need to
use
`get_directory_property` to fetch the corresponding value into a
variable
first, then that variable can be printed via `message`.

Link:
https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fFILE_005fOFFSET_005fBITS
Link:
https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fFILE_005fOFFSET_005fBITS

Fixes: llvm#77395
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants