compile: restore alignment checks for some cases of ALIGN()

[lyakh]

Commit 32fe8a4 ("common.h: restore basic ALIGN() macro compatible with assembly") removed power-of-2 checks from some uses of the ALIGN() macro. Restore them while also making sure linker isn't broken either.

@lgirdwood @marc-hb not sure this version is perfect, please raise concerns, might need some improvement.

[marc-hb]

UPDATE: I wrote this before seeing your comment in #3770, ignore some of it sorry.

---

This looks a bit like a revert of 9db16c1, which I think was added by mistake, did you see my question in PR #3770? If 9db16c1 isn't needed then I'd prefer to start by reverting it and then take it from there. It's very complicated wrap one head's around all cases and possibilities and virtually removing one unnecessary commit from the history would help.

Commit 32fe8a4 ("common.h: restore basic ALIGN() macro compatible with assembly") removed power-of-2 checks from some uses of the ALIGN() macro

Can you be more specific than "some" and give just one example / location where commit 32fe8a4 does not check whereas commit 32fe8a4~1 did check? I took more care and time to do 32fe8a4 than I'm willing to admit.

[lgirdwood]

@marc-hb I assume you are good here, if so please do "approve". If this passes CI we can take for rc1 today and discuss further if needed for rc 2 or v1.7. Lets wait for CI results.

[marc-hb]

This comment is misleading: the linker does not use any C macro, the linker has its own ALIGN() function, see .x.in files. I explained this in the hard learned comment that this PR is currently dropping :-(

In fact moving this macro outside #if !defined(LINKER) is strange because the LINKER doesn't use it.

"suitable for C, assembly" is also misleading because you don't want to use this in C, in C you want to use the better one. The reason this dumber macro exists is for files included in BOTH C and dumber assembly.

On the other hand this PR drops the #else LINKER clause that was just added by @lgirdwood . That is good because common.h is not used by the linker.

[lyakh]

@marc-hb I don't think your analysis is quite correct. You're certainly right about linker's own ALIGN(x) function, that isn't what I meant. But linker scripts do include normal C headers and use values from them, cf. src/platform/cannonlake/cannonlake.x.in

#include <sof/lib/memory.h>
...
PROVIDE(_memmap_vecbase_reset = HP_SRAM_VECBASE_RESET);

[marc-hb]

You're right I should NOT have written "does not use ANY C macro". However it uses a small subset and apparently not any sort of ALIGN() macro which is what matters right here. So this comment should not say "suitable for linker scripts". I put a lot of effort in the comment this PR is currently dropping and I don't yet see what was wrong with it.

[lyakh]

@marc-hb I removed it because I think it's wrong.

 * memory.h files require this
 * exact spelling matching the linker function because memory.h values
 * are _also_ copied unprocessed to the .x[.in] linker script

I don't think that's correct. I don't see any requirement to use an ALIGN() macro in headers to match the ALIGN() linker function and isn't this contradicting what you're saying now? Now you're saying that "not any sort of ALIGN() macro" is used in linker scripts from C headers. Or am I misunderstanding anything?

[marc-hb]

I removed it because I think it's wrong.

OK, next time please state clearly why you remove it because correct or not, a comment phrased like this one was clearly the conclusion of a significant effort and not just some random guesses.

I don't see any requirement to use an ALIGN() macro in headers to match the ALIGN() linker function

I think you haven't tested this as much as I did and you underestimate how complicated it can be.

src/platform/suecreek/include/platform/lib/memory.h has:

#define SRAM_VECBASE_RESET ALIGN(_SRAM_VECBASE_RESET,4096)

After too many layers of indirections in memory.h, this produces many lines like this one in build_sue_gcc/suecreek.x:

ALIGN(((((((((((((0xBE000000 + 0x1000) ..... + 0x1000) + 0x100),4096)

Just change 4096 to 4097 and you will see it. BTW purposely breaking things is the fastest way to expose and understand insane levels of indirections in macros or in the build. Highly recommended.

This is what was broken by 39266ca and that I fixed in 32fe8a4 (and "concurrently" by @lgirdwood
in 9db16c1). The commit message of 32fe8a4 also has a detailed explanation.

Speaking of unseen things, can you point at one specific place in the code where I "removed power-of-2 checks from some uses of the ALIGN() macro"? I tried hard not to do that.

[lyakh]

I don't see any requirement to use an ALIGN() macro in headers to match the ALIGN() linker function

I think you haven't tested this as much as I did and you underestimate how complicated it can be.

src/platform/suecreek/include/platform/lib/memory.h has:

#define SRAM_VECBASE_RESET ALIGN(_SRAM_VECBASE_RESET,4096)

After too many layers of indirections in memory.h, this produces many lines like this one in build_sue_gcc/suecreek.x:

ALIGN(((((((((((((0xBE000000 + 0x1000) ..... + 0x1000) + 0x100),4096)

Just change 4096 to 4097 and you will see it. BTW purposely breaking things is the fastest way to expose and understand insane levels of indirections in macros or in the build. Highly recommended.

So, I still don't see how it is related to the linker ALIGN() function. I know the linker function with one argument like

  .MemoryExceptionVector.text : ALIGN(4)

The above ALIGN() has two arguments. See https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_chapter/ld_3.html#SEC14

This is what was broken by 39266ca and that I fixed in 32fe8a4 (and "concurrently" by @lgirdwood
in 9db16c1). The commit message of 32fe8a4 also has a detailed explanation.

Your description had several entries in it, I didn't understand which specific one referred to this change. @lgirdwood 's fix that you're referring to here too, actually also confirms, that it doesn't necessarily have to be spelled as ALIGN() and he fixed the Sue Creek compilation issue without modifying macros for everybody. AFAIK that was the only thing that my PR broke. At least in what concerns SOF proper. I think you also mentioned some tooling problems.

Speaking of unseen things, can you point at one specific place in the code where I "removed power-of-2 checks from some uses of the ALIGN() macro"? I tried hard not to do that.

You made ALIGN() non-checking. And it is used in memory.h, e.g.

#define SOF_CORE_S_SIZE \
	ALIGN((HEAP_SYSTEM_S_SIZE + HEAP_SYS_RUNTIME_S_SIZE + SOF_STACK_SIZE),\
		SRAM_BANK_SIZE)

and that is then also used in memory.c:

		sof->memory_map->system[i].heap =
			(uintptr_t)&_sof_core_s_start +
			((i - 1) * SOF_CORE_S_SIZE);

[marc-hb]

I know the linker function with one argument

The variant with one argument aligns the location counter, the variant with two arguments aligns anything:
https://sourceware.org/binutils/docs/ld/Builtin-Functions.html

@lgirdwood 's fix that you're referring to here too, actually also confirms, that it doesn't necessarily have to be spelled as ALIGN() and he fixed the Sue Creek compilation issue without modifying macros for everybody.

I just realized @lgirdwood entirely removed any form of ALIGN() from build_sue_gcc/suecreek.x, fascinating! Did you ever look at this file?

Here's how build_sue_gcc/suecreek.x fails when it ends up with ALIGN_SOMETHING() instead of ALIGN():
https://github.com/thesofproject/sof/pull/3822/checks?check_run_id=1837765639
Changing two lines is enough to see this, see the diff.

[lyakh]

I know the linker function with one argument

The variant with one argument aligns the location counter, the variant with two arguments aligns anything:
https://sourceware.org/binutils/docs/ld/Builtin-Functions.html

Ok, I was looking only at our sources and at an older version of documentation. So they must have only added it relatively recently, not more than 10 years ago.

@lgirdwood 's fix that you're referring to here too, actually also confirms, that it doesn't necessarily have to be spelled as ALIGN() and he fixed the Sue Creek compilation issue without modifying macros for everybody.

I just realized @lgirdwood entirely removed any form of ALIGN() from build_sue_gcc/suecreek.x, fascinating! Did you ever look at this file?

Here's how build_sue_gcc/suecreek.x fails when it ends up with ALIGN_SOMETHING() instead of ALIGN():
https://github.com/thesofproject/sof/pull/3822/checks?check_run_id=1837765639
Changing two lines is enough to see this, see the diff.

Interesting, then why does it work with ALIGN_UP_INTERNAL()?

[lgirdwood]

@marc-hb @lyakh do we have a fix ? if so, lets get it ready for rc2 and we can continue discussion on why it did or didn't work later on.

[lyakh]

@marc-hb @lyakh do we have a fix ? if so, lets get it ready for rc2 and we can continue discussion on why it did or didn't work later on.

@lgirdwood As mentioned in #3804 (comment) this my patch restores cases of removes alignment checking, since @marc-hb didn't object to that comment, I presume he agrees. So, yes, I think this is good to merge.

[lgirdwood]

@marc-hb are you good with this to merge now ?

[marc-hb]

and that is then also used in memory.c:

Thanks for the example! Also used in pm_runtime.c. I'm finally pleading guilty of removing some checks, thanks for your patience.

(note this memory.c code is optimized away when CONFIG_CORE_COUNT=1, that confused me a bit)

it doesn't necessarily have to be spelled as ALIGN()

OK, in that case the C code should not use any macro called "ALIGN" exactly like the linker, right? This will avoid any confusion with the TWO linker functions with the exact same name and remove one confusing layer of ALIGN_x->ALIGN-y->ALIGN_z macro indirection. Plus there are very few ALIGN left in C code, so easy enough to replace them all with either ALIGN_UP or even better with ALIGN_UP_COMPILE.

So I went and tried to do exactly that and then look what surprise I found in pre-processed build_apl_gcc/apollolake.x!

    432     _sof_stack_end = ABSOLUTE(.);
    433     . = ALIGN (64);
    434     _sof_core_s_start = ABSOLUTE(.);
->  435     . = . + ((1 - 1) * ALIGN((0x6000 + (64 * 64 + 8 * 512 + 4 * 1024) + 0x1000), (64 * 1024)));
    436     _sof_core_s_end = ABSOLUTE(.);
    437     _bss_end = ABSOLUTE(.);
    438   } >sof_fw :sof_fw_phdr
    439   _end = _sof_stack_start;
    440   PROVIDE(end = _sof_stack_start);
    441   _stack_sentry = _sof_stack_start;
    442   __stack = _sof_stack_end;
->  443   _core_s_size = ALIGN((0x6000 + (64 * 64 + 8 * 512 + 4 * 1024) + 0x1000), (64 * 1024));
    444   _system_heap = _system_heap_start;

This is the .x file before and after this PR. So if the macro ALIGN_UP_INTERNAL() is "suitable for linker scripts" as this PR states, then why is it still not use in some linker scripts? But... used in other linker scripts?

Interesting, then why does it work with ALIGN_UP_INTERNAL()?

I'm not sure, I keep getting lost too. I think we should not merge any additional change unless it can demonstrate understanding of all questions like this one. We have a very serious combinatorial explosion here:

• compiler/assembler/linker
• xcc/gcc
• run-time check / compile-time check / no check
• linker's ALIGN(x) / linker's ALIGN(x,y) / ALIGN(x,y) macro / ALIGN_UP / ALIGN_UP_COMPILE / ALIGN_UP_INTERNAL / ...
• before commit 39266ca, commit 39266ca, commit 32fe8a4 and commit 9db16c1

I'm sorry I underestimated the complexity too and contributed one commit to that explosion. To reduce this huge mess we should not add any new commit until we have a 100% understanding of everything, demonstrated by clear comments preempting all future questions. Bonus points if it reduces some of that combinatorial explosion.

PS: Note how I had to actually go and change some code to find the above because if you just grep ALIGN *.x then you're drown in the noise of the single argument ALIGN(x) function. This collision is a strong reason to stop using the "ALIGN" spelling in C code if possible. Granted, you can grep ALIGN(.*,.*) but that's inconvenient and some people don't know regular expressions.

[lyakh]

@marc-hb Thanks for your analysis. I know that these things are complex, and I agree that we don't have a full understanding of some of the effects here. But I disagree with your suggestion to not commit anything else on this topic. I think we should fix the "roll-back" that your previous PR introduced: we should re-add checks where you dropped them. Without that having partial checks in place seems rather wrong to me.

[lyakh]

@lgirdwood @marc-hb ok, actually, it's rather simple. None of that affects the linker:

sof$ grep '^\#if\|^\#endif' src/include/sof/common.h
#ifndef __SOF_COMMON_H__
#if !defined(LINKER)
#ifdef VERIFY_ALIGN
#ifdef __XCC__
#endif /* not __XCC__ */
#endif
#if !defined(__ASSEMBLER__)
#ifndef __ZEPHYR__ /* Already present and compatible via Zephyr headers */
#endif
#ifndef __ZEPHYR__ /* Already present and compatible via Zephyr headers */
#endif
#if defined __XCC__
#endif /* __XCC__ */
#endif /* __ASSEMBLER__ */
#endif /* LINKER */
#endif /* __SOF_COMMON_H__ */

So, all those ALIGN() definitions are under #if !defined(LINKER). @marc-hb your comment

 * memory.h files require this
 * exact spelling matching the linker function because memory.h values
 * are _also_ copied unprocessed to the .x[.in] linker script

was correct, I've restored it. But your replacing ALIGN_UP() -> ALIGN_UP_INTERNAL() wasn't needed because ALIGN() is never used in assembly files. It wouldn't compile otherwise and to make that clear we could even move the line

#define ALIGN ALIGN_UP

a couple lines down under #if !defined(__ASSEMBLER__).

[lgirdwood]

@marc-hb ok for you now ?

[marc-hb]

But your replacing ALIGN_UP() -> ALIGN_UP_INTERNAL() wasn't needed because ALIGN() is never used in assembly files.

It was until @lgirdwood's commit 9db16c1, see below. That's why I wrote it in the comment.

I spent literally hours on commit 32fe8a4 and while you found a regression in it (thx!), every single line change had a thoroughly tested and documented purpose.

[ 90%] Building ASM object src/arch/xtensa/CMakeFiles/reset.dir/xtos/reset-vector.S.o
sof/src/arch/xtensa/xtos/reset-vector.S: Assembler messages:
sof/src/arch/xtensa/xtos/reset-vector.S:628: Error: bad expression

    627 #if CONFIG_BOOT_LOADER && !CONFIG_VM_ROM
    628         movi    a0, SOF_TEXT_BASE
    629         callx0  a0
    630 #else
    631         call0   _start          // jump to _start (in crt1-*.S)
    632 #endif

It was also used in a static initializer:

[ 86%] Building C object src/platform/suecreek/CMakeFiles/base_module.dir/base_module.c.o
In file included from sof/src/platform/intel/cavs/include/cavs/lib/memory.h:13,
                 from sof/src/platform/suecreek/include/platform/lib/memory.h:15,
                 from sof/src/include/sof/lib/memory.h:11,
                 from sof/src/platform/suecreek/base_module.c:7:
sof/src/include/sof/common.h:56:35: error: braced-group within expression allowed only inside a function
 #define ALIGN_UP(size, alignment) ({      \
                                   ^
sof/src/include/sof/common.h:86:27: note: in expansion of macro 'ALIGN_UP'
 #define ALIGN(val, align) ALIGN_UP(val, align)
                           ^~~~~~~~
sof/src/platform/suecreek/include/platform/lib/memory.h:436:28: note: in expansion of macro 'ALIGN'
 #define SRAM_VECBASE_RESET ALIGN(_SRAM_VECBASE_RESET,4096)
                            ^~~~~
sof/src/platform/suecreek/include/platform/lib/memory.h:226:31: note: in expansion of macro 'SRAM_VECBASE_RESET'
 #define HP_SRAM_VECBASE_RESET SRAM_VECBASE_RESET
                               ^~~~~~~~~~~~~~~~~~
sof/src/platform/suecreek/include/platform/lib/memory.h:231:24: note: in expansion of macro 'HP_SRAM_VECBASE_RESET'
 #define SOF_FW_START  (HP_SRAM_VECBASE_RESET + SOF_TEXT_START_SIZE)
                        ^~~~~~~~~~~~~~~~~~~~~
sof/src/platform/suecreek/include/platform/lib/memory.h:234:26: note: in expansion of macro 'SOF_FW_START'
 #define SOF_TEXT_START  (SOF_FW_START)
                          ^~~~~~~~~~~~
sof/src/platform/suecreek/base_module.c:21:18: note: in expansion of macro 'SOF_TEXT_START'
   .entry_point = SOF_TEXT_START,
                  ^~~~~~~~~~~~~~

BTW we should add some indentation at some point, there's already some elsewhere in the same file:

sof$ grep '^\#if\|^\#endif' src/include/sof/common.h
#ifndef __SOF_COMMON_H__
#  if !defined(LINKER)
#     ifdef VERIFY_ALIGN
#        ifdef __XCC__

[marc-hb]

The current implementation is complicated because it tries to be smart and "automagical" with plenty, nested # if assembler, # if linker conditionals yets fails because suecreek's memory.h has to look like this:

#define SOF_CORE_S_SIZE    ALIGN            ((HEAP_SYSTEM_S_SIZE + ...), SRAM_BANK_SIZE)
...
#define SRAM_VECBASE_RESET ALIGN_UP_INTERNAL(_SRAM_VECBASE_RESET, 4096)

So at the end of the day this super smart common.h code forces the user to use two different ALIGN macro in the same file, one of them called "INTERNAL" (!) and it took us weeks to reverse engineer why that's needed. Really bad.

I agree that we don't have a full understanding of some of the effects here. But I disagree with your suggestion to not commit anything else on this topic.

While I'm not suggesting we fix everything in one PR, we should have some clear design directions in the comments in common.h otherwise we'll keep running in circles. Here's something I tried that I think we can aim at:

A. Don't pretend the user can use only one "ALIGN()" macro everywhere and then something magical will automatically implement the best checks everywhere depending on: linker/assembler/static initializer/run-time. So let's ask the user at least one thing: it this a constant or a variable? For instance: 1. ALIGN_UP_COMPILE() for constants for sure, 2. ALIGN_UP() for likely variables.

B. Redirect the user interface macros to different and actually internal macros depending on linker/assembler/gcc/xcc

C. Maintain a clear separation between user interface and INTERNAL macros, meaning ALIGN() cannot be part of the user interface because it's already a built-in linker function! That proved incredibly painful to troubleshoot.

The conditionals in common.h will still a bit be complex but A B and C above help. For instance, neither the assembler nor the linker need to define macros for variables.

That's one possible direction. Now here are some small, incremental steps towards that direction:

• First, revert 9db16c1. It didn't fix anything that wasn't already fixed (because it was implemented concurrently with 32fe8a4), it introduced one more state to explore => more reverse-engineering work, and finally it used an INTERNAL macro in the code! Reverting it means backtracking the last step/state in the git history.
• Then, restore the checks I removed by mistake. For this, replace ALIGN() with the appropriate macro, in memory.h that's a constant so ALIGN_UP_COMPILE().

I did the above including B, combined it your PR and it compiled! Can you try this or do you prefer me to submit it?

[marc-hb]

The most basic alignment macro with no checks, suitable for C, assembly and for linker scripts

This comment is still misleading:

• maybe you meant "suitable for C static initializers"?
• it seems to be contradicting this other comment below:

memory.h values are also copied unprocessed to the .x[.in] linker script

that's because ALIGN is unprocessed while ALIGN_UP_INTERNAL is. Too much of a mess.

[lyakh]

The most basic alignment macro with no checks, suitable for C, assembly and for linker scripts

This comment is still misleading:

* maybe you meant "suitable for C _static initializers_"?
* it seems to be contradicting this other comment below:

@marc-hb I tried to improve it, please, check.

memory.h values are also copied unprocessed to the .x[.in] linker script

that's because ALIGN is unprocessed while ALIGN_UP_INTERNAL is. Too much of a mess.

I'd suggest, that ALIGN() is unprocessed, because common.h isn't included in memory.h as it really should. But interestingly, adding it there doesn't change the resulting .x, strange.

Anyway, I'd like to just commit this one instead of going your proposed route of reverting and fixing. We have introduced an albeit harmless, but still a regression and it would be good to fix it, if possible for 1.7. After that you can propose any further improvements.

[lgirdwood]

@marc-hb @lyakh btw, does Zephyr have an equivalent ?

[lyakh]

@marc-hb @lyakh btw, does Zephyr have an equivalent ?

@lgirdwood yes, they have ROUND_UP() and ROUND_DOWN() which just use binary power-of-2 alignment with no checks. Those would be perfectly fine for us too if we had done them from the beginning and never had those division-based alignment in the beginning.

[lgirdwood]

@marc-hb can we update to use the Zephyr versions as part of our native Zephyr work.

[paulstelian97]

Please resubmit with "main" as PR base branch.