os/StartProcess #4377

leongross · 2024-08-02T11:11:15Z

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.

aykevl

Quick initial (incomplete) review

src/os/osexec.go

src/os/exec_other.go

src/os/exec_linux.go

leongross · 2024-08-05T15:24:09Z

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 · 2024-08-08T11:00:40Z

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

aykevl · 2024-08-08T12:21:49Z

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.

src/os/osexec.go

Signed-off-by: leongross <[email protected]>

… and macos Signed-off-by: leongross <[email protected]>

Signed-off-by: leongross <[email protected]>

src/os/tempfile.go

leongross · 2024-08-22T12:39:45Z

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

leongross · 2024-08-29T16:07:16Z

@aykevl @deadprogram @dgryski would someone mind reviewing?

src/os/osexec.go

aykevl

There is some commented out code, that looks like testing code. IMHO it should either be used, or removed (it doesn't really add any value in documentation).
You make a few changes to the syscall package, but why? All actual operating systems (Linux, macOS, Windows) use the default syscall package. If you need any changes to the syscall package, that probably indicates that it is called from somewhere (for example, baremetal) and you need to adjust build tags.

src/os/exec.go

src/syscall/syscall_unix.go

src/os/tempfile.go

src/syscall/syscall_unix.go

src/os/osexec.go

src/os/exec_other.go

aykevl · 2024-09-11T10:39:39Z

src/os/osexec.go

+
+	// fail := libc_execve(&argv0[0], &argv1[0], &env1[0])
+	// fail, _, _ := syscall.Syscall(syscall.SYS_EXECVE, uintptr(unsafe.Pointer(&argv0[0])), uintptr(unsafe.Pointer(&argv1[0])), uintptr(unsafe.Pointer(&env1[0])))
+	fail, _, _ := syscall.Syscall(59, uintptr(unsafe.Pointer(&argv0[0])), uintptr(unsafe.Pointer(&argv1[0])), uintptr(unsafe.Pointer(&env1[0])))


Please don't use magic numbers. It looks like you can use syscall.Exec instead.

src/os/osexec.go

aykevl · 2024-09-11T10:51:27Z

src/os/exec_linux.go

+// * No setting of Process Attributes
+// * Ignoring Ctty
+// * No ForkLocking (might be introduced by #4273)
+// * No parent-child communication via pipes (TODO)
+// * No waiting for crashes child processes to prohibit zombie process accumulation / Wait status checking (TODO)


Is it possible to check for these cases, and if users use any of these to return an error instead of continuing and not doing what the user expected?
For example, if Stdin is set, it should either implement that feature or return an error.

That sounds like a reasonable thing to do. I will evaluate this and see how/if this is possible.

aykevl · 2024-09-11T10:54:23Z

src/os/exec.go

+func (p *ProcessState) Exited() bool {
+	return false // TODO
+}


That's... a little ugly.
Is there any way we can track whether the process is still running?

For now, I stubbed it out the same way all the other methods over ProcessState (which is an empty struct) are implemented. Making this functional would mean a quite large rework of the tinygo process representation, which atm is just a pid. I think it is a good idea to rework this, but not as part of this PR.

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]>

deadprogram · 2024-09-17T15:25:06Z

@leongross still are some CI failures from this PR is would appear.

leongross · 2024-10-04T09:47:09Z

I figured out the error here: there is no fork syscall on arm64 1. The original golang implementation abstracts this better and does not do raw syscalls as my implementation does. So for now, arm64 has to be excluded as a possible target architecture for this.

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

leongross force-pushed the os/StartProcessOs branch 6 times, most recently from 43fcf8f to 8cce6c2 Compare August 2, 2024 12:06

aykevl reviewed Aug 2, 2024

View reviewed changes

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 force-pushed the os/StartProcessOs branch from fffffce to 0844543 Compare August 5, 2024 08:45

leongross requested a review from aykevl August 5, 2024 09:21

leongross marked this pull request as ready for review August 6, 2024 12:39

aykevl reviewed Aug 8, 2024

View reviewed changes

src/os/osexec.go Outdated Show resolved Hide resolved

leongross force-pushed the os/StartProcessOs branch from 4189be1 to a67e057 Compare August 8, 2024 12:56

leongross requested a review from aykevl August 8, 2024 14:57

leongross force-pushed the os/StartProcessOs branch 3 times, most recently from e369ae7 to bf011fe Compare August 9, 2024 10:19

leongross added 10 commits August 22, 2024 09:27

add signal stubs in runtime

b0d8477

Signed-off-by: leongross <[email protected]>

add StartProcess functionality (fix)

abef873

renaming

6972705

os rework

fec9251

adjust build tags, un-export fork and execve, add signals for windows…

43fce0f

… and macos Signed-off-by: leongross <[email protected]>

remove signal functionalities

bb4a950

update wrapper comment

84f211f

exclude baremetal targets

1ae8699

test: remove os.Getpid in seed generation on baremetal devices

3ee2472

remove os/signal from baremetal

459b1e1

leongross force-pushed the os/StartProcessOs branch from bf011fe to 318e7b7 Compare August 22, 2024 07:27

leongross force-pushed the os/StartProcessOs branch 3 times, most recently from 246b809 to bbf1159 Compare August 22, 2024 09:40

adjust baremetal build tags

ffa5a87

Signed-off-by: leongross <[email protected]>

leongross force-pushed the os/StartProcessOs branch from bbf1159 to ffa5a87 Compare August 22, 2024 10:41

leongross commented Aug 22, 2024

View reviewed changes

src/os/tempfile.go Outdated Show resolved Hide resolved

leongross changed the title ~~[rework] os/SartProcess~~ os/SartProcess Aug 28, 2024

deadprogram reviewed Sep 6, 2024

View reviewed changes

src/os/osexec.go Outdated Show resolved Hide resolved

deadprogram changed the title ~~os/SartProcess~~ os/StartProcess Sep 6, 2024

remove old comments

404661d

aykevl reviewed Sep 11, 2024

View reviewed changes

leongross requested review from deadprogram and aykevl September 17, 2024 12:09

os/StartProcess: cleanup

79ecb29

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 force-pushed the os/StartProcessOs branch from 17d1525 to 79ecb29 Compare September 17, 2024 12:12

leongross mentioned this pull request Sep 17, 2024

os/exec: Process rework #4471

Open

os/exec: exclude aarch64

5dc63dc

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

leongross force-pushed the os/StartProcessOs branch from 978709f to 5dc63dc Compare October 4, 2024 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

os/StartProcess #4377

os/StartProcess #4377

leongross commented Aug 2, 2024

aykevl left a comment

leongross commented Aug 5, 2024 •

edited

Loading

leongross commented Aug 8, 2024 •

edited

Loading

aykevl commented Aug 8, 2024

leongross commented Aug 22, 2024

leongross commented Aug 29, 2024

aykevl left a comment

aykevl Sep 11, 2024

aykevl Sep 11, 2024

leongross Sep 11, 2024

aykevl Sep 11, 2024

leongross Sep 17, 2024 •

edited

Loading

deadprogram commented Sep 17, 2024

leongross commented Oct 4, 2024

os/StartProcess #4377

Are you sure you want to change the base?

os/StartProcess #4377

Conversation

leongross commented Aug 2, 2024

aykevl left a comment

Choose a reason for hiding this comment

leongross commented Aug 5, 2024 • edited Loading

leongross commented Aug 8, 2024 • edited Loading

aykevl commented Aug 8, 2024

leongross commented Aug 22, 2024

leongross commented Aug 29, 2024

aykevl left a comment

Choose a reason for hiding this comment

aykevl Sep 11, 2024

Choose a reason for hiding this comment

aykevl Sep 11, 2024

Choose a reason for hiding this comment

leongross Sep 11, 2024

Choose a reason for hiding this comment

aykevl Sep 11, 2024

Choose a reason for hiding this comment

leongross Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

deadprogram commented Sep 17, 2024

leongross commented Oct 4, 2024

leongross commented Aug 5, 2024 •

edited

Loading

leongross commented Aug 8, 2024 •

edited

Loading

leongross Sep 17, 2024 •

edited

Loading