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] Add "include/" to the LLVM include directories #83199

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

jhuber6
Copy link
Contributor

@jhuber6 jhuber6 commented Feb 27, 2024

Summary:
Recent changes added an include path in the float128 type that used the
internal libc path to find the macro. This doesn't work once it's
installed because we need to search from the root of the install dir.
This patch adds "include/" to the include path so that our inclusion
of installed headers always match the internal use.

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 27, 2024

@llvm/pr-subscribers-libc

Author: Joseph Huber (jhuber6)

Changes

Summary:
Recent changes added an include path in the float128 type that used the
internal libc path to find the macro. This doesn't work once it's
installed because we need to search from the root of the install dir.
Move the include to the top-level math.h so we can find it.


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

3 Files Affected:

  • (modified) libc/include/CMakeLists.txt (+1)
  • (modified) libc/include/llvm-libc-types/float128.h (-2)
  • (modified) libc/include/math.h.def (+1-1)
diff --git a/libc/include/CMakeLists.txt b/libc/include/CMakeLists.txt
index 9090b3bca01e0d..fc16ce0c27cbbd 100644
--- a/libc/include/CMakeLists.txt
+++ b/libc/include/CMakeLists.txt
@@ -99,6 +99,7 @@ add_gen_header(
   DEPENDS
     .llvm_libc_common_h
     .llvm-libc-macros.math_macros
+    .llvm-libc-macros.float_macros
     .llvm-libc-types.double_t
     .llvm-libc-types.float_t
     .llvm-libc-types.float128
diff --git a/libc/include/llvm-libc-types/float128.h b/libc/include/llvm-libc-types/float128.h
index 61a094fdb96b12..dedb8076ecf03e 100644
--- a/libc/include/llvm-libc-types/float128.h
+++ b/libc/include/llvm-libc-types/float128.h
@@ -9,8 +9,6 @@
 #ifndef __LLVM_LIBC_TYPES_FLOAT128_H__
 #define __LLVM_LIBC_TYPES_FLOAT128_H__
 
-#include <include/llvm-libc-macros/float-macros.h> // LDBL_MANT_DIG
-
 // Currently, C23 `_Float128` type is only defined as a built-in type in GCC 7
 // or later, and only for C.  For C++, or for clang, `__float128` is defined
 // instead, and only on x86-64 targets.
diff --git a/libc/include/math.h.def b/libc/include/math.h.def
index 927e2d6697c67d..b2252cbb6170d0 100644
--- a/libc/include/math.h.def
+++ b/libc/include/math.h.def
@@ -11,9 +11,9 @@
 
 #include <__llvm-libc-common.h>
 #include <llvm-libc-macros/math-macros.h>
+#include <llvm-libc-macros/float-macros.h>
 #include <llvm-libc-types/float128.h>
 
-
 %%public_api()
 
 #endif // LLVM_LIBC_MATH_H

Copy link
Contributor

@lntue lntue left a comment

Choose a reason for hiding this comment

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

I think we plan (and currently we do) to directly include many of those headers in llvm-libc-macros and llvm-libc-types directly in our internal implementations inside src folder, we should add libc/include in the include paths so that internal inclusions will match with installed headers as #include <llvm-libc-macros/*>. WDYT? @nickdesaulniers @michaelrj-google @gchatelet

@jhuber6
Copy link
Contributor Author

jhuber6 commented Feb 27, 2024

I think we plan (and currently we do) to directly include many of those headers in llvm-libc-macros and llvm-libc-types directly in our internal implementations inside src folder, we should add libc/include in the include paths so that internal inclusions will match with installed headers as #include <llvm-libc-macros/*>. WDYT? @nickdesaulniers @michaelrj-google @gchatelet

We'd end up with include/include/... and you'd need to append include/ to the internal include path, but that would be one option. Generally it's easier just to use the include directory as the root, but it's only difficult in this scenario where we want to include something in the macros from the types.

@@ -9,7 +9,7 @@
#ifndef __LLVM_LIBC_TYPES_FLOAT128_H__
#define __LLVM_LIBC_TYPES_FLOAT128_H__

#include <include/llvm-libc-macros/float-macros.h> // LDBL_MANT_DIG
#include <llvm-libc-macros/float-macros.h> // LDBL_MANT_DIG
Copy link
Member

Choose a reason for hiding this comment

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

This should never have been <>. While you're here, please change this to "".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other headers seem to use <> in stuff like math.h.def.

Copy link
Member

Choose a reason for hiding this comment

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

Check my grep command. libc/include/llvm-libc-types/float128.h is the ONLY place where <> is used to include float-macros.h. Every other include uses "". Consistency is good.

Copy link
Contributor

Choose a reason for hiding this comment

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

all the #includes in /include/ for things in llvm-libc-types or llvm-libc-macros currently use <>. It may be worthwhile to change this, but that should probably be one cleanup patch.

Copy link
Member

Choose a reason for hiding this comment

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

Filed: #83210

@jhuber6 jhuber6 changed the title [libc] Fix installed 'math.h' header paths [libc] Add "include/" to the LLVM include directories Feb 27, 2024
Copy link
Member

@nickdesaulniers nickdesaulniers left a comment

Choose a reason for hiding this comment

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

Assuming the result builds (please wait for presubmit results), this looks like a nice cleanup! Thanks! 👏 🤹

Summary:
Recent changes added an include path in the float128 type that used the
internal `libc` path to find the macro. This doesn't work once it's
installed because we need to search from the root of the install dir.
Move the include to the top-level math.h so we can find it.
@jhuber6
Copy link
Contributor Author

jhuber6 commented Feb 27, 2024

Assuming the result builds (please wait for presubmit results), this looks like a nice cleanup! Thanks! 👏 🤹

I added it in a few more locations, hopefully it fixes the CI.

@jhuber6 jhuber6 merged commit 04e8653 into llvm:main Feb 27, 2024
4 checks passed
metaflow added a commit that referenced this pull request Feb 28, 2024
lntue added a commit to lntue/llvm-project that referenced this pull request Feb 29, 2024
lntue added a commit to lntue/llvm-project that referenced this pull request Feb 29, 2024
lntue added a commit to lntue/llvm-project that referenced this pull request Feb 29, 2024
lntue added a commit to lntue/llvm-project that referenced this pull request Feb 29, 2024
lntue added a commit that referenced this pull request Feb 29, 2024
With some header fix forward for GPU builds.
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.

None yet

5 participants