Skip to content

[libc] add stdlib.h header to the _Exit func proxy in full build #114718

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

Merged
1 commit merged into from Nov 3, 2024
Merged

[libc] add stdlib.h header to the _Exit func proxy in full build #114718

1 commit merged into from Nov 3, 2024

Conversation

ghost
Copy link

@ghost ghost commented Nov 3, 2024

No description provided.

@llvmbot llvmbot added the libc label Nov 3, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 3, 2024

@llvm/pr-subscribers-libc

Author: Job Henandez Lara (Jobhdez)

Changes

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

1 Files Affected:

  • (modified) libc/hdr/func/_Exit.h (+2-1)
diff --git a/libc/hdr/func/_Exit.h b/libc/hdr/func/_Exit.h
index 575b0426e508c6..e024a651a50bcf 100644
--- a/libc/hdr/func/_Exit.h
+++ b/libc/hdr/func/_Exit.h
@@ -10,7 +10,8 @@
 #define LLVM_LIBC_HDR_FUNC_EXIT_H
 
 #ifdef LIBC_FULL_BUILD
-extern "C" void _Exit(int);
+// We will use the `_Exit` declaration from our generated stdlib.h
+#include <stdlib.h>
 
 #else // Overlay mode
 

@ghost ghost merged commit 2afc562 into llvm:main Nov 3, 2024
9 checks passed
@nickdesaulniers
Copy link
Member

Do you have more info about this change? Why not just provide the declaration here?

@ghost
Copy link
Author

ghost commented Nov 4, 2024

Do you have more info about this change? Why not just provide the declaration here?

the previous declaration was conflicted with the one generated in our stdlib.h:

/home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-gcc-fullbuild-dbg/build/projects/libc/include/stdlib.h:101:16: error: function ‘void _Exit(int)’ declared ‘[[noreturn]]’ but its first declaration was not
  101 | _Noreturn void _Exit(int) __NOEXCEPT;

Should we remove __NOEXCEPT in our _Exit, because it is annotated as THROW in glibc header?

@nickdesaulniers
Copy link
Member

nickdesaulniers commented Nov 4, 2024

Should we remove __NOEXCEPT in our _Exit

I would expect exit to be noecept, since exit may execute callbacks provided by the user via atexit or on_exit calls. If those callbacks throw when invoked via a call to exit, then it will appear that the call to exit threw. But _Exit is supposed to skip those callbacks.

So it's not clear to my why glibc does not mark it's _Exit as noexcept.

@nickdesaulniers
Copy link
Member

extern void _Exit (int __status) __THROW __attribute__ ((__noreturn__));

is what my glibc host's headers have. __THROW which expands to:

#  define __THROW       __attribute__ ((__nothrow__ __LEAF))

so glibc's _Exit is nothrow.


regardless, the warning is about noreturn, not nothrow. The question isn't about throw/nothrow, it's that our declaration was missing noreturn. If we redeclare a function, it needs to keep noreturn once declared with that fn attr.

@ghost
Copy link
Author

ghost commented Nov 4, 2024

extern void _Exit (int __status) __THROW __attribute__ ((__noreturn__));

is what my glibc host's headers have. __THROW which expands to:

#  define __THROW       __attribute__ ((__nothrow__ __LEAF))

so glibc's _Exit is nothrow.

regardless, the warning is about noreturn, not nothrow. The question isn't about throw/nothrow, it's that our declaration was missing noreturn. If we redeclare a function, it needs to keep noreturn once declared with that fn attr.

So do you want me to do a quick pr and add this to the proxy

[[noreturn]] void _Exit() noexcept;

Can you please confirm if this declaration is the right one. Thanks

@nickdesaulniers
Copy link
Member

So do you want me to do a quick pr and add this to the proxy

[[noreturn]] void _Exit() noexcept;

Can you please confirm if this declaration is the right one. Thanks

don't forget the int parameter!

[[noreturn]] void _Exit(int) noexcept;

but yeah, I'd approve such a PR.

@ghost
Copy link
Author

ghost commented Nov 5, 2024

So do you want me to do a quick pr and add this to the proxy
[[noreturn]] void _Exit() noexcept;
Can you please confirm if this declaration is the right one. Thanks

don't forget the int parameter!

[[noreturn]] void _Exit(int) noexcept;

but yeah, I'd approve such a PR.

Ok will do this now

PhilippRados pushed a commit to PhilippRados/llvm-project that referenced this pull request Nov 6, 2024
ghost pushed a commit that referenced this pull request Nov 7, 2024
…n tests (#114904)

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`
bherrera pushed a commit to misttech/integration that referenced this pull request Nov 12, 2024
…nd use LIBC_NAMESPACE::_Exit in tests (#114904)

This improves/fixes this pr
llvm/llvm-project#114718. In this PR we removed
the _Exit proxy func because it was not needed. Instead we used
`LIBC_NAMESPACE::_Exit`

GitOrigin-RevId: e71903630a42d9d22f68634f2f833cfbc4c82572
Original-Revision: 7a1d7e7fcd4655b6f2b7efe120e442734a8d20bd
Change-Id: I30d44087e7ed9f28ccbb61b799854bbf2c5a2dbc
bherrera pushed a commit to misttech/integration that referenced this pull request Nov 12, 2024
…ve _Exit proxy func header and use LIBC_NAMESPACE::_Exit in tests (#114904)

This improves/fixes this pr
llvm/llvm-project#114718. In this PR we removed
the _Exit proxy func because it was not needed. Instead we used
`LIBC_NAMESPACE::_Exit`

GitOrigin-RevId: 0b99129c6f14da2553ea6702291f56ef858a1aac
Original-Revision: 7a1d7e7fcd4655b6f2b7efe120e442734a8d20bd
Original-Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1153254
Original-Revision: 03a718a6745a6fcd6aed461d69e6f1eaf4a04740
Change-Id: Ia6da8c64fd00a2a008200142f80a69cb9bd6a672
bherrera pushed a commit to misttech/mist-os that referenced this pull request Nov 12, 2024
…header and use LIBC_NAMESPACE::_Exit in tests (#114904)

This improves/fixes this pr
llvm/llvm-project#114718. In this PR we removed
the _Exit proxy func because it was not needed. Instead we used
`LIBC_NAMESPACE::_Exit`

GitOrigin-RevId: e71903630a42d9d22f68634f2f833cfbc4c82572
Original-Revision: 7a1d7e7fcd4655b6f2b7efe120e442734a8d20bd
Roller-URL: https://cr-buildbucket.appspot.com/build/8731899817509253313
CQ-Do-Not-Cancel-Tryjobs: true
Change-Id: I91646b35e697eafebc5c89d54c09cf4adcf70342
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1153254
Groverkss pushed a commit to iree-org/llvm-project that referenced this pull request Nov 15, 2024
…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`
This pull request was closed.
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.

3 participants