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

Use unsafeWithForeignPtr in Data.ByteString.Unsafe #401

Merged
merged 3 commits into from
Jun 17, 2021

Conversation

Bodigrim
Copy link
Contributor

@Bodigrim Bodigrim added this to the 0.11.2.0 milestone Jun 10, 2021
@Bodigrim Bodigrim requested a review from sjakobi June 10, 2021 18:28
Copy link
Member

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

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

These instances do seem good to convert to the unsafe version. There's still a bunch of withForeignPtrs in the library though, e.g. in BS.split. Did you or @bgamari make a complete pass over these?

In functions like findIndex and filter, I suspect we have to stick with withForeignPtr because the user-defined predicates can't be assumed to be safe?!

@Bodigrim
Copy link
Contributor Author

As far as I understand from https://gitlab.haskell.org/ghc/homepage/-/merge_requests/84/diffs, since unsafeWithForeignPtr is equivalent to pre-9.0 withForeignPtr, and bytestring was doing just fine with the latter, we can make all instances unsafe.

@bgamari in b82da95 does not hesitate to use unsafe version even for functions with callbacks such as foldl'. It is dangerous to use unsafe, if a callback can be divergent, but useful at the same time (e. g., does something in IO, like forever $ ...). But if a callback is a pure function, throwing error, it probably does not make much difference to touch or not to touch. @bgamari any chance to chime in and clarify our understanding, please?

@bgamari
Copy link
Contributor

bgamari commented Jun 15, 2021

@bgamari in b82da95 does not hesitate to use unsafe version even for functions with callbacks such as foldl'. It is dangerous to use unsafe, if a callback can be divergent, but useful at the same time (e. g., does something in IO, like forever $ ...). But if a callback is a pure function, throwing error, it probably does not make much difference to touch or not to touch. @bgamari any chance to chime in and clarify our understanding, please?

Sure. Let's look at the case of foldl':

foldl' :: (a -> Word8 -> a) -> a -> ByteString -> a
foldl' f v (BS fp len) =
    accursedUnutterablePerformIO $ unsafeWithForeignPtr fp g
  where
    g ptr = go v ptr
      where
        end  = ptr `plusPtr` len
        -- tail recursive; traverses array left to right
        go !z !p | p == end  = return z
                 | otherwise = do x <- peek p
        go (f z x) (p `plusPtr` 1)

Note how go does work (namely predicating on p == end) before applying the user-supplied f. Consequently, it is impossible for GHC to conclude that go unconditionally diverges, even if f does.

In general, the question to ask yourself in these situations is:

can the user provide inputs to the function that would cause it to diverge unconditionally.

@Bodigrim
Copy link
Contributor Author

@bgamari thanks, now it is much clearer.
@sjakobi I checked other encounters of withForeignPtr and pushed a patch, please take a look.

Copy link
Member

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

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

One small suggestion. LGTM apart from that.

Data/ByteString.hs Outdated Show resolved Hide resolved
@Bodigrim Bodigrim merged commit 36b9fde into haskell:master Jun 17, 2021
@Bodigrim Bodigrim deleted the more-unsafeWithForeignPtr branch June 17, 2021 22:34
Bodigrim added a commit to Bodigrim/bytestring that referenced this pull request Jul 1, 2021
* Use unsafeWithForeignPtr in Data.ByteString.Unsafe

* Review remaining cases of withForeignPtr

* Review suggestions
noughtmare pushed a commit to noughtmare/bytestring that referenced this pull request Dec 12, 2021
* Use unsafeWithForeignPtr in Data.ByteString.Unsafe

* Review remaining cases of withForeignPtr

* Review suggestions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants