-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[libc] Remove _Exit proxy func header and use LIBC_NAMESPACE::_Exit in tests #114904
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
Conversation
@llvm/pr-subscribers-libc Author: Job Henandez Lara (Jobhdez) ChangesThis improves/fixes this pr #114718. In this pr we added an _Exit declaration to Full diff: https://github.com/llvm/llvm-project/pull/114904.diff 1 Files Affected:
diff --git a/libc/hdr/func/_Exit.h b/libc/hdr/func/_Exit.h
index e024a651a50bcf..b8160e9af3e4e3 100644
--- a/libc/hdr/func/_Exit.h
+++ b/libc/hdr/func/_Exit.h
@@ -10,8 +10,7 @@
#define LLVM_LIBC_HDR_FUNC_EXIT_H
#ifdef LIBC_FULL_BUILD
-// We will use the `_Exit` declaration from our generated stdlib.h
-#include <stdlib.h>
+extern "C" [[noreturn]] void _Exit(int) noexcept;
#else // Overlay mode
|
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.
Why do we need a proxy header for _Exit
at all? Why don't we just call our internal _Exit
in the atexit
and atquickexit
tests?
Yeah this makes sense. I will update the PR to remove the |
Yes that sounds like the correct thing to do. Thank you! |
No problem! |
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, please update the title and description
ok I will. do you want me to merge after I change the title? or do you want to merge yourself? |
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.
This should be fine to merge as-is. I'd recommend waiting until the morning to merge it (so that people can help troubleshoot if necessary), but it's up to you.
ok ill wait until tomorrow morning. thanks. can you please suggest a new title for the pr? thanks |
You can change it to |
ok sounds good. I'm going to merge now! |
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/171/builds/9876 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/71/builds/10064 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/131/builds/9925 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/179/builds/9800 Here is the relevant piece of the build log for the reference
|
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.
Tests are failing, I think I figured out why:
@@ -6,7 +6,6 @@ | |||
// | |||
//===----------------------------------------------------------------------===// | |||
|
|||
#include "hdr/func/_Exit.h" |
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.
looks like you need to include _Exit
here explicitly (#include "src/stdlib/_Exit.h
). Applies to both tests.
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.
ok will fix it right now
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.
im working on it now. its building in full build mode
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/196/builds/680 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/183/builds/5946 Here is the relevant piece of the build log for the reference
|
…n tests (llvm#114904) This improves/fixes this pr llvm#114718. In this PR we removed the _Exit proxy func because it was not needed. Instead we used `LIBC_NAMESPACE::_Exit`
…` and `atexit_test.cpp` (llvm#115351) Hello, I merged this llvm#114904 a few mins ago and the tests failed because i did not add the header `src/stdlib/_Exit.h` in `at_quick_exit_test.cpp` and `atexit_test.cpp`. I ran both builds/tests and everything was good. thanks
This improves/fixes this pr #114718. In this PR we removed the _Exit proxy func because it was not needed. Instead we used
LIBC_NAMESPACE::_Exit