Skip to content

modules: Apparently asking 0 alignment is dangerous, use 4 as default #10324

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

Open
jsarha wants to merge 1 commit into
base: main
Choose a base branch
from
Open

modules: Apparently asking 0 alignment is dangerous, use 4 as default #10324

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 4 additions & 2 deletions src/include/sof/audio/module_adapter/module/generic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,13 +191,15 @@ int module_init(struct processing_module *mod);
void *mod_balloc_align(struct processing_module *mod, size_t size, size_t alignment);
static inline void *mod_balloc(struct processing_module *mod, size_t size)
{
return mod_balloc_align(mod, size, 0);
/* IS_ALIGNED() crashes if alignment is 0, so use pointer size as default alignment */
return mod_balloc_align(mod, size, sizeof(void *));
Copy link
Collaborator

@lyakh lyakh Oct 27, 2025

Choose a reason for hiding this comment

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

is this because of

sof/zephyr/lib/alloc.c

Line 296 in a86f371

assert(IS_ALIGNED(mem, align));
? Should we fix it there maybe?

Copy link
Contributor Author

@jsarha jsarha Oct 27, 2025

Choose a reason for hiding this comment

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

Possibly both places. But defaulting to ptr size alignment is a sane thing to do even if there was no assert there.

Copy link
Collaborator

@lyakh lyakh Oct 27, 2025

Choose a reason for hiding this comment

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

Possibly both places. But defaulting to ptr size alignment is a sane thing to do even if there was no assert there.

@jsarha but why here? This is way too high a level imho. This is just a module allocation wrapper. In principle - yes, I'd agree, that allocating with less than 4 bytes alignment isn't very meaningful, and I actually suspect this is already the case with Zephyr heap allocator. So, doesn't seem to make much sense to me here.

Copy link
Contributor Author

@jsarha jsarha Oct 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@lyakh, I do not want to have parallel mod_alloc implementations for specified alignment and unspecified alignment, so I need to implement mod_alloc_align() and call it from mod_alloc(). In that call I have to specify the default alignment and sizeof(void *) is a sane default.

There is still a strong argument to fix the rballoc_align() implementation so that it would not crash with zero alignment argument, or even fixing the Zephyr IS_ALIGNED() macro so that it would not crash if align argument is zero. But those valid fixes do not make this change invalid.

Copy link
Collaborator

@lyakh lyakh Oct 28, 2025

Choose a reason for hiding this comment

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

@jsarha sorry, I still don't find this useful. Forcing a sizeof(void *) alignment in just one of high level allocation APIs seems too random to me, while we have a (perceived) bug in a specific place that should be fixed. And fixing it in two locations appears redundant to me too.

Copy link
Contributor Author

@jsarha jsarha Oct 28, 2025

Choose a reason for hiding this comment

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

@lyakh , Ok, would you then let default alignment of 1 to pass? I feel that alignment of 1 would really be an odd choice, but not as odd as 0. Or should I just close this PR? Its not like any of my PR are moving any more.

Copy link
Member

@abonislawski abonislawski Oct 28, 2025

Choose a reason for hiding this comment

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

Lets fix this in this PR, alignment "0" is simply wrong

Copy link
Collaborator

@lyakh lyakh Oct 29, 2025

Choose a reason for hiding this comment

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

@jsarha @abonislawski I think it's rather common to use 0 to mean "no explicit alignment," e.g. https://github.com/zephyrproject-rtos/zephyr/blob/a0096b719c22e1af818f56f6ae0657c1635c3cf0/kernel/kheap.c#L136 or

sof/zephyr/lib/alloc.c

Line 489 in a86f371

return rmalloc_align(flags, bytes, 0);

Copy link
Member

@abonislawski abonislawski Oct 29, 2025

Choose a reason for hiding this comment

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

Upon checking it looks like alignment 0 is present in some zephyr docs, in that case our assert call for IS_ALIGNED is too general and needs an additional condition

}

void *mod_alloc_align(struct processing_module *mod, size_t size, size_t alignment);
static inline void *mod_alloc(struct processing_module *mod, size_t size)
{
return mod_alloc_align(mod, size, 0);
/* IS_ALIGNED() crashes if alignment is 0, so use pointer size as default alignment */
return mod_alloc_align(mod, size, sizeof(void *));
}

static inline void *mod_zalloc(struct processing_module *mod, size_t size)
Expand Down
Loading