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.

Sign up for GitHub

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

Jump to bottom

kernel: Unify and simplify constructor support #74700

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+95 −177
Open

kernel: Unify and simplify constructor support #74700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91c964e
kernel: Unify .ctors and .init_array handling
keith-packard Jun 21, 2024
7095dba
tests/kernel: Validate constructor execution order
keith-packard Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 1 addition & 10 deletions include/zephyr/arch/arc/v2/linker.ld
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,6 @@ SECTIONS {
#include <snippets-rodata.ld>
#include <zephyr/linker/kobject-rom.ld>

#if defined(CONFIG_CPP) && !defined(CONFIG_STATIC_INIT_GNU) && defined(__MWDT_LINKER_CMD__)
Copy link
Member

Choose a reason for hiding this comment

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

I would really like to have the arc guys (@evgeniy-paltsev @ruuddw ) confirm things keep working fine for this toolchain. I'm inclined to set a "dnm" until they do.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, my first intuition is that this should remain for MetaWare - but need to check with our toolchain or other people that know more about this. I expect the _fctors/_ectors are required for the MetaWare runtime library to handle the constructors/destructors. Not sure that can be replaced by other Zephyr initialization code without other consequences/side effects.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, I think that's the best plan -- I think this should work correctly even on MetaWare as we're completely replacing the whole constructor path with Zephyr-specific code, effectively bypassing the MetaWare runtime. But, as you say, we need to have the arc people review this before merging.

. = ALIGN(4);
_fctors = .;
KEEP(*(.ctors*))
_ectors = .;
#endif /* CONFIG_CPP && !CONFIG_STATIC_INIT_GNU && __MWDT_LINKER_CMD__ */

/* This extra MPU alignment of RAMABLE_REGION is only required if we put ROMABLE_REGION and
* RAMABLE_REGION into the same (continuous) memory - otherwise we can get beginning of the
* RAMABLE_REGION in the end of ROMABLE_REGION MPU aperture.
Expand Down Expand Up @@ -311,11 +304,9 @@ SECTIONS {
#endif

/DISCARD/ : {
#if defined(CONFIG_CPP) && !defined(CONFIG_STATIC_INIT_GNU) && defined(__MWDT_LINKER_CMD__)
/* Discard all destructors */
*(.dtors*)
*(.fini*)
*(.eh_frame*)
#endif /* CONFIG_CPP && !CONFIG_STATIC_INIT_GNU && __MWDT_LINKER_CMD__ */
*(.note.GNU-stack)
*(.got.plt)
*(.igot.plt)
Expand Down
101 changes: 44 additions & 57 deletions include/zephyr/linker/common-rom/common-rom-init.ld
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,76 +1,63 @@
/* SPDX-License-Identifier: Apache-2.0 */

#ifdef CONFIG_STATIC_INIT_GNU
Copy link
Member

Choose a reason for hiding this comment

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

(Continuing with the previous comment)
I think we should leave the conditions to choose this this as they are based on CONFIG_STATIC_INIT_GNU (together with the check)

diff --git a/Kconfig.zephyr b/Kconfig.zephyr
index 9f3e937f849..0e5c665b21d 100644
--- a/Kconfig.zephyr
+++ b/Kconfig.zephyr
@@ -434,7 +434,6 @@ config CODING_GUIDELINE_CHECK
 config NATIVE_LIBC
        bool
        select FULL_LIBC_SUPPORTED
-       select STATIC_INIT_GNU
        help
          Zephyr will use the host system C library.
 
diff --git a/include/zephyr/linker/common-rom/common-rom-init.ld b/include/zephyr/linker/common-rom/common-rom-init.ld
index 5523882a109..80587af1d1b 100644
--- a/include/zephyr/linker/common-rom/common-rom-init.ld
+++ b/include/zephyr/linker/common-rom/common-rom-init.ld
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: Apache-2.0 */
 
-#ifdef CONFIG_TOOLCHAIN_SUPPORTS_STATIC_INIT_GNU
+#ifdef CONFIG_STATIC_INIT_GNU
        SECTION_PROLOGUE(init_array,,)
        {
                /*
@@ -14,36 +14,19 @@
                        SORT_BY_INIT_PRIORITY(.ctors.*)))
                KEEP (*(.init_array .ctors))
                __zephyr_init_array_end = .;
-#ifdef CONFIG_NATIVE_LIBC
-               /*
-                 * The __CTOR_LIST__ and __CTOR_END__ symbols are always defined
-                 * to result in an empty list. This is necessary to fix an issue
-                 * where the glibc process initialization code on native_posix
-                 * platforms calls constructors before Zephyr loads (issue #39347).
-                */
-               __CTOR_LIST__ = .;
-#ifdef CONFIG_64BIT
-               QUAD(0)
-               QUAD(0)
-#else
-               LONG(0)
-               LONG(0)
-#endif
-               __CTOR_END__ = .;
-#endif
-               /*
-               * Similar to the schenanigans required for the __CTOR_LIST__ and
-               * __CTOR_END__ symbols we define __init_array_start and __init_array_end
-               * to the same address to define an empty list. This prevents the glibc
-               * startup code from calling any global constructors before Zephyr loads.
-               */
-               __init_array_start = .;
-               __init_array_end = .;
        } GROUP_ROM_LINK_IN(RAMABLE_REGION, ROMABLE_REGION)
 
-#ifndef CONFIG_STATIC_INIT_GNU
-       ASSERT(__zephyr_init_array_start == __zephyr_init_array_end,
-              "GNU-style constructors required but STATIC_INIT_GNU not enabled")
-#endif
+#elif defined(CONFIG_TOOLCHAIN_SUPPORTS_STATIC_INIT_GNU) && !defined(CONFIG_NATIVE_APPLICATION)
+       /*
+        * If the code to invoke constructors is not enabled,
+        * make sure there aren't any in the application
+        */
+       SECTION_PROLOGUE(init_array,,)
+       {
+               KEEP(*(SORT_BY_NAME(".ctors*")))
+               KEEP(*(SORT_BY_NAME(".init_array*")))
+       } GROUP_ROM_LINK_IN(RAMABLE_REGION, ROMABLE_REGION)
 
+       ASSERT (SIZEOF(init_array) == 0,
+               "GNU-style constructors required but STATIC_INIT_GNU not enabled")
 #endif

Note I don't think we need anymore the code that creates the empty lists for native_posix
as with the current code I don't think we have anymore those problems (#39347 #36858)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I need to switch from the Zephyr names to native C library names and then always include constructors when NATIVE_LIBC is enabled. That seems pretty simple.

Copy link
Member

Choose a reason for hiding this comment

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

It sounds like we need a new mode which explicitly constructs a glibc-compatible constructor array, and then only use that when NATIVE_LIBC is enabled.

I don't think we need to. Why do you say that?
AFAIR that was the reason why we have had that bug with the double run of the C++ constructors long ago. I think the original direction you had in this PR was fine, except for the condition when it was enabled.

I think I need to switch from the Zephyr names to native C library names and then always include constructors when NATIVE_LIBC is enabled.

If we do that, won't we be reintroducing #75407 or the counter bug?

A bit more background:

When we build for native simulator targets (NATIVE_LIBRARY), the Zephyr/embedded SW side of things goes first thru a partial linking on its own without the host C library. There we create Zephyr's constructor list which we will go thru at Zephyr init.
(Almost) All embedded symbols are then localized so later linker stages will not mistakenly grab them.
In a later pass, the native_simulator runner itself will be linked together with the host C library, and with whatever other host libraries. That will end up with its own separate Linux compliant constructor list which will be executed during the zephyr.exe init, and which is not a concern for Zephyr. i.e. We end up with one constructor list for the host, executed in the runner initialization context; and another for Zephyr, executed by Zephyr in Zephyr context.

For native_posix like targets (NATIVE_APPLICATION), we do the linking of Zephyr, with the "runner", with the host C library and any other host libraries, in one step. Here the Zephyr linker script expands the host linker script (so a Linux compliant constructor list will be built by the host linker script by default)
In this case, because we have a single link stage, and the linker sees all constructors at the same time, both of host libraries and embedded ones, we have 2 options
a) Do not create our own Zephyr constructor list, and do not have Zephyr run them (just let the host linker script build it and the executable initialization run thru it), or
b) Create a constructor list which Zephyr runs, and ensure the one for the host does not end up with a list of duplicates.
Neither is specially good, as in some cases you need to run the constructors at the executable init because a host library needs it, and in other cases you may need to run it at Zephyr init, because the constructor is from the embedded code and requires enough of Zephyr to be up.
Today in main, by default, we do a) unless CONFIG_CPP=y. So we ensure we don't break host C libraries constructors (so we don't have, say, this bug #75407). For CONFIG_CPP we kept b) just because it is what it was done in that case. In most cases, it will be fine to do a) even if one has embedded C++ constructors, and if not the user can set CONFIG_STATIC_INIT_GNU as they need.

That all being said as native_posix and NATIVE_APPLICATION is being deprecated, I think we can just keep that behavior for NATIVE_APPLICATION by now.

And for NATIVE_LIBRARY we can just behave exactly like for embedded targets. No need to treat it differently even if the embedded code will be eventually linked with the host C library.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, so the only place we need to worry about mixing native and Zephyr initializers is with NATIVE_APPLICATION? And that's because there's only a single linker stage? I assume Zephyr is still generating the linker script for that? If so, will that script need to generate a native-compatible initializer list? Which format should it use? Presumably "both", using the old style (count + inits + null) for .ctors and the new style (inits) for .init_array?

If all of my assumptions are correct, then the current patch is nearly correct; it just needs to have the selection between Zephyr and Native intializers corrected so that Native initializers are used with NATIVE_APPLICATION and otherwise we always get Zephyr initializers?

In any case, we'll still need help from someone who understands MetaWare to make sure we can use the same initializers as all other toolchains.

Copy link
Member

Choose a reason for hiding this comment

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

ok, so the only place we need to worry about mixing native and Zephyr initializers is with NATIVE_APPLICATION? And that's because there's only a single linker stage?

yep & yep

I assume Zephyr is still generating the linker script for that?

Partly. In this case Zephyr's linker script is an extension to the default host linker script (see below).

If so, will that script need to generate a native-compatible initializer list?

Not necessarily. If for that case we do nothing with .ctors & .init_array's, the host linker script will still generate a fully correct list for the host executable init.
In this NATIVE_APPLICATION case we only need to generate this constructors list if we want to have "our own" to be run in Zephyr's init, wrapped in our own __zephyr_init_array_start/end, instead of the host one.
When we generate our own .ctors list, the host one generated by the host linker script will be empty (to be precise, it will not have whatever we have already picked into our list).

Meaning, for NATIVE_APPLICATION, if we do nothing for .ctors in the Zephyr linked script, and do not build in the constructor initialization, the host linker script will still build a correct list, and during the executable init it will be run thru.

Just for reference, this is the relevant part of the host linker script in my case:

.preinit_array    :
  {
    PROVIDE_HIDDEN (__preinit_array_start = .);
    KEEP (*(.preinit_array))
    PROVIDE_HIDDEN (__preinit_array_end = .);
  }
  .init_array    :
  {
    PROVIDE_HIDDEN (__init_array_start = .);
    KEEP (*(SORT_BY_INIT_PRIORITY(.init_array.*) SORT_BY_INIT_PRIORITY(.ctors.*)))
    KEEP (*(.init_array EXCLUDE_FILE (*crtbegin.o *crtbegin?.o *crtend.o *crtend?.o ) .ctors))
    PROVIDE_HIDDEN (__init_array_end = .);
  }
  .fini_array    :
  {
    PROVIDE_HIDDEN (__fini_array_start = .);
    KEEP (*(SORT_BY_INIT_PRIORITY(.fini_array.*) SORT_BY_INIT_PRIORITY(.dtors.*)))
    KEEP (*(.fini_array EXCLUDE_FILE (*crtbegin.o *crtbegin?.o *crtend.o *crtend?.o ) .dtors))
    PROVIDE_HIDDEN (__fini_array_end = .);
  }
  .ctors          :
  {
    /* gcc uses crtbegin.o to find the start of
       the constructors, so we make sure it is
       first.  Because this is a wildcard, it
       doesn't matter if the user does not
       actually link against crtbegin.o; the
       linker won't look for a file to match a
       wildcard.  The wildcard also means that it
       doesn't matter which directory crtbegin.o
       is in.  */
    KEEP (*crtbegin.o(.ctors))
    KEEP (*crtbegin?.o(.ctors))
    /* We don't want to include the .ctor section from
       the crtend.o file until after the sorted ctors.
       The .ctor section from the crtend file contains the
       end of ctors marker and it must be last */
    KEEP (*(EXCLUDE_FILE (*crtend.o *crtend?.o ) .ctors))
    KEEP (*(SORT(.ctors.*)))
    KEEP (*(.ctors))
  }
  .dtors          :
  {
    KEEP (*crtbegin.o(.dtors))
    KEEP (*crtbegin?.o(.dtors))
    KEEP (*(EXCLUDE_FILE (*crtend.o *crtend?.o ) .dtors))
    KEEP (*(SORT(.dtors.*)))
    KEEP (*(.dtors))
  }

Copy link
Member

Choose a reason for hiding this comment

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

In any case, we'll still need help from someone who understands MetaWare to make sure we can use the same initializers as all other toolchains.

@evgeniy-paltsev is doing some tests to see if the PR works or not. But even if it works 'today', it may break later if MetaWare changes internal structures and/or C/C++ runtime libs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's unclear how MetaWare could even break things -- none of the Zephyr initializer code is something the toolchain has influence in, except for the ELF sections assigned to the initializer addresses (currently either .init_array or .ctors).

In any case, the tests included in this PR should catch any toolchain changes in how initializers are handled by the compiler, so if they work today (and the tests pass), we can be reasonably confident that future changes will not pass unnoticed and Zephyr can adapt as needed. Which isn't really any different from handling toolchain changes anywhere else in the system.

SECTION_PROLOGUE(_CTOR_SECTION_NAME,,)
#if defined(CONFIG_TOOLCHAIN_SUPPORTS_STATIC_INIT_GNU) || defined(CONFIG_NATIVE_APPLICATION)
SECTION_PROLOGUE(init_array,,)
{
/*
* The compiler fills the constructor pointers table below,
* hence symbol __CTOR_LIST__ must be aligned on word
* boundary. To align with the C++ standard, the first element
* of the array contains the number of actual constructors. The
* last element is NULL.
*
* The __CTOR_LIST__ and __CTOR_END__ symbols are always defined
* to result in an empty list. This is necessary to fix an issue
* where the glibc process initialization code on native_posix
* platforms calls constructors before Zephyr loads (issue #39347).
*
* Zephyr's start-up code uses the __ZEPHYR_CTOR_LIST__ and
* __ZEHPYR_CTOR_END__ symbols, so these need to be correctly set.
* Add all of the GNU-style constructors in priority order. Note
* that this doesn't build the ctors in the "usual" fashion with
* a length value first and NULL terminator, but we're creating
* an init_array style list and leaving the ctors list empty
*/
#ifdef CONFIG_NATIVE_APPLICATION
/* Use the native LIBC constructor code so that any native
* constructors get run before main is invoked
*/
__init_array_start = .;
#else
__zephyr_init_array_start = .;
#endif
KEEP (*(SORT_BY_INIT_PRIORITY(.init_array.*)
SORT_BY_INIT_PRIORITY(.ctors.*)))
KEEP (*(.init_array .ctors))
#ifdef CONFIG_NATIVE_APPLICATION
__init_array_end = .;
#else
__zephyr_init_array_end = .;
#endif

#ifdef CONFIG_NATIVE_LIBC
/*
* The __CTOR_LIST__ and __CTOR_END__ symbols are always defined
* to result in an empty list. This is necessary to fix an issue
* where the glibc process initialization code on native_posix
* platforms calls constructors before Zephyr loads (issue #39347).
*/
#ifdef CONFIG_64BIT
. = ALIGN(8);
__ZEPHYR_CTOR_LIST__ = .;
QUAD((__ZEPHYR_CTOR_END__ - __ZEPHYR_CTOR_LIST__) / 8 - 2)
KEEP(*(SORT_BY_NAME(".ctors*")))
__CTOR_LIST__ = .;
#ifdef CONFIG_64BIT
QUAD(0)
__ZEPHYR_CTOR_END__ = .;
QUAD(0)
__CTOR_END__ = .;
#else
. = ALIGN(4);
__ZEPHYR_CTOR_LIST__ = .;
LONG((__ZEPHYR_CTOR_END__ - __ZEPHYR_CTOR_LIST__) / 4 - 2)
KEEP(*(SORT_BY_NAME(".ctors*")))
__CTOR_LIST__ = .;
LONG(0)
__ZEPHYR_CTOR_END__ = .;
LONG(0)
__CTOR_END__ = .;
#endif
} GROUP_ROM_LINK_IN(RAMABLE_REGION, ROMABLE_REGION)

SECTION_PROLOGUE(init_array,,)
{
__CTOR_END__ = .;
#ifndef CONFIG_NATIVE_APPLICATION
/*
* Similar to the schenanigans required for the __CTOR_LIST__ and
* __CTOR_END__ symbols we define __init_array_start and __init_array_end
* to the same address to define an empty list. This prevents the glibc
* startup code from calling any global constructors before Zephyr loads.
*
* Zephyr's start-up code uses the __zephyr_init_array_start and
* __zephyr_init_array_end sybmols, so these need to be set correctly.
*/
. = ALIGN(4);
* Similar to the schenanigans required for the __CTOR_LIST__ and
* __CTOR_END__ symbols we define __init_array_start and __init_array_end
* to the same address to define an empty list. This prevents the glibc
* startup code from calling any global constructors before Zephyr loads.
*/
__init_array_start = .;
__init_array_end = .;
__zephyr_init_array_start = .;
KEEP(*(SORT_BY_NAME(".init_array*")))
__zephyr_init_array_end = .;
#endif
#endif
} GROUP_ROM_LINK_IN(RAMABLE_REGION, ROMABLE_REGION)

#elif defined(CONFIG_TOOLCHAIN_SUPPORTS_STATIC_INIT_GNU) && !defined(CONFIG_NATIVE_APPLICATION)
/*
* If the code to invoke constructors is not enabled,
* make sure there aren't any in the application
*/
SECTION_PROLOGUE(init_array,,)
{
KEEP(*(SORT_BY_NAME(".ctors*")))
KEEP(*(SORT_BY_NAME(".init_array*")))
} GROUP_ROM_LINK_IN(RAMABLE_REGION, ROMABLE_REGION)
#if !defined(CONFIG_STATIC_INIT_GNU) && !defined(CONFIG_NATIVE_APPLICATION)
ASSERT(__zephyr_init_array_start == __zephyr_init_array_end,
"GNU-style constructors required but STATIC_INIT_GNU not enabled")
#endif

ASSERT (SIZEOF(init_array) == 0,
"GNU-style constructors required but STATIC_INIT_GNU not enabled")
#endif
1 change: 0 additions & 1 deletion kernel/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ list(APPEND kernel_files
errno.c
fatal.c
init.c
init_static.c
kheap.c
mem_slab.c
float.c
Expand Down
21 changes: 9 additions & 12 deletions kernel/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -976,10 +976,10 @@ config KERNEL_WHOLE_ARCHIVE
to be included, rather than searching the archive for required object files.

config TOOLCHAIN_SUPPORTS_STATIC_INIT_GNU
# As of today only ARC MWDT toolchain doesn't support GNU-compatible
# initialization of static objects, new toolchains can be added
# here if required.
def_bool "$(ZEPHYR_TOOLCHAIN_VARIANT)" != "arcmwdt"
# As of today, we don't know of any toolchains that don't create
# either .ctors or .init_array sections containing initializer
# addresses in a fashion compatible with how Zephyr uses them.
def_bool y

config STATIC_INIT_GNU
bool "Support GNU-compatible initializers and constructors"
Expand All @@ -989,14 +989,11 @@ config STATIC_INIT_GNU
help
GNU-compatible initialization of static objects. This is required for
C++ constructor support as well as for initializer functions as
defined by GNU-compatible toolchains. This increases the size
of Zephyr binaries by around 100 bytes. If you know your
application doesn't need any initializers, you can disable this
option.
The ARC MWDT toolchain, does not support or use this setting,
and has instead separate C++ constructor initialization code.
Note the option CMAKE_LINKER_GENERATOR does not yet support this feature
or CPP.
defined by GNU-compatible toolchains. This increases the size of
Zephyr binaries by around 24 bytes. If you know your application
doesn't need any initializers, you can disable this option. The linker
will emit an error if constructors are needed and this option has been
disabled.

endmenu

Expand Down
19 changes: 17 additions & 2 deletions kernel/init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,20 @@

extern void boot_banner(void);

#ifdef CONFIG_STATIC_INIT_GNU

extern void (*__zephyr_init_array_start[])();
extern void (*__zephyr_init_array_end[])();

static void z_static_init_gnu(void)
{
void (**fn)();

for (fn = __zephyr_init_array_start; fn != __zephyr_init_array_end; fn++)
(**fn)();
}

Check notice on line 417 in kernel/init.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

You may want to run clang-format on this change 

kernel/init.c:417 
-	void	(**fn)();
-
-	for (fn = __zephyr_init_array_start; fn != __zephyr_init_array_end; fn++)
+	void (**fn)();
+
+	for (fn = __zephyr_init_array_start; fn != __zephyr_init_array_end; fn++) {
 		(**fn)();
+	}

#endif

/**
* @brief Mainline for kernel's background thread
Expand Down Expand Up @@ -433,8 +447,9 @@
#endif /* CONFIG_STACK_POINTER_RANDOM */
boot_banner();

void z_init_static(void);
z_init_static();
#ifdef CONFIG_STATIC_INIT_GNU
Copy link
Member

Choose a reason for hiding this comment

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

This would be removing support for constructors for the arc mwdt toolchain.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That seems bad. I'm really unclear as to how those constructors differ from the GNU constructors and whether we can support all of them in a single chunk of linker script.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@keith-packard

For MWDT we just call internal __do_global_ctors_aux function via z_cpp_init_static:

zephyr/lib/cpp/abi/cpp_init.c

Line 27 in e5c05da

__do_global_ctors_aux();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So MWDT places all constructors in .ctors sections, just like some GCC architectures (most GCC targets now use .init_array for "reasons", I assume). I'm not seeing any incompatibility between MWDT and GCC here; we can just collect all of the .ctors and .init_array sections together into one giant array of initialization functions and call them in the right order. That should make everything the same, eliminating any MWDT specific bits.

The current v2/linker.ld script places the .ctors sections between _fctors and _ectors, which looks like it might be specific to some arc initialization sequence which is not known to Zephyr? Does that ever get used? I can't see how it could work...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've pushed a separate patch in this PR which removes the special case for MWDT toolchains; I'd appreciate help understanding if this is correct or not.

Copy link
Member

Choose a reason for hiding this comment

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

The current v2/linker.ld script places the .ctors sections between _fctors and _ectors, which looks like it might be specific to some arc initialization sequence which is not known to Zephyr?

I guess to whatever that toolchain provided __do_global_ctors_aux() does, which is not something I can see. But why not just leave the MWDT specific handling there by now? (we don't need to clean up everything in one go, specially if we risk breaking things we can't even try).

Copy link
Member

Choose a reason for hiding this comment

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

The MetaWare __do_global_ctors_aux and _fctors/_ectors do something similar as the generic 'GNU' version, but it doesn't feel right to remove it. I'm not sure they indeed are fully equivalent, and if they are today, that may change in a next MetaWare version.
@evgeniy-paltsev, what do you think? Could we try and align/reuse GNU here, or should we stick with the MetaWare toolchain initialization?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But why not just leave the MWDT specific handling there by now? (we don't need to clean up everything in one go, specially if we risk breaking things we can't even try).

Yup, we figured that out yesterday :-). So, #74682 will be merged for 3.7 with just the fixes required to fix the bug.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The MetaWare __do_global_ctors_aux and _fctors/_ectors do something similar as the generic 'GNU' version, but it doesn't feel right to remove it.

Unless _fctors/_ectors are used by code outside the Zephyr tree, then the current MWDT bits don't seem like they could possibly work? It feels like this was ported from some other system which used these symbols in the same way that Zephyr uses __zephyr_init_array_start/__zephyr_init_array_end.

MWDT is different from the generic GNU version currently though -- it uses .ctors sections but doesn't construct a "gnu compatible" ctors section. A GNU compatible ctors section consists of a count word followed by an array of function pointers and finally a NULL pointer. The current MWDT ctors bits in Zephyr don't include the count or the trailing NULL.

This PR makes the GNU code match the MWDT code, removing the leading count and trailing NULL for the .ctors section.

I'm not sure they indeed are fully equivalent, and if they are today, that may change in a next MetaWare version. @evgeniy-paltsev, what do you think? Could we try and align/reuse GNU here, or should we stick with the MetaWare toolchain initialization?

From what I can see, the MWDT compiler places all C++ constructor addresses in .ctors sections. If you use the Zephyr initialization code (which I think you must so that constructors can use new), then there isn't really any further MWDT dependency -- the Zephyr linker script defines how those addresses are laid out in memory, and then the Zephyr initialization code defines how that memory area is processed at runtime.

Reading through the ARC libgcc code leaves me with some questions though:

https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=libgcc/config/arc/initfini.c;h=0183dd495ac5db165cc706ba2d62d9431e638972;hb=refs/heads/master

In that code, destructors are run in forward order while constructors are run in reverse order. All other systems run constructors in forward order (and destructors in reverse order).

z_static_init_gnu();
#endif /* CONFIG_STATIC_INIT_GNU */

/* Final init level before app starts */
z_sys_init_run_level(INIT_LEVEL_APPLICATION);
Expand Down
87 changes: 0 additions & 87 deletions kernel/init_static.c
View file
Open in desktop

This file was deleted.

32 changes: 24 additions & 8 deletions tests/kernel/common/src/constructor.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,37 @@
*
*/

static int constructor_value;
static int constructor_number;
static int constructor_values[3];

#define CONSTRUCTOR_VALUE 1
void __attribute__((__constructor__)) __constructor_init(void)
{
constructor_values[constructor_number++] = 31415;
}

void __attribute__((__constructor__(101))) __constructor_init_priority_101(void)
{
constructor_values[constructor_number++] = 101;
}

void
__attribute__((__constructor__))
__constructor_init(void)
void __attribute__((__constructor__(1000))) __constructor_init_priority_1000(void)
{
constructor_value = CONSTRUCTOR_VALUE;
constructor_values[constructor_number++] = 1000;
}

ZTEST(constructor, test_constructor)
{
zassert_equal(constructor_value, CONSTRUCTOR_VALUE,
"constructor test failed: constructor not called");
zassert_equal(constructor_number, 3,
"constructor test failed: constructor missing");
zassert_equal(constructor_values[0], 101,

Check notice on line 43 in tests/kernel/common/src/constructor.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

You may want to run clang-format on this change 

tests/kernel/common/src/constructor.c:43 
-	zassert_equal(constructor_number, 3,
-		      "constructor test failed: constructor missing");
+	zassert_equal(constructor_number, 3, "constructor test failed: constructor missing");
"constructor priority test failed:"
"constructor 101 not called first");
zassert_equal(constructor_values[1], 1000,
"constructor priority test failed:"
"constructor 1000 not called second");
zassert_equal(constructor_values[2], 31415,
"constructor priority test failed:"
"constructor without priority not called last");
}

extern void *common_setup(void);
Expand Down
Loading