-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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] Added transitive bindings for OffsetType #87397
Conversation
cc: @nickdesaulniers |
@michaelrj-google @lntue , with this change checked out locally ( Perhaps instead, libc/include/stdio.h.def and libc/include/fcntl.h.def need to be modified?
|
Ah, libc/config/linux/api.td!!! diff --git a/libc/config/linux/api.td b/libc/config/linux/api.td
index eb5ed8089850..6ae39f385c07 100644
--- a/libc/config/linux/api.td
+++ b/libc/config/linux/api.td
@@ -49,7 +49,7 @@ def CTypeAPI : PublicAPI<"ctype.h"> {
}
def FCntlAPI : PublicAPI<"fcntl.h"> {
- let Types = ["mode_t"];
+ let Types = ["mode_t", "off_t"];
}
def IntTypesAPI : PublicAPI<"inttypes.h"> {
@@ -77,7 +77,7 @@ def StdIOAPI : PublicAPI<"stdio.h"> {
SimpleMacroDef<"_IOLBF", "1">,
SimpleMacroDef<"_IONBF", "2">,
];
- let Types = ["size_t", "FILE", "cookie_io_functions_t"];
+ let Types = ["size_t", "FILE", "cookie_io_functions_t", "off_t"];
}
def StdlibAPI : PublicAPI<"stdlib.h"> { (but we should start putting these on their own line) I wonder if libc/include/CMakeLists.txt should also add DEPENDS? Seems not necessary to fix the build, but... |
So I think
Look like you also need to add those types to https://github.com/llvm/llvm-project/blob/main/libc/config/linux/api.td#L80 for it to automatically generate the includes. |
Made the changes. Any clarification on the point of Types in HeaderSpecs though? |
For the moment, defining the types in the |
@Sh0g0-1758 please make that change.
@michaelrj-google is that in addition to changes to libc/config/linux/api.td? Do we we need to modify other api.td files? Seems like modifying the api.td files is necessary to fix the build; so I'm not sure still what the point of the
@michaelrj-google do we also need something like: diff --git a/libc/include/CMakeLists.txt b/libc/include/CMakeLists.txt
index 4203f0bc901b..02c7dc8fbc0b 100644
--- a/libc/include/CMakeLists.txt
+++ b/libc/include/CMakeLists.txt
@@ -41,9 +41,10 @@ add_gen_header(
DEF_FILE fcntl.h.def
GEN_HDR fcntl.h
DEPENDS
- .llvm_libc_common_h
.llvm-libc-macros.fcntl_macros
.llvm-libc-types.mode_t
+ .llvm-libc-types.off_t
+ .llvm_libc_common_h
)
add_gen_header(
@@ -264,13 +265,14 @@ add_gen_header(
DEF_FILE stdio.h.def
GEN_HDR stdio.h
DEPENDS
- .llvm_libc_common_h
.llvm-libc-macros.file_seek_macros
.llvm-libc-macros.stdio_macros
- .llvm-libc-types.size_t
- .llvm-libc-types.ssize_t
.llvm-libc-types.FILE
.llvm-libc-types.cookie_io_functions_t
+ .llvm-libc-types.off_t
+ .llvm-libc-types.size_t
+ .llvm-libc-types.ssize_t
+ .llvm_libc_common_h
)
add_gen_header( (which again, feels like boilerplate) |
Yes, you also still need to modify Yes you also need to add the dependency in cmake, that's what tells the build system which type headers in |
@nickdesaulniers, made the required changes. Was confused by what you meant by And I have pushed changes in CMakeLists.txt also as suggested by michael. |
In that case, we'll also need similar changes to libc/config/gpu/api.td and libc/config/baremetal/api.td. |
@llvm/pr-subscribers-libc Author: Shourya Goel (Sh0g0-1758) ChangesAdding OffTType to fcntl.h and stdio.h 's Macro lists in libc/spec/posix.td as mentioned here: #87266 Full diff: https://github.com/llvm/llvm-project/pull/87397.diff 7 Files Affected:
diff --git a/libc/config/baremetal/api.td b/libc/config/baremetal/api.td
index 25aa06aacb642e..690edbda1311fe 100644
--- a/libc/config/baremetal/api.td
+++ b/libc/config/baremetal/api.td
@@ -57,7 +57,10 @@ def MathAPI : PublicAPI<"math.h"> {
}
def StdIOAPI : PublicAPI<"stdio.h"> {
- let Types = ["size_t"];
+ let Types = [
+ "size_t",
+ "off_t",
+ ];
}
def StdlibAPI : PublicAPI<"stdlib.h"> {
diff --git a/libc/config/gpu/api.td b/libc/config/gpu/api.td
index adaf5bfd747ac7..523ad49ffa3fd4 100644
--- a/libc/config/gpu/api.td
+++ b/libc/config/gpu/api.td
@@ -64,7 +64,11 @@ def StdIOAPI : PublicAPI<"stdio.h"> {
SimpleMacroDef<"_IOLBF", "1">,
SimpleMacroDef<"_IONBF", "2">,
];
- let Types = ["size_t", "FILE"];
+ let Types = [
+ "FILE",
+ "off_t",
+ "size_t",
+ ];
}
def IntTypesAPI : PublicAPI<"inttypes.h"> {
diff --git a/libc/config/linux/api.td b/libc/config/linux/api.td
index eb5ed8089850e4..9964971f191b75 100644
--- a/libc/config/linux/api.td
+++ b/libc/config/linux/api.td
@@ -49,7 +49,10 @@ def CTypeAPI : PublicAPI<"ctype.h"> {
}
def FCntlAPI : PublicAPI<"fcntl.h"> {
- let Types = ["mode_t"];
+ let Types = [
+ "mode_t",
+ "off_t",
+ ];
}
def IntTypesAPI : PublicAPI<"inttypes.h"> {
@@ -77,7 +80,12 @@ def StdIOAPI : PublicAPI<"stdio.h"> {
SimpleMacroDef<"_IOLBF", "1">,
SimpleMacroDef<"_IONBF", "2">,
];
- let Types = ["size_t", "FILE", "cookie_io_functions_t"];
+ let Types = [
+ "FILE",
+ "cookie_io_functions_t",
+ "off_t",
+ "size_t",
+ ];
}
def StdlibAPI : PublicAPI<"stdlib.h"> {
diff --git a/libc/include/CMakeLists.txt b/libc/include/CMakeLists.txt
index 4203f0bc901b22..02c7dc8fbc0b33 100644
--- a/libc/include/CMakeLists.txt
+++ b/libc/include/CMakeLists.txt
@@ -41,9 +41,10 @@ add_gen_header(
DEF_FILE fcntl.h.def
GEN_HDR fcntl.h
DEPENDS
- .llvm_libc_common_h
.llvm-libc-macros.fcntl_macros
.llvm-libc-types.mode_t
+ .llvm-libc-types.off_t
+ .llvm_libc_common_h
)
add_gen_header(
@@ -264,13 +265,14 @@ add_gen_header(
DEF_FILE stdio.h.def
GEN_HDR stdio.h
DEPENDS
- .llvm_libc_common_h
.llvm-libc-macros.file_seek_macros
.llvm-libc-macros.stdio_macros
- .llvm-libc-types.size_t
- .llvm-libc-types.ssize_t
.llvm-libc-types.FILE
.llvm-libc-types.cookie_io_functions_t
+ .llvm-libc-types.off_t
+ .llvm-libc-types.size_t
+ .llvm-libc-types.ssize_t
+ .llvm_libc_common_h
)
add_gen_header(
diff --git a/libc/spec/posix.td b/libc/spec/posix.td
index cfa8d3afedde3f..45f7ecfe84e98e 100644
--- a/libc/spec/posix.td
+++ b/libc/spec/posix.td
@@ -210,7 +210,10 @@ def POSIX : StandardSpec<"POSIX"> {
HeaderSpec FCntl = HeaderSpec<
"fcntl.h",
[], // Macros
- [ModeTType],
+ [
+ ModeTType,
+ OffTType,
+ ],
[], // Enumerations
[
FunctionSpec<
@@ -1180,7 +1183,7 @@ def POSIX : StandardSpec<"POSIX"> {
HeaderSpec StdIO = HeaderSpec<
"stdio.h",
[], // Macros
- [], // Types
+ [OffTType], // Types
[], // Enumerations
[
FunctionSpec<
diff --git a/libc/src/stdio/fseeko.h b/libc/src/stdio/fseeko.h
index 3202ed2f97d0ef..77fb41215c318f 100644
--- a/libc/src/stdio/fseeko.h
+++ b/libc/src/stdio/fseeko.h
@@ -10,7 +10,6 @@
#define LLVM_LIBC_SRC_STDIO_FSEEKO_H
#include <stdio.h>
-#include <unistd.h>
namespace LIBC_NAMESPACE {
diff --git a/libc/src/stdio/ftello.h b/libc/src/stdio/ftello.h
index 0fdf13ab6bdbcd..5ab17f9244a5ad 100644
--- a/libc/src/stdio/ftello.h
+++ b/libc/src/stdio/ftello.h
@@ -10,7 +10,6 @@
#define LLVM_LIBC_SRC_STDIO_FTELLO_H
#include <stdio.h>
-#include <unistd.h>
namespace LIBC_NAMESPACE {
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; thanks for the patch! 🐰 🌵
Ah :) Any context for the emojis? |
@nickdesaulniers, I would be needing you to land this for me. |
Nope, just me being silly again.
Ack. Thanks for the patch! |
This patch broke our Clang toolchain builders.
Could you please fix or revert it? |
This reverts commit 3ee93f4 because it broke Fuchsia Clang toolchain builders: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8751633430491432833/+/u/clang/build/stdout
Hmm. It looks like that error message could have come from 1 of 2 places:
So it's looking at
RequiredTypes :
Perhaps it's because @Sh0g0-1758 can you please:
@gulfemsavrun can you please help test the PR once issued? |
When the PR is issued, please let me know and I'm happy to verify it. |
@gulfemsavrun, opened a PR. |
Followup to issues addressed here: #87397
Adding OffTType to fcntl.h and stdio.h 's Macro lists in libc/spec/posix.td as mentioned here: #87266