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.

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

Kernel/aarch64: Don't set multiboot_modules to an empty array on-stack #18567

Merged
merged 1 commit into from
Apr 29, 2023

Conversation

supercomputer7
Copy link
Member

Since multiboot_modules_count is set to 0, we can safely set the multiboot_modules pointer to 0 (null pointer), as we don't use multiboot on aarch64 anyway.

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Apr 29, 2023
Kernel/Arch/init.cpp Outdated Show resolved Hide resolved
Since multiboot_modules_count is set to 0, we can safely set the
multiboot_modules pointer to 0 (null pointer), as we don't use multiboot
on aarch64 anyway.
@nico
Copy link
Contributor

nico commented Apr 29, 2023

Could maybe say "fixed the aarch64 build after $commit_hash" in the commit description

@supercomputer7
Copy link
Member Author

Could maybe say "fixed the aarch64 build after $commit_hash" in the commit description

I am not 100% sure which commit led to this. Is this my commit (897c4e5) in which I baked the prekernel and kernel together for x86?

@ADKaster
Copy link
Member

Even if we go back before that PR, we were doing this awkward memcpy from nullptr of 0 * sizeof(mutliboot_module_t) bytes. I'm not sure I understand either why this is suddenly broke.

I just tested latest master with a GCC build and it seems to work fine without this patch? Who was actually broken by this? was it @BertalanD ?

@BertalanD
Copy link
Member

Yes, it was me.

897c4e5 removed the multiboot_modules variable without also removing the assignment to it in the ARCH(AARCH64) code path.

https://discord.com/channels/830522505605283862/897113928168013855/1101503142375456799

@ADKaster
Copy link
Member

ADKaster commented Apr 29, 2023

Since we have since reverted 897c4e5 because of #18558, I think it's fine to merge this without pointing out that it fixes a secondary regression from it. 🤷

@ADKaster ADKaster merged commit d430ee8 into SerenityOS:master Apr 29, 2023
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Apr 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants