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] Additional check for File support in scanf_core #128079

Closed
wants to merge 1 commit into from

Conversation

Caslyn
Copy link
Contributor

@Caslyn Caslyn commented Feb 20, 2025

This adds a check for TARGET libc.src.__support.File.file before appending libc.src.__support.File.file to file_deps.

This adds a check for `TARGET libc.src.__support.File.file` before
appending ` libc.src.__support.File.file` to file_deps.
@llvmbot
Copy link
Member

llvmbot commented Feb 20, 2025

@llvm/pr-subscribers-libc

Author: Caslyn Tonelli (Caslyn)

Changes

This adds a check for TARGET libc.src.__support.File.file before appending libc.src.__support.File.file to file_deps.


Full diff: https://github.com/llvm/llvm-project/pull/128079.diff

1 Files Affected:

  • (modified) libc/src/stdio/scanf_core/CMakeLists.txt (+1-1)
diff --git a/libc/src/stdio/scanf_core/CMakeLists.txt b/libc/src/stdio/scanf_core/CMakeLists.txt
index ce639fe65a106..813f4213d160a 100644
--- a/libc/src/stdio/scanf_core/CMakeLists.txt
+++ b/libc/src/stdio/scanf_core/CMakeLists.txt
@@ -16,7 +16,7 @@ if(LIBC_TARGET_OS_IS_GPU)
     libc.src.stdio.ungetc
     libc.src.stdio.ferror
   )
-elseif(LLVM_LIBC_FULL_BUILD)
+elseif(TARGET libc.src.__support.File.file AND LLVM_LIBC_FULL_BUILD)
   list(APPEND file_deps
     libc.src.__support.File.file
   )

@Caslyn
Copy link
Contributor Author

Caslyn commented Feb 20, 2025

@michaelrj-google - I ought to have tested your previous patch locally. I think this might be the missing piece to resolve the error on our Fuchsia builder.

@Caslyn Caslyn self-assigned this Feb 20, 2025
Copy link
Contributor

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@michaelrj-google
Copy link
Contributor

I'm not sure if this will work for baremetal, since right now the fscanf scanner requires either the system's FILE or our own FILE. We might need to add a hook similar to what's in https://github.com/llvm/llvm-project/blob/main/libc/src/stdio/baremetal/printf.cpp, or alternately just add baremetal versions of getc and ungetc

@petrhosek
Copy link
Member

I'm not sure if this will work for baremetal, since right now the fscanf scanner requires either the system's FILE or our own FILE. We might need to add a hook similar to what's in https://github.com/llvm/llvm-project/blob/main/libc/src/stdio/baremetal/printf.cpp, or alternately just add baremetal versions of getc and ungetc

I have a WIP implementation of that locally, I'll clean it up and send it for review.

@michaelrj-google
Copy link
Contributor

I'm not sure if this will work for baremetal, since right now the fscanf scanner requires either the system's FILE or our own FILE. We might need to add a hook similar to what's in https://github.com/llvm/llvm-project/blob/main/libc/src/stdio/baremetal/printf.cpp, or alternately just add baremetal versions of getc and ungetc

I have a WIP implementation of that locally, I'll clean it up and send it for review.

Would it make sense to turn off scanf on baremetal temporarily to unbreak the bots then?

@petrhosek
Copy link
Member

I'm not sure if this will work for baremetal, since right now the fscanf scanner requires either the system's FILE or our own FILE. We might need to add a hook similar to what's in https://github.com/llvm/llvm-project/blob/main/libc/src/stdio/baremetal/printf.cpp, or alternately just add baremetal versions of getc and ungetc

I have a WIP implementation of that locally, I'll clean it up and send it for review.

Would it make sense to turn off scanf on baremetal temporarily to unbreak the bots then?

It's not included in any of the baremetal entrypoints.txt files so I'm not sure why it's being pulled in.

@michaelrj-google
Copy link
Contributor

I'm not sure if this will work for baremetal, since right now the fscanf scanner requires either the system's FILE or our own FILE. We might need to add a hook similar to what's in https://github.com/llvm/llvm-project/blob/main/libc/src/stdio/baremetal/printf.cpp, or alternately just add baremetal versions of getc and ungetc

I have a WIP implementation of that locally, I'll clean it up and send it for review.

Would it make sense to turn off scanf on baremetal temporarily to unbreak the bots then?

It's not included in any of the baremetal entrypoints.txt files so I'm not sure why it's being pulled in.

Ah, in that case this might fix the build. These aren't entrypoints so they're being built unconditionally, and if nothing is providing getc, ungetc, and ferror they should fail to link, but if nothing tries to link them it might just pass.

michaelrj-google added a commit to michaelrj-google/llvm-project that referenced this pull request Feb 21, 2025
A temporary fix based on discussions in llvm#128079

Currently, baremetal targets are failing to build because the scanf
internals require FILE* (either from the system's libc or our libc).
Normally we'd just turn off the broken entrypoint, but since the scanf
internals are built separately that doesn't work. This patch adds extra
conditions to building those internals, which we can hopefully remove
once we have a proper way to build scanf for embedded.
@Caslyn
Copy link
Contributor Author

Caslyn commented Feb 21, 2025

See #128097

@Caslyn Caslyn closed this Feb 21, 2025
michaelrj-google added a commit that referenced this pull request Feb 21, 2025
A temporary fix based on discussions in #128079

Currently, baremetal targets are failing to build because the scanf
internals require FILE* (either from the system's libc or our libc).
Normally we'd just turn off the broken entrypoint, but since the scanf
internals are built separately that doesn't work. This patch adds extra
conditions to building those internals, which we can hopefully remove
once we have a proper way to build scanf for embedded.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants