add poll syscall

[tgrez]

This change is 

[nh2]

Hi, cool!

Which part of it is WIP, are there things you want to finish about it or would you like feedback etc?

[tgrez]

It was marked as WIP, as I intended to add poll and ppoll in a single pull request. This will, however, take longer than I expected, and I won't have time to finish this before September.

The work for poll syscall is done, except maybe some whitespace corrections, and is ready for review & merge after that.

[tgrez]

ready for review and merge

[qrilka]

Reviewed 1 of 7 files at r1, 3 of 4 files at r2.
Reviewable status: 4 of 7 files reviewed, 4 unresolved discussions (waiting on @tgrez)

---

src/System/Hatrace.hs, line 1556 at r2 (raw file):

              enterDetail@SyscallEnterDetails_poll{ fds, nfds } -> do
                let convertedNfds :: (Convert.ConvertResult Int) = Convert.safeConvert nfds
                let n = fromRight (maxBound :: Int) convertedNfds

I'm not quite sold on adding convertible as a dependency at least for this use case: we just cap CULong value by maxBound::Int why can't we just do such a comparison or just do this cap using max?

---

src/System/Hatrace.hs, line 1557 at r2 (raw file):

                let convertedNfds :: (Convert.ConvertResult Int) = Convert.safeConvert nfds
                let n = fromRight (maxBound :: Int) convertedNfds
                pollfds <- System.Hatrace.peekArray (TracedProcess pid) n fds

Do we need a fully qualified name here?

---

src/System/Hatrace.hs, line 1573 at r2 (raw file):

peekElemOff :: Storable a => TracedProcess -> Ptr a -> Int -> IO a
peekElemOff proc addr offset = do
  result <- Ptrace.peekBytes proc (addr `plusPtr` (offset * size)) size

why do we need to do peekBytes for every element? Won't it be better to do 1 call of peekBytes and then just do Foreign.Marshal.Array.peekArray?

---

src/System/Hatrace/Types.hsc, line 217 at r2 (raw file):

              [ "POLLERR" | pollerr gpe ] ++
              [ "POLLHUP" | pollhup gpe ] ++
              [ "POLLNVAL" | pollnval gpe ] ++

what about POLLRDNORM and others available when _XOPEN_SOURCE is defined? At least we could have a haddock telling that we don't support them (yet?)

[qrilka]

Reviewed 2 of 7 files at r1, 1 of 4 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @tgrez)

[tgrez]

Reviewable status: 4 of 6 files reviewed, 4 unresolved discussions (waiting on @qrilka)

---

src/System/Hatrace.hs, line 1556 at r2 (raw file):

Previously, qrilka (Kirill Zaborsky) wrote…

I'm not quite sold on adding convertible as a dependency at least for this use case: we just cap CULong value by maxBound::Int why can't we just do such a comparison or just do this cap using max?

I was looking for general solution for converting numbers, but I just got convinced that it's not necessary in here :) Fixed.

---

src/System/Hatrace.hs, line 1557 at r2 (raw file):

Previously, qrilka (Kirill Zaborsky) wrote…

Do we need a fully qualified name here?

No. Fixed.

---

src/System/Hatrace.hs, line 1573 at r2 (raw file):

Previously, qrilka (Kirill Zaborsky) wrote…

why do we need to do peekBytes for every element? Won't it be better to do 1 call of peekBytes and then just do Foreign.Marshal.Array.peekArray?

This approach does indeed sound better, so I already changed the implementation.

---

src/System/Hatrace/Types.hsc, line 217 at r2 (raw file):

Previously, qrilka (Kirill Zaborsky) wrote…

what about POLLRDNORM and others available when _XOPEN_SOURCE is defined? At least we could have a haddock telling that we don't support them (yet?)

I guess I just ignored those initially, as those constants are described on the man page as: "which convey no further information beyond the bits listed above". There is also a mention of POLLMSG, but: "Linux also knows about, but does not use POLLMSG". In header files I could also see others, eg POLLREMOVE. For now, I added POLLRDNORM and similar, while skipping those which are not explicitly defined in man page. If you think it would be better to include them, I will do that.

[qrilka]

Reviewed 5 of 6 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tgrez)

---

src/System/Hatrace/Types.hsc, line 217 at r2 (raw file):

Previously, tgrez (Tomasz Guz) wrote…

I guess I just ignored those initially, as those constants are described on the man page as: "which convey no further information beyond the bits listed above". There is also a mention of POLLMSG, but: "Linux also knows about, but does not use POLLMSG". In header files I could also see others, eg POLLREMOVE. For now, I added POLLRDNORM and similar, while skipping those which are not explicitly defined in man page. If you think it would be better to include them, I will do that.

I think it should be good enough to add some general note like "only explicitly defined in man pages poll bits are currently used". With something like that in place this PR LGTM.

[qrilka]

Reviewable status: complete! all files reviewed, all discussions resolved

[qrilka]

Sorry for late reply @tgrez - was sick a bit. BTW could you rebase the branch from the master so we could merge it giving us linear history if @nh2 doesn't have any further comments on this PR?

[qrilka]

I mean after adding that poll bits note I mentioned

[tgrez]

Where should I add this note "only explicitly..."? Is it enough to add a haddoc comment for GranularPollEvents?

[qrilka]

Yes, that sounds like the best place to me, thanks.

[tgrez]

Reviewable status: 3 of 6 files reviewed, all discussions resolved (waiting on @qrilka)

---

src/System/Hatrace/Types.hsc, line 217 at r2 (raw file):

Previously, qrilka (Kirill Zaborsky) wrote…

I think it should be good enough to add some general note like "only explicitly defined in man pages poll bits are currently used". With something like that in place this PR LGTM.

The note has been added.

[qrilka]

Reviewed 3 of 3 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved

[qrilka]

thanks @tgrez
@nh2 didn't you also want to have a quick look at this one?

[nh2]

Reviewed 1 of 7 files at r1, 2 of 6 files at r3, 1 of 3 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @tgrez)

---

src/System/Hatrace.hs, line 1587 at r4 (raw file):

            DetailedSyscallEnter_poll
              enterDetail@SyscallEnterDetails_poll{ fds, nfds } -> do
                let n = fromIntegral $ min nfds $ fromIntegral (maxBound :: Int)

Can you put a comment what the idea behind this capping to max Int here is?

Is it an overflow protection? If yes, for which scenario?

---

src/System/Hatrace/Types.hsc, line 310 at r4 (raw file):

              [ "POLLPRI" | pollpri gpe ] ++
              [ "POLLOUT" | pollout gpe ] ++
#ifdef __USE_GNU

Can Haskell's CPP actually define this macro?

If yes, please put a comment that explains it.

---

test/HatraceSpec.hs, line 695 at r4 (raw file):

                                    { enterDetail = SyscallEnterDetails_poll{ nfds }, pollfds })
                           ) <- events
                         ]

Mini style request:

My preference is to make indendation independent of variable lengths, e.g. putting the stuff after teh pollResult = on the next line. That keeps diffs small when names are changed.

I don't want to bother you with it in this great PR though, so feel free to ignore it and I'll do such style changes separately.

[nh2]

This looks great; I put 2 small questions for things I didn't understand / requests for comments.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @tgrez)

[tgrez]

The capping to max int comes from the var nfds being a long, while peekArray taking as an argument just an int. The assumption I made in here is that the number of checked fds will be less than max int.

Should this explanation be added as a comment in code?

I will check the issue with macro once I'm back from holidays ;)

[nh2]

Should this explanation be added as a comment in code?

Yes that would be great.

I will check the issue with macro once I'm back from holidays ;)

Have a nice time!

[tgrez]

Reviewable status: 4 of 6 files reviewed, 3 unresolved discussions (waiting on @nh2, @qrilka, and @tgrez)

---

src/System/Hatrace.hs, line 1587 at r4 (raw file):

Previously, nh2 (Niklas Hambüchen) wrote…

Can you put a comment what the idea behind this capping to max Int here is?

Is it an overflow protection? If yes, for which scenario?

Done. Comment added.

---

src/System/Hatrace/Types.hsc, line 310 at r4 (raw file):

Previously, nh2 (Niklas Hambüchen) wrote…

Can Haskell's CPP actually define this macro?

If yes, please put a comment that explains it.

Nice catch! This macro was invalid in here. I just checked poll man and there was _GNU_SOURCE macro mentioned, so I was confused why did I put __USE_GNU in here. Most probably I took it from bits/poll.h, as it is present there, but according to https://stackoverflow.com/questions/7296963/gnu-source-and-use-gnu only _GNU_SOURCE should be used from our code.

[nh2]

Reviewed 1 of 3 files at r4, 2 of 2 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tgrez)

---

src/System/Hatrace/Types.hsc, line 310 at r4 (raw file):

Previously, tgrez (Tomasz Guz) wrote…

Nice catch! This macro was invalid in here. I just checked poll man and there was _GNU_SOURCE macro mentioned, so I was confused why did I put __USE_GNU in here. Most probably I took it from bits/poll.h, as it is present there, but according to https://stackoverflow.com/questions/7296963/gnu-source-and-use-gnu only _GNU_SOURCE should be used from our code.

I am still a bit confused by that:

My understanding (from man 2 poll and man feature_test_macros) is that _GNU_SOURCE is a feature-test macro that you have to set before the import of poll.h, and if you do, you get the POLLRDHUP constant into scope by the import.

See https://stackoverflow.com/questions/18207977/why-do-we-need-to-define-gnu-source-macro-before-using-pollrdhup-event-flag-in

My understanding is also that it's not a macro you can use whether something is supported, that means you can't do #ifdef _GNU_SOURCE -- it will always evaluate to false; so I think these ifdefs are dead code (please let me know if this understanding is not correct).

Probably the way this has to be done is that we have to check whether it's Linux in the Cabal file, if yes, set a CPP macro, and if that macro is true, set _GNU_SOURCE before the import of poll.h. (So that for example on OSX, we would not use the POLLRDHUP constant because there it shouldn't exist.)

[tgrez]

Reviewable status: 3 of 7 files reviewed, 2 unresolved discussions (waiting on @nh2, @qrilka, and @tgrez)

---

src/System/Hatrace/Types.hsc, line 310 at r4 (raw file):

Previously, nh2 (Niklas Hambüchen) wrote…

I am still a bit confused by that:

My understanding (from man 2 poll and man feature_test_macros) is that _GNU_SOURCE is a feature-test macro that you have to set before the import of poll.h, and if you do, you get the POLLRDHUP constant into scope by the import.

See https://stackoverflow.com/questions/18207977/why-do-we-need-to-define-gnu-source-macro-before-using-pollrdhup-event-flag-in

My understanding is also that it's not a macro you can use whether something is supported, that means you can't do #ifdef _GNU_SOURCE -- it will always evaluate to false; so I think these ifdefs are dead code (please let me know if this understanding is not correct).

Probably the way this has to be done is that we have to check whether it's Linux in the Cabal file, if yes, set a CPP macro, and if that macro is true, set _GNU_SOURCE before the import of poll.h. (So that for example on OSX, we would not use the POLLRDHUP constant because there it shouldn't exist.)

From what I have checked, your understanding is correct. I changed the poll.c program, so it includes POLLRDHUP event and hatrace decodes it properly only if we define the _GNU_SOURCE macro ourselves.

So I implemented it the way you proposed: check the OS in the Cabal file and set _GNU_SOURCE if on Linux. I also changed the test to check it.

[nh2]

Reviewed 4 of 4 files at r6.
Reviewable status: 3 of 7 files reviewed, 1 unresolved discussion (waiting on @nh2 and @tgrez)

---

src/System/Hatrace/Types.hsc, line 310 at r4 (raw file):

Previously, tgrez (Tomasz Guz) wrote…

From what I have checked, your understanding is correct. I changed the poll.c program, so it includes POLLRDHUP event and hatrace decodes it properly only if we define the _GNU_SOURCE macro ourselves.

So I implemented it the way you proposed: check the OS in the Cabal file and set _GNU_SOURCE if on Linux. I also changed the test to check it.

That looks good. I have pushed another small commit into the PR that makes it possible to control the POLLRDHUP feature explicitly, and added some comments.

[nh2]

Reviewable status: 3 of 7 files reviewed, 1 unresolved discussion (waiting on @nh2 and @tgrez)

[nh2]

Ready from my side; would like anybody to review the last change I made (and we need to check why the CI failed with exit code 11).

[nh2]

and we need to check why the CI failed with exit code 11

Rerunning it fixed it.

[qrilka]

Reviewed 1 of 2 files at r5, 4 of 4 files at r7.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tgrez)

[nh2]

Reviewable status: complete! all files reviewed, all discussions resolved

[nh2]

Thanks for this @tgrez! 👍