toolchain: Add assembly-side counterpart of __aligned #85758

hIAhmad · 2025-02-13T23:31:48Z

Zephyr toolchain abstraction provides a macro __aligned(x) to add specified alignment, in bytes, to a chosen symbol for C/C++ source files but there is no portable counterpart available for symbols defined in assembly source files. This change-set adds a new macro ALIGN(x) for this purpose.

tejlmand

I would prefer having the macro named ALIGN.

tejlmand · 2025-02-18T11:01:15Z

include/zephyr/toolchain/common.h

+	defined(CONFIG_XTENSA) || defined(CONFIG_MIPS) || \
+	defined(CONFIG_ARCH_POSIX)
+
+    #define   BALIGN(x)   .balign   x


I think ALIGN is a better term because alignment is what we want.
I know the differences between .align and .balign, but for the code that wants to use alignment, then ALIGN is a better macro name and aligns with the PERFOPT_ALIGN macros in naming (which also uses .balign when possible).

tejlmand · 2025-02-18T11:04:44Z

include/zephyr/toolchain/common.h

+
+    #define   BALIGN(x)   .balign   x
+
+  #elif defined(CONFIG_ARC) || defined(CONFIG_SPARC)


Technically support for .balign is not an architecture thing but a toolchain supported instruction, whereas .align is generally supported across toolchains (but where the behavior is system specific 😞 ).

Yes, indeed. For preprocessor conditionals, I just followed the similar scheme that the logically close existing macro PERFOPT_ALIGN is using. But theoretically, yes, this is a bit off. However, it won't cause any trouble unless we (need to) support multiple toolchains for an architecture that either treat .align differently (i.e. one interprets its param as number of bytes and other as number of lower order bits to be zeroed) and/or one supports .balign and other doesn't.

Let's keep for now.
Just wanted to point it out.

Perhaps a code comment would be appropriate so someone later don't change this by accident.

Just noticed that PERFOPT_ALIGN implementation has a comment exactly for this clarification for the ARC case - I have updated the code to reuse the same comment.

RobinKastberg · 2025-02-18T11:26:47Z

Currently IAR use GNU assembler, but if we start using iasmarm alignment in assembly files are specified in Power-Of-Two unfortunately.
E.g.

 alignram 6 ; Align to a 64-byte boundary

Maybe this is a shortcoming we will need to fix in our toolchain however, because I am not sure how to make a clean solution that would allow both

@bjorniuppsala any ideas?

bjorniuppsala · 2025-02-18T11:34:24Z

Currently IAR use GNU assembler, but if we start using iasmarm alignment in assembly files are specified in Power-Of-Two unfortunately.
...
@bjorniuppsala any ideas?

I think fixing our assembler is the way to go.

ruuddw · 2025-02-18T11:56:20Z

include/zephyr/toolchain/common.h

+
+    #define   BALIGN(x)   .balign   x
+
+  #elif defined(CONFIG_ARC) || defined(CONFIG_SPARC)


For ARC classic there are multiple compilers, gnu and MetaWare. Metaware indeed doesn't support .balign, and .align works for both gnu and MetaWare. Should the condition for this choice be architecture, or rather toolchain variant? Maybe this shouldn't be in 'common.h' but specified per toolchain?

hIAhmad · 2025-02-18T12:30:16Z

I would prefer having the macro named ALIGN.

I also preferred it but unfortunately it conflicts with a (GNU/GNU-compatible) linker script directive of the same name and syntax. So instances like . = ALIGN(4); in linker scripts will get messed up after the preprocessor run, resulting in eventual link failures.

BYTE_ALIGN can be another alternative name that I initially used but then shortened it to BALIGN. If you'd prefer BYTE_ALIGN compared to BALGIN, I can revert to that.

Or we can consider some other good name, but I don't have any in my mind, as of yet.

tejlmand · 2025-02-21T09:10:43Z

I also preferred it but unfortunately it conflicts with a (GNU/GNU-compatible) linker script directive of the same name and syntax. So instances like . = ALIGN(4); in linker scripts will get messed up after the preprocessor run, resulting in eventual link failures.

You just need to properly safe guard the definition so that it's not used for linker script preprocessing, like this:

#if defined(_ASMLANGUAGE) && !defined(_LINKER)

tejlmand · 2025-02-21T09:13:02Z

include/zephyr/toolchain/common.h

+ * 'test_symbol' will get aligned to 4 bytes.
+ */
+
+#ifdef _ASMLANGUAGE


If you use:

Suggested change

#ifdef _ASMLANGUAGE

#if defined(_ASMLANGUAGE) && !defined(_LINKER)

then it will be defined for the linker script template pre-processin, and thus not conflict with the linker directive of same name and you can use ALIGN as macro name.

Thanks for the pointer. I have updated it.

hIAhmad · 2025-02-21T14:31:34Z

@tejlmand BTW, now PERFOPT_ALIGN can be implemented in terms of this newly introduced ALIGN macro. It will consolidate the implementation and reduce the effort to add support for new architectures/toolchains.

If you agree, I can post a follow-up PR afterwards.

include/zephyr/toolchain/common.h

tejlmand

minor nit observed, rest looks good.

tejlmand · 2025-02-26T11:42:04Z

include/zephyr/toolchain/common.h

+
+  #elif defined(CONFIG_ARC)
+
+    /* .align assembler directive is supposed by all ARC toolchains and it is


supposed ?

Suggested change

/* .align assembler directive is supposed by all ARC toolchains and it is

/* .align assembler directive is supported by all ARC toolchains and it is

Carry over from the existing comment in PERFOPT_ALIGN that I reused. Fixed here. Will also fix the old reference comment in the follow-up PR while rebasing PERFOPT_ALIGN over this newly introduced ALIGN macro.

tejlmand · 2025-02-26T11:43:35Z

If you agree, I can post a follow-up PR afterwards.

will be appreciated 👍

Zephyr toolchain abstraction provides a macro __aligned(x) to add specified alignment, in bytes, to a chosen symbol for C/C++ source files but there is no portable counterpart available for symbols defined in assembly source files. This change-set adds a new macro ALIGN(x) for this purpose. Signed-off-by: Irfan Ahmad <irfan.ahmad@siemens.com>

hIAhmad · 2025-02-26T13:48:12Z

If you agree, I can post a follow-up PR afterwards.

will be appreciated 👍

Created #86366 - will rebase and put in review, once this PR gets merged.

hIAhmad · 2025-03-21T11:33:01Z

@stephanosio, @nordicjm Can you please review/re-review and give your +1 or feedback? This PR is blocking other PRs. Already has +1 from @tejlmand - needs one more.

zephyrbot added the area: Toolchains Toolchains label Feb 13, 2025

zephyrbot requested review from stephanosio and tejlmand February 13, 2025 23:32

zephyrbot assigned tejlmand Feb 13, 2025

hIAhmad force-pushed the portable_balign branch 3 times, most recently from 4260068 to 8e0624a Compare February 14, 2025 02:43

tejlmand requested changes Feb 18, 2025

View reviewed changes

tejlmand requested review from RobinKastberg, ruuddw, evgeniy-paltsev and abrodkin February 18, 2025 11:05

ruuddw reviewed Feb 18, 2025

View reviewed changes

tejlmand reviewed Feb 21, 2025

View reviewed changes

hIAhmad force-pushed the portable_balign branch from 8e0624a to e8411f4 Compare February 21, 2025 13:59

hIAhmad mentioned this pull request Feb 24, 2025

build: A high-level mechanism for embedding raw data from binary files #86204

Open

nordicjm reviewed Feb 26, 2025

View reviewed changes

include/zephyr/toolchain/common.h Show resolved Hide resolved

tejlmand previously approved these changes Feb 26, 2025

View reviewed changes

hIAhmad dismissed tejlmand’s stale review via 899b376 February 26, 2025 12:35

hIAhmad force-pushed the portable_balign branch from e8411f4 to 899b376 Compare February 26, 2025 12:35

hIAhmad force-pushed the portable_balign branch from 899b376 to 553927d Compare February 26, 2025 12:50

hIAhmad mentioned this pull request Feb 26, 2025

toolchain: Rebase PERFOPT_ALIGN on top of ALIGN(x) #86366

Open

tejlmand approved these changes Mar 3, 2025

View reviewed changes

nordicjm approved these changes Mar 24, 2025

View reviewed changes

kartben merged commit 58d5e35 into zephyrproject-rtos:main Mar 24, 2025
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

toolchain: Add assembly-side counterpart of __aligned #85758

toolchain: Add assembly-side counterpart of __aligned #85758

hIAhmad commented Feb 13, 2025 •

edited

Loading

tejlmand left a comment

tejlmand Feb 18, 2025

tejlmand Feb 18, 2025

hIAhmad Feb 18, 2025 •

edited

Loading

tejlmand Feb 21, 2025

hIAhmad Feb 21, 2025 •

edited

Loading

RobinKastberg commented Feb 18, 2025

bjorniuppsala commented Feb 18, 2025

ruuddw Feb 18, 2025

hIAhmad commented Feb 18, 2025

tejlmand commented Feb 21, 2025

tejlmand Feb 21, 2025

hIAhmad Feb 21, 2025

hIAhmad commented Feb 21, 2025 •

edited

Loading

tejlmand left a comment

tejlmand Feb 26, 2025

hIAhmad Feb 26, 2025

tejlmand commented Feb 26, 2025

hIAhmad commented Feb 26, 2025

hIAhmad commented Mar 21, 2025


		#define BALIGN(x) .balign x

		#elif defined(CONFIG_ARC) \|\| defined(CONFIG_SPARC)

	#ifdef _ASMLANGUAGE
	#if defined(_ASMLANGUAGE) && !defined(_LINKER)


		#elif defined(CONFIG_ARC)

		/* .align assembler directive is supposed by all ARC toolchains and it is

	/* .align assembler directive is supposed by all ARC toolchains and it is
	/* .align assembler directive is supported by all ARC toolchains and it is

toolchain: Add assembly-side counterpart of __aligned #85758

toolchain: Add assembly-side counterpart of __aligned #85758

Conversation

hIAhmad commented Feb 13, 2025 • edited Loading

tejlmand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hIAhmad Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hIAhmad Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

RobinKastberg commented Feb 18, 2025

bjorniuppsala commented Feb 18, 2025

Choose a reason for hiding this comment

hIAhmad commented Feb 18, 2025

tejlmand commented Feb 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hIAhmad commented Feb 21, 2025 • edited Loading

tejlmand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejlmand commented Feb 26, 2025

hIAhmad commented Feb 26, 2025

hIAhmad commented Mar 21, 2025

hIAhmad commented Feb 13, 2025 •

edited

Loading

hIAhmad Feb 18, 2025 •

edited

Loading

hIAhmad Feb 21, 2025 •

edited

Loading

hIAhmad commented Feb 21, 2025 •

edited

Loading