[libc] Fix scanf cmake for targets without FILE #128056
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-libc Author: Michael Jones (michaelrj-google) ChangesAnother followup fix to #121215 The new cmake wouldn't define the readerat all if the target wasn't GPU As a followup, I'd like to make Full diff: https://github.com/llvm/llvm-project/pull/128056.diff 2 Files Affected:
diff --git a/libc/src/stdio/scanf_core/CMakeLists.txt b/libc/src/stdio/scanf_core/CMakeLists.txt
index 35b8b3d318a9f..ef1fb477e8e29 100644
--- a/libc/src/stdio/scanf_core/CMakeLists.txt
+++ b/libc/src/stdio/scanf_core/CMakeLists.txt
@@ -8,6 +8,22 @@ if(scanf_config_copts)
list(PREPEND scanf_config_copts "COMPILE_OPTIONS")
endif()
+
+list(APPEND file_deps
+ libc.hdr.types.FILE
+)
+if(LIBC_TARGET_OS_IS_GPU)
+ list(APPEND file_deps
+ libc.src.stdio.getc
+ libc.src.stdio.ungetc
+ libc.src.stdio.ferror
+ )
+elseif(LLVM_LIBC_FULL_BUILD)
+ list(APPEND file_deps
+ libc.src.__support.File.file
+ )
+endif()
+
add_header_library(
scanf_config
HDRS
@@ -52,28 +68,19 @@ add_object_library(
.converter
.core_structs
libc.src.__support.arg_list
+ ${file_deps}
+ ${use_system_file}
)
-if(LIBC_TARGET_OS_IS_GPU)
-add_header_library(
- reader
- HDRS
- reader.h
- DEPENDS
- libc.src.__support.macros.attributes
-)
-elseif((TARGET libc.src.__support.File.file) OR (NOT LLVM_LIBC_FULL_BUILD))
add_header_library(
reader
HDRS
reader.h
DEPENDS
libc.src.__support.macros.attributes
- libc.hdr.types.FILE
- libc.src.__support.File.file
+ ${file_deps}
${use_system_file}
)
-endif()
add_object_library(
converter
@@ -101,33 +108,19 @@ add_object_library(
libc.src.__support.CPP.limits
libc.src.__support.char_vector
libc.src.__support.str_to_float
+ ${file_deps}
+ ${use_system_file}
)
-if(LIBC_TARGET_OS_IS_GPU)
- add_header_library(
- vfscanf_internal
- HDRS
- vfscanf_internal.h
- DEPENDS
- .reader
- .scanf_main
- libc.include.stdio
- libc.src.__support.arg_list
- libc.src.stdio.getc
- libc.src.stdio.ungetc
- libc.src.stdio.ferror
- )
-elseif(TARGET libc.src.__support.File.file OR (NOT LLVM_LIBC_FULL_BUILD))
- add_header_library(
+#TODO: condense the file-related code as possible.
+add_header_library(
vfscanf_internal
HDRS
vfscanf_internal.h
DEPENDS
.reader
.scanf_main
- libc.include.stdio
- libc.src.__support.File.file
libc.src.__support.arg_list
+ ${file_deps}
${use_system_file}
- )
-endif()
+)
diff --git a/libc/test/src/stdio/scanf_core/CMakeLists.txt b/libc/test/src/stdio/scanf_core/CMakeLists.txt
index 06735ddb23be7..9cdc6547821ee 100644
--- a/libc/test/src/stdio/scanf_core/CMakeLists.txt
+++ b/libc/test/src/stdio/scanf_core/CMakeLists.txt
@@ -1,3 +1,8 @@
+if(NOT(LLVM_LIBC_FULL_BUILD))
+ # in overlay mode, use the system's file to test the reader.
+ set(use_system_file "-DLIBC_COPT_STDIO_USE_SYSTEM_FILE")
+endif()
+
add_libc_unittest(
parser_test
SUITE
@@ -22,14 +27,10 @@ add_libc_unittest(
DEPENDS
libc.src.stdio.scanf_core.reader
libc.src.__support.CPP.string_view
+ COMPILE_OPTIONS
+ ${use_system_file}
)
-if(NOT (TARGET libc.src.__support.File.file))
- # Not all platforms have a file implementation. If file is unvailable,
- # then we must skip all the parts that need file.
- return()
-endif()
-
add_libc_unittest(
converter_test
SUITE
@@ -40,4 +41,6 @@ add_libc_unittest(
libc.src.stdio.scanf_core.reader
libc.src.stdio.scanf_core.converter
libc.src.__support.CPP.string_view
+ COMPILE_OPTIONS
+ ${use_system_file}
)
|
petrhosek
approved these changes
Feb 20, 2025
Another followup fix to llvm#121215 The new cmake wouldn't define the readerat all if the target wasn't GPU or didn't have a definition of FILE. This patch rewrites the cmake to be more general. As a followup, I'd like to make `use_system_file` consistent between /test and /src. Currently in /src it includes the `COMPILE_OPTIONS` and in /test it does not.
michaelrj-google
force-pushed
the
libcScanfCmakeFixup
branch
from
ca46374 to
535c21f
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/13370 Here is the relevant piece of the build log for the reference |
|
This failure seems unrelated |
|
Yeah, it's been doing that. Fortunately we'll soon have a new gpu libc bot that doesn't run the flaky offload tests. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Another followup fix to #121215
The new cmake wouldn't define the readerat all if the target wasn't GPU
or didn't have a definition of FILE. This patch rewrites the cmake to be
more general.
As a followup, I'd like to make
use_system_fileconsistent between/test and /src. Currently in /src it includes the
COMPILE_OPTIONSandin /test it does not.