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

tests/valgrind: fix handling of forks #509

Merged
merged 1 commit into from
Jan 26, 2024
Merged

tests/valgrind: fix handling of forks #509

merged 1 commit into from
Jan 26, 2024

Conversation

gperciva
Copy link
Member

No description provided.

@gperciva
Copy link
Member Author

gperciva commented Jan 15, 2024

We tried to support valgrind checking of fork() in:

2023-09-20 tests/valgrind: improve handling for fork() and exec()
38e07b4

However, that method assumed that the first pid would be the parent, while the second (and subsequent ones) would be the children. That has two flaws:

  1. if the pid rolls over, the order would be broken. For example, if
    the parent pid was 99999 on FreeBSD (the maximum value), then the
    child would be something much lower (probably in the hundreds).

    (I knew about that problem, considered it sufficiently unlikely.)

  2. since $_valgrind_check_logfiles are defined by ls "blah"*, they are
    listed in lexicographical order, not numerical order. For example,
    given two logfiles foo-9999.log and foo-10000.log, foo-10000.log
    would come first.

    (I did not anticipate this problem, but even if it had occurred to
    me, I probably would have deemed it sufficiently unlikely. However,
    by an unlikely fluke, this seems to occur roughly 50% of the time with
    github actions: when checking 24-fork-func with the gcc binaries, the parent
    lands on pid 9999 exactly and the child is 10000.)

The fix is not to rely on the order: instead, generate a list of all pids involved. If a valgrind logfile lists a parent pid which is in that list, it's a child; otherwise, it's the parent.

@gperciva gperciva marked this pull request as draft January 15, 2024 21:00
@gperciva gperciva marked this pull request as ready for review January 15, 2024 21:01
@gperciva gperciva marked this pull request as draft January 15, 2024 21:16
We tried to support valgrind checking of fork() in:
    2023-09-20 tests/valgrind: improve handling for fork() and exec()
    38e07b4

However, that method assumed that the lowest pid would be the parent,
while the higher one(s) would be the children.  That has two flaws:

1) If the pid rolls over, the order would be broken.  For example, if
   the parent pid was 99999 on FreeBSD (the maximum value), then the
   child would be something much lower (probably in the hundreds).

   (I knew about that problem, considered it sufficiently unlikely.)

2) Since $_valgrind_check_logfiles are defined by `ls "blah"*`, they are
   listed in lexicographical order, not numerical order.  For example,
   given two logfiles foo-9999.log and foo-10000.log, foo-10000.log
   would come first.

   (I did not anticipate this problem, but even if it had occurred to
   me, I probably would have deemed it sufficiently unlikely.  However,
   by unlikely fluke, this seems to occur roughly 50% of the time with
   github actions: when checking 24-fork-func with the gcc binaries, the
   parent lands on pid 9999 exactly and the child is 10000.)

The fix is not to rely on the order: instead, generate a list of all
pids involved.  If a valgrind logfile lists a parent pid which is in
that list, it's a child; otherwise, it's the parent.
@gperciva gperciva marked this pull request as ready for review January 15, 2024 22:29
@cperciva cperciva merged commit 4511e76 into master Jan 26, 2024
2 checks passed
@gperciva gperciva deleted the fix-valgrind-fork branch January 26, 2024 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants