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.

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

Jump to bottom

os/StartProcess #4377

Open
wants to merge 24 commits into
base: dev
Choose a base branch
from
Open

os/StartProcess #4377

wants to merge 24 commits into from

Conversation

leongross
Copy link
Contributor

This is a rework of #4323 (based on that branch) to move the Execve and Fork wrappers to the os package to circumvent the goroot overrides.

@leongross leongross force-pushed the os/StartProcessOs branch 6 times, most recently from 43fcf8f to 8cce6c2 Compare August 2, 2024 12:06
aykevl
aykevl reviewed Aug 2, 2024
View reviewed changes
Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

Quick initial (incomplete) review

src/os/osexec.go Outdated Show resolved Hide resolved
src/os/osexec.go Outdated Show resolved Hide resolved
src/os/exec_other.go Show resolved Hide resolved
src/os/exec_linux.go Outdated Show resolved Hide resolved
@leongross leongross requested a review from aykevl August 5, 2024 09:21
@leongross
Copy link
Contributor Author

leongross commented Aug 5, 2024 •
edited
Loading

main_test.go:394: /home/runner/work/tinygo/tinygo/src/os/osexec.go:14:23: undefined: syscall.Syscall

ci fails with undefined syscall.Syscall. I thought since it is just a package the compiler would pick that up when building it. @aykevl

EDIT: it looks like this is only happening on architectures that the createRawSyscall function does not target, which makes sense, namely EmulatedCortexM3 : cortex-m-qemu and EmulatedRISCV : riscv-qemu. Anyways, this should not happen.
https://github.com/tinygo-org/tinygo/blob/dev/main_test.go#L522

Anyhow the smoke tests fail without any further information of what went wrong on linux, windows and mac. the smoke test cannot be removed, it is not clear though why it wasn't build in the first place

rm: cannot remove 'smoke.test': No such file or directory

@leongross leongross marked this pull request as ready for review August 6, 2024 12:39
@leongross
Copy link
Contributor Author

leongross commented Aug 8, 2024 •
edited
Loading

Not sure if something like #4398 would fix this? @aykevl

@aykevl
Copy link
Member

aykevl commented Aug 8, 2024

Not sure if something like #4398 would fix this? @aykevl

No, that's the wrong approach. If syscall.Syscall fails on baremetal systems, then the solution is not to implement these syscalls (because they're baremetal, so there is no OS to call into), but rather to figure out why these calls are made.

aykevl
aykevl reviewed Aug 8, 2024
View reviewed changes
src/os/osexec.go Outdated Show resolved Hide resolved
@leongross leongross requested a review from aykevl August 8, 2024 14:57
@leongross leongross force-pushed the os/StartProcessOs branch 3 times, most recently from e369ae7 to bf011fe Compare August 9, 2024 10:19
@leongross leongross force-pushed the os/StartProcessOs branch 5 times, most recently from bbf1159 to ffa5a87 Compare August 22, 2024 10:41
leongross
leongross commented Aug 22, 2024
View reviewed changes
src/os/tempfile.go Outdated Show resolved Hide resolved
@leongross
Copy link
Contributor Author

@deadprogram @dgryski @aykevl build and tests finally succeed, would appreciate another review.

@leongross leongross changed the title [rework] os/SartProcess os/SartProcess Aug 28, 2024
@leongross
Copy link
Contributor Author

@aykevl @deadprogram @dgryski would someone mind reviewing?

deadprogram
deadprogram reviewed Sep 6, 2024
View reviewed changes
src/os/osexec.go Outdated Show resolved Hide resolved
@deadprogram deadprogram changed the title os/SartProcess os/StartProcess Sep 6, 2024
@leongross
os rework
8fd1ba4
@leongross
adjust build tags, un-export fork and execve, add signals for windows…
1faa457 
… and macos

Signed-off-by: leongross <[email protected]>
@leongross
update wrapper comment
8494457
@leongross
exclude baremetal targets
c9703df
@leongross
test: remove os.Getpid in seed generation on baremetal devices
b33e04f
@leongross
remove os/signal from baremetal
44bc700
@leongross
adjust baremetal build tags
c037cea 
Signed-off-by: leongross <[email protected]>
@leongross
remove old comments
e30e603
@leongross
os/StartProcess: cleanup
f975bbf 
remove old comments, replace explicit syscall numbers with named
variants, undo tempfile seed generation, remove old // +build tags,
improve StartProcess documentation, remove custom syscall number
definitions.

Signed-off-by: leongross <[email protected]>
@leongross
os/exec: exclude aarch64
7f7559c 
aarch64 has no support for the fork ssycall. for now exclude it.

Signed-off-by: leongross <[email protected]>
@leongross
add fallback stub for aarch64
fb71295
@leongross
add syscall error handling
6c6f254 
Signed-off-by: leongross <[email protected]>
@leongross
change build tags aarch64 -> arm64
fdf44d8 
Signed-off-by: leongross <[email protected]>
@leongross
add signal implementations for linux arm64
be48fa5
@leongross
wip
9b8dd29
@leongross
update TestForkExec with invalid testcase
6d74b9d
@leongross
remove comments, remove unused/obsolete files
61267ad 
Signed-off-by: leongross <[email protected]>
@leongross
replace raw syscall implementation with musl wrapper
70bfa96 
Signed-off-by: leongross <[email protected]>
@leongross
remove arm64 handling
5878a4b 
Signed-off-by: leongross <[email protected]>
@leongross
update musl builder to include arch specific process files
706477a 
Signed-off-by: leongross <[email protected]>
@leongross
Copy link
Contributor Author

@aykevl

b0ch3nski
b0ch3nski reviewed Nov 2, 2024
View reviewed changes
src/os/exec_linux.go Outdated Show resolved Hide resolved
aykevl
aykevl reviewed Nov 4, 2024
View reviewed changes
Copy link
Member

@aykevl aykevl 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 few small things, otherwise this PR seems ready for merging.

src/os/exec_linux.go
return 0, errors.New("exec: no argv")
}

if len(argv) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: with this check, the previous check is unnecessary. len(argv) of a nil slice is always 0.

src/os/os.test Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This file needs to be removed.

src/os/osexec_aarch64.go Outdated
@@ -0,0 +1,18 @@
//go:build linux && !baremetal && !darwin && !tinygo.wasm && aarch64
Copy link
Member

Choose a reason for hiding this comment

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

aarch64 is not a valid Go build tag.

I think this file can be removed entirely now, since these things are implemented in osexec.go?

aykevl
aykevl reviewed Nov 4, 2024
View reviewed changes
src/os/osexec.go Outdated
//go:build linux && !baremetal && !tinygo.wasm

// arm64 does not have a fork syscall, so ignore it for now
// TODO: add support for arm64 with clone or use musl implementation
Copy link
Member

Choose a reason for hiding this comment

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

This comment needs to be updated or removed since it is outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aykevl aykevl aykevl left review comments

@deadprogram deadprogram Awaiting requested review from deadprogram

@b0ch3nski b0ch3nski Awaiting requested review from b0ch3nski

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@leongross @aykevl @deadprogram @b0ch3nski