Fix exit_group test

[pwmarcz]

Description of the changes

This fixes #1937, but more importantly, makes sure that any thread calling exit() will cause the correct exit code to be reported.

The issue has been fixed by threads rework in #1949, this adds a test for exit() from a thread.

How to test this PR?

The exit_group test is a test for this scenario:

cd LibOS/shim/test/regression
make exit_group exit_group.manifest

# run the test:
../../../../Scripts/run-pytest -k test_401_exit_group -v

# trigger the binary manually:
./exit_group 3 100; echo $?
./pal_loader exit_group 3 100; echo $?

---

This change is 

[yamahata]

Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)

[boryspoplawski]

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @pwmarcz)

a discussion (no related file):
Today I'll submit a PR reworking threads (just waiting for #1938) which will get rid of these per thread exit codes and unfortunately remove all code introduced in this PR.
Now I know why I hit that LTP accept02 issue :)

We could merge this as temporary fix, but I'll revert all this changes anyway. What you guys think? cc @dimakuv @mkow @yamahata

---

[pwmarcz]

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv, @mkow, and @yamahata)

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

Today I'll submit a PR reworking threads (just waiting for #1938) which will get rid of these per thread exit codes and unfortunately remove all code introduced in this PR.
Now I know why I hit that LTP accept02 issue :)

We could merge this as temporary fix, but I'll revert all this changes anyway. What you guys think? cc @dimakuv @mkow @yamahata

No problem, the change is somewhat hacky anyway, but you can still use the test :)

---

[boryspoplawski]

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv, @mkow, and @yamahata)

a discussion (no related file):

Previously, pwmarcz (Paweł Marczewski) wrote…

No problem, the change is somewhat hacky anyway, but you can still use the test :)

Yeah, we will definitely keep the reworked test, the question is when do we merge it. I expect threads rework merge to be lengthy process, as the PR will be quite big.

---

[mkow]

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv and @yamahata)

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

Yeah, we will definitely keep the reworked test, the question is when do we merge it. I expect threads rework merge to be lengthy process, as the PR will be quite big.

@boryspoplawski Your call, I'm fine with both options. But if we decide to merge it, then I'd do it before submitting your changes, because they will conflict.

---

[boryspoplawski]

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv and @yamahata)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

@boryspoplawski Your call, I'm fine with both options. But if we decide to merge it, then I'd do it before submitting your changes, because they will conflict.

Given that it would be me, who needs to resolve the conflicts and that this is not a fatal issue, I would wait and merge just the test after threads PR :)

---

[dimakuv]

Reviewed 4 of 8 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @yamahata)

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

Given that it would be me, who needs to resolve the conflicts and that this is not a fatal issue, I would wait and merge just the test after threads PR :)

What about we drop the second commit from this PR (bar that skip of the accept02 LTP test)? And mark it as blocked until we merge Borys's PR (This PR must be rebased on top of ...).

---

[boryspoplawski]

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @yamahata)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What about we drop the second commit from this PR (bar that skip of the accept02 LTP test)? And mark it as blocked until we merge Borys's PR (This PR must be rebased on top of ...).

I guess we can live without this test (old version) in the meantime, so I'm ok with this

---

[boryspoplawski]

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @yamahata)

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

I guess we can live without this test (old version) in the meantime, so I'm ok with this

Threads PR was merged, this can be updated (just the test will remain, I guess?)

---

[pwmarcz]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @yamahata)

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

Threads PR was merged, this can be updated (just the test will remain, I guess?)

Done. I rebased the code on newest master and left only the test.

---

[dimakuv]

Reviewed 3 of 8 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @yamahata)

a discussion (no related file):

In addition, propagate the exit code from a forked process. That use case is actually broken and will be fixed in next commit.

I believe this para in the commit message should be removed (in the final rebase).

---

[mkow]

Reviewed 8 of 8 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @pwmarcz and @yamahata)

---

LibOS/shim/test/regression/exit_group.c, line 53 at r2 (raw file):

    pid_t pid = fork();
    if (fork < 0) {

Ouch. Are you testing here a function pointer for being negative? :P
Really weird that this didn't produce a warning...

[pwmarcz]

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @yamahata)

---

LibOS/shim/test/regression/exit_group.c, line 53 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Ouch. Are you testing here a function pointer for being negative? :P
Really weird that this didn't produce a warning...

Wow :) Yeah, crazy that this compiled without warnings. Fixed.

[mkow]

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @yamahata)

[mkow]

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @yamahata)

[dimakuv]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yamahata)

[yamahata]

Reviewed 2 of 8 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yamahata)

[boryspoplawski]

Reviewed 2 of 8 files at r2, 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved