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

Rewrite darling_thread_entry Inline Assembly #1401

Merged
merged 2 commits into from
Oct 30, 2023

Conversation

CuriousTommy
Copy link
Contributor

@CuriousTommy CuriousTommy commented Jul 9, 2023

From the discussion in Discord, there are some strange/potentially problematic design choices with the current inline assembly code. This PR rewrites the problematic assembly code and documents what the assembly code is trying to do.

In addition to the assembly fix, the arch executable is also updated to allow running universal binaries in i386 mode.

@CuriousTommy

This comment was marked as outdated.

@CuriousTommy CuriousTommy force-pushed the mldr_thread_entry_rework branch from 27fe5f8 to 2ded39e Compare September 4, 2023 16:10
@CuriousTommy

This comment was marked as resolved.

@CuriousTommy CuriousTommy force-pushed the mldr_thread_entry_rework branch 2 times, most recently from 0549204 to c036704 Compare September 7, 2023 02:49
@CuriousTommy CuriousTommy changed the title [WIP] Rewrite darling_thread_entry Inline Assembly Rewrite darling_thread_entry Inline Assembly Sep 7, 2023
@CuriousTommy
Copy link
Contributor Author

@bugaevc and @facekapow: If you have the freetime, can you check my PR? Since I'm not too familiar with x86/x86_64 assembly, I want to make sure it looks good before I merge the changes in.

@CuriousTommy CuriousTommy marked this pull request as ready for review September 7, 2023 10:28
Copy link
Member

@facekapow facekapow left a comment

Choose a reason for hiding this comment

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

Just a minor note, but LGTM.

src/startup/mldr/elfcalls/threads.c Outdated Show resolved Hide resolved
@CuriousTommy CuriousTommy force-pushed the mldr_thread_entry_rework branch 2 times, most recently from 6eaf40e to b92e59c Compare September 30, 2023 16:07
From the discussion in Discord, there are some strange/potentionally problematic design choices with the current inline assembly code.

To take advantage of the GNU register asm(...) feature, the std version need to be set to gnu11, instead of c11.
@CuriousTommy CuriousTommy force-pushed the mldr_thread_entry_rework branch from b92e59c to 44ccee6 Compare October 30, 2023 17:41
@CuriousTommy CuriousTommy force-pushed the mldr_thread_entry_rework branch from 44ccee6 to 2c0c72d Compare October 30, 2023 20:03
@CuriousTommy CuriousTommy merged commit 82525f6 into master Oct 30, 2023
4 checks passed
@CuriousTommy CuriousTommy deleted the mldr_thread_entry_rework branch October 30, 2023 23:06
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.

2 participants