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] fix includes of src/__support/macros/config.h #86579

Open
nickdesaulniers opened this issue Mar 25, 2024 · 7 comments
Open

[libc] fix includes of src/__support/macros/config.h #86579

nickdesaulniers opened this issue Mar 25, 2024 · 7 comments
Assignees
Labels
code-cleanup good first issue https://github.com/llvm/llvm-project/contribute libc

Comments

@nickdesaulniers
Copy link
Member

A few of these are being cleaned up in #86554 so it's probably best for that to land first. But, I can see latent includes of src/__support/macros/config.h that look wrong, such as the one in libc/src/string/memory_utils/aarch64/inline_memcpy.h which has a comment that it includes that header for the definition of LIBC_INLINE.

LIBC_INLINE is defined in libc/src/__support/macros/attributes.h. As such libc/src/string/memory_utils/aarch64/inline_memcpy.h should include libc/src/__support/macros/attributes.h, not src/__support/macros/config.h and the corresponding cmake+bazel build rules fixed.

Probably there are other places too where a similar pattern is occurring and show be fixed up, too.

I suspect this is a fixup for 29f8e07.

@nickdesaulniers nickdesaulniers added good first issue https://github.com/llvm/llvm-project/contribute code-cleanup libc labels Mar 25, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 25, 2024

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

  1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
  2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
  3. Fix the issue locally.
  4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
  5. Create a Git commit.
  6. Run git clang-format HEAD~1 to format your changes.
  7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 25, 2024

@llvm/issue-subscribers-good-first-issue

Author: Nick Desaulniers (nickdesaulniers)

A few of these are being cleaned up in #86554 so it's probably best for that to land first. But, I can see latent includes of src/__support/macros/config.h that look wrong, such as the one in libc/src/string/memory_utils/aarch64/inline_memcpy.h which has a comment that it includes that header for the definition of LIBC_INLINE.

LIBC_INLINE is defined in libc/src/__support/macros/attributes.h. As such libc/src/string/memory_utils/aarch64/inline_memcpy.h should include libc/src/__support/macros/attributes.h, not src/__support/macros/config.h and the corresponding cmake+bazel build rules fixed.

Probably there are other places too where a similar pattern is occurring and show be fixed up, too.

I suspect this is a fixup for 29f8e07.

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 25, 2024

@llvm/issue-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

A few of these are being cleaned up in #86554 so it's probably best for that to land first. But, I can see latent includes of src/__support/macros/config.h that look wrong, such as the one in libc/src/string/memory_utils/aarch64/inline_memcpy.h which has a comment that it includes that header for the definition of LIBC_INLINE.

LIBC_INLINE is defined in libc/src/__support/macros/attributes.h. As such libc/src/string/memory_utils/aarch64/inline_memcpy.h should include libc/src/__support/macros/attributes.h, not src/__support/macros/config.h and the corresponding cmake+bazel build rules fixed.

Probably there are other places too where a similar pattern is occurring and show be fixed up, too.

I suspect this is a fixup for 29f8e07.

@divyaxdv
Copy link

divyaxdv commented Mar 25, 2024

@nickdesaulniers I would like to work on this issue!
If this is available

@nickdesaulniers
Copy link
Member Author

Thanks @divyaxdv , all yours. You may want to subscribe to #86554 and watch/wait for that to land first.

@divyaxdv
Copy link

Hello @nickdesaulniers I tried looking into this and seems like you did few cleanup. Do we need more similar cleanups. I am not sure because I am facing issues while running the bazel test. Initially I tried running the same commands

cd utils/bazel/llvm-project-overlay

bazel query @llvm-project//libc/... | xargs bazel test --config=generic_clang --test_output=errors --test_tag_filters=-nobuildkite --build_tag_filters=-nobuildkite --@llvm-project//libc:mpfr=system

but I am getting this error while running this command https://pastebin.com/NHQA677f.

Then I tried running the bazel test using the command bazel test --config=generic_clang @llvm-project//libc/... from the bazel directory instead of the llvm-project-overlay directory but still facing issues which I have asked in the libc channel discord. It will be helpful if you can provide some direction. Here is the link to the question asked in the discord.

https://discord.com/channels/636084430946959380/636732994891284500/1224100594244124742

@nickdesaulniers
Copy link
Member Author

nickdesaulniers commented Apr 1, 2024

From discord, it sounds like you're on macos? If so, something has likely bitrot in the bazel build for ensuring we don't build linux files on non-linux hosts.

EDIT: if that's the case, then just stick with the cmake based build (unless that has bitrot as well).

@nickdesaulniers
Copy link
Member Author

Do we need more similar cleanups.

Yes, these are still necessary.

@divyaxdv
Copy link

divyaxdv commented Apr 2, 2024

I am getting this error when running test using the ninja check-libc
https://pastebin.com/rTWfvN94
I am new to the llvm. if you can provide some direction for solving this issue then it will helpful.

@nickdesaulniers
Copy link
Member Author

Yeah, the issue is that the build has bitrot on Darwin (guessing you're on an aarch64 based Mac?). I broke this recently in #85955 when I deleted libc/src/__support/OSUtil/darwin/quick_exit.h rather than convert it to libc/src/__support/OSUtil/darwin/quick_exit.cpp (as was done for libc/src/__support/OSUtil/baremetal/quick_exit.cpp as an example).

Currently, all first party developers are developing on Linux hosts; we could use assistance in getting the MacOS build back up to snuff. If you'd like to help with that, it may be a better place to start than this bug. We can discuss the details more in a new (not-yet-created) issue, or on discord.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-cleanup good first issue https://github.com/llvm/llvm-project/contribute libc
Projects
None yet
Development

No branches or pull requests

3 participants