Skip to content

[Refactor] Syscall number unification in glibc #132

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

Draft
wants to merge 32 commits into
base: tmp-main
Choose a base branch
from

Conversation

Yaxuan-w
Copy link
Member

@Yaxuan-w Yaxuan-w commented Mar 24, 2025

No description provided.

Copy link
Member

@JustinCappos JustinCappos left a comment

Choose a reason for hiding this comment

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

Overall, I'm keen on what this is trying to do. This cleanup will be really useful when it lands. The PR does seem to have a lot of unrelated changes mixed in though...

@Yaxuan-w Yaxuan-w mentioned this pull request Mar 24, 2025
@bblackheart013 bblackheart013 marked this pull request as ready for review March 29, 2025 04:19
Copy link
Member Author

@Yaxuan-w Yaxuan-w left a comment

Choose a reason for hiding this comment

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

I don't think this PR is ready for review. I see you've made some progress I mentioned on #135 . Can you push your changes to here once you finished modifications according to my feedbacks? @bblackheart013

Copy link
Member Author

Choose a reason for hiding this comment

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

Unintended modifications to this file should also be reverted

Copy link
Member Author

Choose a reason for hiding this comment

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

Unintended modifications to this file should also be reverted

Copy link
Member Author

Choose a reason for hiding this comment

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

Unintended modifications to this file should also be reverted

@Yaxuan-w Yaxuan-w marked this pull request as draft March 31, 2025 16:57
@Yaxuan-w Yaxuan-w changed the base branch from main to tmp-main March 31, 2025 19:35
@bblackheart013
Copy link

I’ve finished aligning all syscall numbers in lind_syscall_num.h with the official Linux syscall table (x86_64), based on the latest syscall_64.tbl. I double-checked each value line-by-line.

@Yaxuan-w
Copy link
Member Author

Yaxuan-w commented Apr 14, 2025

@bblackheart013 I had a quick look and wasn’t able to locate the commit 3ae0ce5 you mentioned in our DM. Also, just to reiterate what we’ve discussed (including in issue #135 and our previous chats, including last night): all changes should be pushed to the main codebase and reflected in this draft PR, so we can track and review everything in one place.

It also looks like the following files still contain some unintended changes. Could you double-check and clean them up?

src/glibc/sysdeps/unix/syscall-template.h
src/glibc/sysdeps/unix/sysv/linux/dup.c
src/glibc/sysdeps/unix/sysv/linux/fcntl.c
src/glibc/sysdeps/unix/sysv/linux/getdents.c
src/glibc/sysdeps/unix/sysv/linux/lseek.c
src/glibc/sysdeps/unix/sysv/linux/pipe.c
src/glibc/sysdeps/unix/sysv/linux/sigprocmask.c

As for modifying libc_sigaction and setitimer, here are the specific steps we agreed on, just re-sharing here to keep things clear:

For libc_sigaction:

Edit: src/glibc/sysdeps/unix/sysv/linux/i386/libc_sigaction.c
Add header around line 22
Replace 147 with SIGACTION_SYSCALL at line 113

And for setitimer:

Edit: src/glibc/sysdeps/unix/sysv/linux/setitimer.c
Add header around line 25
Replace 150 with SETITIMER_SYSCALL at line 31

Let me know if anything’s unclear. It’d be great to move these forward this week!

@Yaxuan-w
Copy link
Member Author

Yaxuan-w commented Apr 15, 2025

@bblackheart013 Great start! Changes to sigprocmask.c looks good now and align with reverting unintended file changes. Please keep finishing tasks I mentioned on previous comments. Lmk if there's anything unclear

@Yaxuan-w Yaxuan-w changed the title Syscall number unification in glibc [Refactor] Syscall number unification in glibc Apr 16, 2025
Copy link
Member Author

@Yaxuan-w Yaxuan-w left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks @xhanghe for picking up this task and finishing. The overall changes make sense to me. All comments I left are related to unnecessary space changes. Beside these, everything looks good to me.

}

int
chdir (const char *__path)
{
return MAKE_SYSCALL(130, "syscall|chdir", (uint64_t) __path, NOTUSED, NOTUSED, NOTUSED, NOTUSED, NOTUSED);
return MAKE_SYSCALL(CHDIR_SYSCALL, "syscall|chdir", (uint64_t) __path, NOTUSED, NOTUSED, NOTUSED, NOTUSED, NOTUSED);
Copy link
Contributor

Choose a reason for hiding this comment

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

let's delete the entire body of chdir and use an alias from __chdir

}

int dup (int fd) {
return MAKE_SYSCALL(24, "syscall|dup", (uint64_t) fd, NOTUSED, NOTUSED, NOTUSED, NOTUSED, NOTUSED);
return MAKE_SYSCALL(DUP_SYSCALL, "syscall|dup", (uint64_t) fd, NOTUSED, NOTUSED, NOTUSED, NOTUSED, NOTUSED);
Copy link
Contributor

Choose a reason for hiding this comment

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

same for dup here, let's use alias here instead of a duplicated definition

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder what's going on with this file, why an entire file is added for execve

Copy link
Member Author

Choose a reason for hiding this comment

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

Should replace this file:

src/glibc/sysdeps/unix/sysv/linux/i386/execve.c

}

int
kill (__pid_t a, int b)
{
return MAKE_SYSCALL(148, "syscall|kill", (uint64_t) a, (uint64_t) b, NOTUSED, NOTUSED, NOTUSED, NOTUSED);
return MAKE_SYSCALL(KILL_SYSCALL, "syscall|kill", (uint64_t) a, (uint64_t) b, NOTUSED, NOTUSED, NOTUSED, NOTUSED);
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, replace duplicated definition with alias

// return 0;
}

int munmap (void *addr, size_t len)
{
return MAKE_SYSCALL(22, "syscall|munmap", (uint64_t)(uintptr_t) addr, (uint64_t) len, NOTUSED, NOTUSED, NOTUSED, NOTUSED);
return MAKE_SYSCALL(MUNMAP_SYSCALL, "syscall|munmap", (uint64_t)(uintptr_t) addr, (uint64_t) len, NOTUSED, NOTUSED, NOTUSED, NOTUSED);
// return 0;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

same for munmap here

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you confirmed whether glibc can compile correctly with this change? This header file is under the lind_syscall folder, which might not be in the default header search path when building glibc. I might be misremembering, but just want to double-check that this is handled properly. You can verify by checking if, e.g, accept.o is generated correctly after running lindtool make_all.

Copy link
Contributor

@rennergade rennergade left a comment

Choose a reason for hiding this comment

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

This looks good stylewise for merging into the temp branch.

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.

6 participants