[libc] Make hdrgen support macro_header YAML field. #123265
Conversation
A macro can specify macro_header instead of macro_value to indicate that an llvm-libc-macros/ header file is supposed to define this macro. This is used for dlfcn.h, which previously bogusly redefined the RTLD_* macros to empty.
|
✅ With the latest revision this PR passed the Python code formatter. |
|
@llvm/pr-subscribers-libc Author: Roland McGrath (frobtech) ChangesA macro can specify macro_header instead of macro_value to Full diff: https://github.com/llvm/llvm-project/pull/123265.diff 9 Files Affected:
diff --git a/libc/include/dlfcn.yaml b/libc/include/dlfcn.yaml
index 9e8803cb5fa785..78bbeff4e60d98 100644
--- a/libc/include/dlfcn.yaml
+++ b/libc/include/dlfcn.yaml
@@ -2,13 +2,13 @@ header: dlfcn.h
header_template: dlfcn.h.def
macros:
- macro_name: RTLD_LAZY
- macro_value: null
+ macro_header: dlfcn-macros.h
- macro_name: RTLD_NOW
- macro_value: null
+ macro_header: dlfcn-macros.h
- macro_name: RTLD_GLOBAL
- macro_value: null
+ macro_header: dlfcn-macros.h
- macro_name: RTLD_LOCAL
- macro_value: null
+ macro_header: dlfcn-macros.h
types: []
enums: []
objects: []
diff --git a/libc/utils/hdrgen/gpu_headers.py b/libc/utils/hdrgen/gpu_headers.py
index 8c4ff6e08b1126..505adfa8fee8a4 100644
--- a/libc/utils/hdrgen/gpu_headers.py
+++ b/libc/utils/hdrgen/gpu_headers.py
@@ -8,6 +8,7 @@
from header import HeaderFile
+
class GpuHeaderFile(HeaderFile):
def __str__(self):
content = []
diff --git a/libc/utils/hdrgen/header.py b/libc/utils/hdrgen/header.py
index 9339acceaf7a97..7e14e768de2bbe 100644
--- a/libc/utils/hdrgen/header.py
+++ b/libc/utils/hdrgen/header.py
@@ -6,6 +6,8 @@
#
# ==-------------------------------------------------------------------------==#
+from pathlib import PurePath
+
class HeaderFile:
def __init__(self, name):
@@ -32,8 +34,20 @@ def add_object(self, object):
def add_function(self, function):
self.functions.append(function)
+ def includes(self):
+ return sorted(
+ {
+ PurePath("llvm-libc-macros") / macro.header
+ for macro in self.macros
+ if macro.header is not None
+ }
+ )
+
def public_api(self):
- content = [""]
+ header_dir = PurePath(self.name).parent
+ content = [
+ f'#include "{file.relative_to(header_dir)}"' for file in self.includes()
+ ] + [""]
for macro in self.macros:
content.append(f"{macro}\n")
@@ -76,11 +90,9 @@ def public_api(self):
content.append(f"#endif // {current_guard}")
content.append("")
- for object in self.objects:
- content.append(str(object))
+ content.extend(str(object) for object in self.objects)
if self.objects:
- content.append("\n__END_C_DECLS")
- else:
- content.append("__END_C_DECLS")
+ content.append("")
+ content.append("__END_C_DECLS")
return "\n".join(content)
diff --git a/libc/utils/hdrgen/macro.py b/libc/utils/hdrgen/macro.py
index 9a712f2a1c743b..e1eb678f7a832e 100644
--- a/libc/utils/hdrgen/macro.py
+++ b/libc/utils/hdrgen/macro.py
@@ -8,12 +8,16 @@
class Macro:
- def __init__(self, name, value=None):
+ def __init__(self, name, value=None, header=None):
self.name = name
self.value = value
+ self.header = header
def __str__(self):
+ if self.header != None:
+ return f"""#ifndef {self.name}
+#error "{self.name} should be defined by llvm-libc-macros/{self.header}"
+#endif"""
if self.value != None:
return f"#define {self.name} {self.value}"
- else:
- return f"#define {self.name}"
+ return f"#define {self.name}"
diff --git a/libc/utils/hdrgen/tests/expected_output/test_header.h b/libc/utils/hdrgen/tests/expected_output/test_header.h
index a777976134b045..97870df58d0e87 100644
--- a/libc/utils/hdrgen/tests/expected_output/test_header.h
+++ b/libc/utils/hdrgen/tests/expected_output/test_header.h
@@ -11,13 +11,25 @@
#include "__llvm-libc-common.h"
#include "llvm-libc-macros/float16-macros.h"
-#include "llvm-libc-macros/test_small-macros.h"
#include "llvm-libc-types/float128.h"
+#include "llvm-libc-macros/test_more-macros.h"
+#include "llvm-libc-macros/test_small-macros.h"
+
#define MACRO_A 1
#define MACRO_B 2
+#define MACRO_C
+
+#ifndef MACRO_D
+#error "MACRO_D should be defined by llvm-libc-macros/test_small-macros.h"
+#endif
+
+#ifndef MACRO_E
+#error "MACRO_E should be defined by llvm-libc-macros/test_more-macros.h"
+#endif
+
#include <llvm-libc-types/type_a.h>
#include <llvm-libc-types/type_b.h>
diff --git a/libc/utils/hdrgen/tests/input/test_small.h.def b/libc/utils/hdrgen/tests/input/test_small.h.def
index 075be957b81368..587b163b68d964 100644
--- a/libc/utils/hdrgen/tests/input/test_small.h.def
+++ b/libc/utils/hdrgen/tests/input/test_small.h.def
@@ -11,7 +11,6 @@
#include "__llvm-libc-common.h"
#include "llvm-libc-macros/float16-macros.h"
-#include "llvm-libc-macros/test_small-macros.h"
#include "llvm-libc-types/float128.h"
%%public_api()
diff --git a/libc/utils/hdrgen/tests/input/test_small.yaml b/libc/utils/hdrgen/tests/input/test_small.yaml
index 1d4b2990a30027..d5bb2bbfe4468c 100644
--- a/libc/utils/hdrgen/tests/input/test_small.yaml
+++ b/libc/utils/hdrgen/tests/input/test_small.yaml
@@ -5,6 +5,11 @@ macros:
macro_value: 1
- macro_name: MACRO_B
macro_value: 2
+ - macro_name: MACRO_C
+ - macro_name: MACRO_D
+ macro_header: test_small-macros.h
+ - macro_name: MACRO_E
+ macro_header: test_more-macros.h
types:
- type_name: type_a
- type_name: type_b
diff --git a/libc/utils/hdrgen/tests/test_integration.py b/libc/utils/hdrgen/tests/test_integration.py
index 49cb08cd1b339d..110a218fbde8f3 100644
--- a/libc/utils/hdrgen/tests/test_integration.py
+++ b/libc/utils/hdrgen/tests/test_integration.py
@@ -10,6 +10,7 @@ def setUp(self):
self.output_dir = TestHeaderGenIntegration.output_dir
self.source_dir = Path(__file__).parent
self.main_script = self.source_dir.parent / "main.py"
+ self.maxDiff = 80*100
def run_script(self, yaml_file, output_file, entry_points):
command = [
diff --git a/libc/utils/hdrgen/yaml_to_classes.py b/libc/utils/hdrgen/yaml_to_classes.py
index d64feafc260b7c..4a3f48633dc1dc 100644
--- a/libc/utils/hdrgen/yaml_to_classes.py
+++ b/libc/utils/hdrgen/yaml_to_classes.py
@@ -38,7 +38,13 @@ def yaml_to_classes(yaml_data, header_class, entry_points=None):
header.template_file = yaml_data.get("header_template")
for macro_data in yaml_data.get("macros", []):
- header.add_macro(Macro(macro_data["macro_name"], macro_data["macro_value"]))
+ header.add_macro(
+ Macro(
+ macro_data["macro_name"],
+ macro_data.get("macro_value"),
+ macro_data.get("macro_header"),
+ )
+ )
types = yaml_data.get("types", [])
sorted_types = sorted(types, key=lambda x: x["type_name"])
|
/nods yeah that's silly. Looking into what The current convention is that if one of these .h files defines macros, then we have (stdio example): libc/include/stdio.h.def so I don't see the point of
So that's 2 reasons to remove If we remove |
|
Ah, so this is more of "add checks to generated headers that if a given macro isn't defined as expected, we will Coincidentally, I just reviewed a patch to bionic where I learned that they have dedicated header tests (seemingly only for POSIX IIUC): https://android.googlesource.com/platform/bionic/+/main/tests/headers/posix/ So I guess those are two different means of testing the generated headers. The trade off being that with the approach in this PR, our headers are bigger; we pay that cost of the preprocessor checking, rechecking, rechecking, ... for every compile, regardless of whether or not we just mean to do a sanity check. Privately, you seemed to indicate that your had more patches that build on top of this PR. Can you describe a little more how you plan to extend this feature? Otherwise, I think I mildly prefer bionic's approach of just having unit tests. We already have this concept in the tests under #ifndef RTLD_LAZY
#error "WTF"
#endif
#ifndef ...which is effectively what the bionic tests are doing. That way, we keep our generated headers smaller (FWIW), and can remove macro_name/macro_value support from hdrgen (#124957). cc @enh-google |
That was really an incidental feature. I've removed it now, because I concur that having the public headers do this self-test logic is not really the best way to do it. We can get the same automation in testing by having a new mode for hdrgen to produce test sources to perform these things. But we can make that a later addition.
I do have some additional changes ready, and I'm considering more along these lines. The overall thrust is to get more of the information to have its single source of truth in YAML and thus be machine-usable and machine-checkable; and also to reduce redundant information that has to be hand-maintained in general. To those ends, my next changes are:
In this vein, I'd like to do more to get more information maintained solely in YAML and machine-usable over time. These aren't on my critical path, which right now is frankly just about producing a sufficient
I agree separate tests are better. Unlike normal unit tests, this kind of test is entirely mechanical and contains no semantic checking. That means that manual maintenance of such tests is especially likely to have cut&paste and omission errors that make it not test as much as intended. So I think it's better to generate these tests from the YAML files as the single source of truth of who is supposed to define what.
I am not in favor of that direction. As I outlined above, I want to push very much in the direction of having the sources of truth be more in YAML, not less--at least for the "what is the API contract?" truths. For macros, so far that's just the name. There are some macros where the value is also standards-required, such as the The |
A macro can specify macro_header instead of macro_value to indicate that an llvm-libc-macros/ header file is supposed to define this macro. This is used for dlfcn.h, which previously bogusly redefined the RTLD_* macros to empty.
A macro can specify macro_header instead of macro_value to indicate that an llvm-libc-macros/ header file is supposed to define this macro. This is used for dlfcn.h, which previously bogusly redefined the RTLD_* macros to empty.
A macro can specify macro_header instead of macro_value to
indicate that an llvm-libc-macros/ header file is supposed to
define this macro. This is used for dlfcn.h, which previously
bogusly redefined the RTLD_* macros to empty.