[libc] Move preinit/init/fini arrays to namespace #158746
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In change llvm#146863 we moved definitions of preinit/init/fini arrays to header but unintentionally moved outside of the namespace. Since the namespace also controls the visibility (through LIBC_NAMESPACE_DECL), as a consequence these symbols no longer have the hidden visibility which changes the codegen from: ``` 4: 4c11 ldr r4, [pc, #0x44] @ 0x4c <__libc_init_array+0x4c> 6: 4812 ldr r0, [pc, #0x48] @ 0x50 <__libc_init_array+0x50> 8: 447c add r4, pc a: 4478 add r0, pc c: 1b00 subs r0, r0, r4 ``` to: ``` 4: 4813 ldr r0, [pc, #0x4c] @ 0x54 <__libc_init_array+0x54> 6: 4914 ldr r1, [pc, #0x50] @ 0x58 <__libc_init_array+0x58> 8: 4478 add r0, pc a: 4479 add r1, pc c: 6804 ldr r4, [r0] e: 6808 ldr r0, [r1] 10: 1b00 subs r0, r0, r4 ``` The `ldr` will trigger a fault in case where these symbols aren't pointing to a valid memory location which is sometimes the case when the array is empty.
petrhosek
requested review from
Prabhuk,
frobtech,
michaelrj-google and
saturn691
|
@llvm/pr-subscribers-libc Author: Petr Hosek (petrhosek) ChangesIn change #146863 we moved definitions of preinit/init/fini arrays to header but unintentionally moved outside of the namespace. Since the namespace also controls the visibility (through LIBC_NAMESPACE_DECL), as a consequence these symbols no longer have the hidden visibility which changes the codegen from: to: The Full diff: https://github.com/llvm/llvm-project/pull/158746.diff 2 Files Affected:
diff --git a/libc/startup/baremetal/fini.h b/libc/startup/baremetal/fini.h
index 74e9601983a33..681cbf954f94a 100644
--- a/libc/startup/baremetal/fini.h
+++ b/libc/startup/baremetal/fini.h
@@ -7,6 +7,9 @@
//===----------------------------------------------------------------------===//
#include "hdr/stdint_proxy.h"
+#include "src/__support/macros/config.h"
+
+namespace LIBC_NAMESPACE_DECL {
extern "C" {
extern uintptr_t __fini_array_start[];
@@ -14,3 +17,6 @@ extern uintptr_t __fini_array_end[];
void __libc_fini_array(void);
} // extern "C"
+
+
+} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/startup/baremetal/init.h b/libc/startup/baremetal/init.h
index 6b545db3976da..da20c7766fee6 100644
--- a/libc/startup/baremetal/init.h
+++ b/libc/startup/baremetal/init.h
@@ -7,6 +7,9 @@
//===----------------------------------------------------------------------===//
#include "hdr/stdint_proxy.h"
+#include "src/__support/macros/config.h"
+
+namespace LIBC_NAMESPACE_DECL {
extern "C" {
extern uintptr_t __preinit_array_start[];
@@ -16,3 +19,5 @@ extern uintptr_t __init_array_end[];
void __libc_init_array(void);
} // extern "C"
+
+} // namespace LIBC_NAMESPACE_DECL
|
jhuber6
approved these changes
Sep 15, 2025
lntue
approved these changes
Sep 15, 2025
michaelrj-google
approved these changes
Sep 15, 2025
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| #include "hdr/stdint_proxy.h" | ||
| #include "src/__support/macros/config.h" | ||
|
|
||
| namespace LIBC_NAMESPACE_DECL { |
There was a problem hiding this comment.
Given that the only declarations inside this namespace are extern "C", it would be easy to overlook this namespace as being unnecessary for correctness -- especially if you don't realize the gnu::visibility("hidden") magic behind it. This probably facilitated the bug being introduced in #146863.
Perhaps a comment here would be warranted (and same on init.h of course).
Prabhuk
approved these changes
Sep 15, 2025
|
Good catch! |
kimsh02
pushed a commit
to kimsh02/llvm-project
that referenced
this pull request
Sep 19, 2025
In change llvm#146863 we moved definitions of preinit/init/fini arrays to header but unintentionally moved outside of the namespace. Since the namespace also controls the visibility (through LIBC_NAMESPACE_DECL), as a consequence these symbols no longer have the hidden visibility which changes the codegen from: ``` 4: 4c11 ldr r4, [pc, #0x44] @ 0x4c <__libc_init_array+0x4c> 6: 4812 ldr r0, [pc, #0x48] @ 0x50 <__libc_init_array+0x50> 8: 447c add r4, pc a: 4478 add r0, pc c: 1b00 subs r0, r0, r4 ``` to: ``` 4: 4813 ldr r0, [pc, #0x4c] @ 0x54 <__libc_init_array+0x54> 6: 4914 ldr r1, [pc, #0x50] @ 0x58 <__libc_init_array+0x58> 8: 4478 add r0, pc a: 4479 add r1, pc c: 6804 ldr r4, [r0] e: 6808 ldr r0, [r1] 10: 1b00 subs r0, r0, r4 ``` The `ldr` will trigger a fault in case where these symbols aren't pointing to a valid memory location which is sometimes the case when the array is empty.
itzexpoexpo
pushed a commit
to itzexpoexpo/llvm-project
that referenced
this pull request
Sep 21, 2025
In change llvm#146863 we moved definitions of preinit/init/fini arrays to header but unintentionally moved outside of the namespace. Since the namespace also controls the visibility (through LIBC_NAMESPACE_DECL), as a consequence these symbols no longer have the hidden visibility which changes the codegen from: ``` 4: 4c11 ldr r4, [pc, #0x44] @ 0x4c <__libc_init_array+0x4c> 6: 4812 ldr r0, [pc, #0x48] @ 0x50 <__libc_init_array+0x50> 8: 447c add r4, pc a: 4478 add r0, pc c: 1b00 subs r0, r0, r4 ``` to: ``` 4: 4813 ldr r0, [pc, #0x4c] @ 0x54 <__libc_init_array+0x54> 6: 4914 ldr r1, [pc, #0x50] @ 0x58 <__libc_init_array+0x58> 8: 4478 add r0, pc a: 4479 add r1, pc c: 6804 ldr r4, [r0] e: 6808 ldr r0, [r1] 10: 1b00 subs r0, r0, r4 ``` The `ldr` will trigger a fault in case where these symbols aren't pointing to a valid memory location which is sometimes the case when the array is empty.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In change #146863 we moved definitions of preinit/init/fini arrays to header but unintentionally moved outside of the namespace. Since the namespace also controls the visibility (through LIBC_NAMESPACE_DECL), as a consequence these symbols no longer have the hidden visibility which changes the codegen from:
to:
The
ldrwill trigger a fault in case where these symbols aren't pointing to a valid memory location which is sometimes the case when the array is empty.