∞-loop, space==bound in primMapByteStringBounded #204

vdukhovni · 2020-02-23T04:47:25Z

In primMapByteStringBounded, when the potentially needed space for
encoding one more byte (the 'bound' of the provided BoundedPrim)
is exactly equal to the free space in the output buffer, we must
fill the buffer, rather than signal BufferFull.

Otherwise, when we ask for "bounded" more bytes, no new space
is allocated, and an infinite loop follows (at least when the output is
via the direct Builder hPut).

Code that was freezing without the change, runs fine with the test
condition updated to <=

In primMapByteStringBounded, when the potentially needed space for encoding one more byte (the 'bound' of the provided BoundedPrim) is exactly equal to the free space in the output buffer, we must fill the buffer, rather than signal BufferFull. Otherwise, when we ask for "bounded" more bytes, no new space is allocated, and an infinite loop follows.

vdukhovni · 2020-02-24T05:39:05Z

What's the next step? Is there anything I need to do?

sjakobi · 2020-02-24T14:16:37Z

What's the next step? Is there anything I need to do?

Since @hvr has already expressed his desire for this fix in #117 I believe he or @dcoutts will accept and merge this PR quickly once they become aware of it. Since GitHub @-mentions seem to be rather ineffective means to catch their attention, you may want to try reaching out to them via other channels like IRC or email.

Two things might be nice to have (from you), but are not for me to decide:

A changelog entry.
A regression test.

vdukhovni · 2020-03-06T06:52:44Z

@hvr and/or @dcoutts what is the next step to get this simple fix merged?

vdukhovni · 2020-05-02T10:10:59Z

@hvr and/or @dcoutts what is the next step to get this simple fix merged?

I keep encountering infinite loops in fairly naive uses of ByteString builders. This is a critical bug. A new release is required. Any chance this can happen soon?

Cc: @cartazio

cartazio · 2020-05-04T13:46:53Z

@vdukhovni your PR doesn't build on every version of the ci build matrix

sjakobi · 2020-05-04T13:50:29Z

@cartazio CI is simply broken right now. See #195 and #196.

vdukhovni · 2020-05-04T17:27:22Z

@vdukhovni your PR doesn't build on every version of the ci build matrix

There are bugs in the test suite: findSubstrings sometimes fails with:

findSubstrings : [Failed]
*** Failed! Falsified (after 66 tests and 6 shrinks):
"\DLE\156176"
0
1
(used seed -1356285094314398714)

The master branch sometimes fails with (not at all surprising :-) infinite loop in the builder tests!
And there's an issue with encodings in an hPutBuilder test (found in the GHC 7.2.2 branch), perhaps the output stream was not in binary mode.

Data.ByteString.Builder:
  toLazyByteStringWith: [OK, passed 100 tests]
  hPutBuilder: [Failed]
*** Failed! Exception: '/tmp/TestBuilder7351: commitBuffer: invalid argument (invalid character)' (after 99 tests and 22 shrinks):
(UnicodeString {getUnicodeString = ""},UnicodeString {getUnicodeString = "\61376"},UnicodeString {getUnicodeString = ""},Recipe Safe 0 0 "" [])
(used seed 7794270791837805650)

None of this is the result of my PR, which actually builds much cleaner than master.

vdukhovni · 2020-05-05T01:35:31Z

There are bugs in the test suite: findSubstrings sometimes fails with:
findSubstrings : [Failed]
*** Failed! Falsified (after 66 tests and 6 shrinks):
"\DLE\156176"
0
1
(used seed -1356285094314398714)

This test failure is the result of 8-bit truncation of the characters in the input strings, and recent QuickCheck generating unicode strings. The two characters in the string are identical mod 256, but not the same. The ByteString pack makes the string == [16,16] which has the substring 16 in it twice.

I'll shortly add a commit to this PR to resolve this one.

vdukhovni · 2020-05-05T03:01:57Z

The master branch sometimes fails with (not at all surprising :-) infinite loop in the builder tests!
And there's an issue with encodings in an hPutBuilder test (found in the GHC 7.2.2 branch), perhaps the output stream was not in binary mode.
Data.ByteString.Builder:
  toLazyByteStringWith: [OK, passed 100 tests]
  hPutBuilder: [Failed]
*** Failed! Exception: '/tmp/TestBuilder7351: commitBuffer: invalid argument (invalid character)' (after 99 tests and 22 shrinks):
(UnicodeString {getUnicodeString = ""},UnicodeString {getUnicodeString = "\61376"},UnicodeString {getUnicodeString = ""},Recipe Safe 0 0 "" [])
(used seed 7794270791837805650)

That is caused by a file handle I/O roundtrip issue in GHC prior to 7.4 (and so as expected we see failures in 7.2).

Rather than attempt to solve this, my inclination is to drop both 7.2 and 7.0 from the test matrix. The latter does not build at all, because "text" fails to build. As for 7.2, I'm not convinced this is worth trying to fix.

All the other builds (including GHC "head") currently pass CI. So unless someone things that 7.2 is important to fix, this PR fixes all outstanding CI issues.

cartazio · 2020-05-05T23:15:23Z

@RyanGlScott any chance you can help out viktor on how to modernize the Travis ci formula and to what extent we can use newer cabal to work around that build failure on 7.0?

cartazio · 2020-05-05T23:29:44Z

I do recommend trying out the Haskell-ci tool to regenerate a better Travis ci formula.

Also I think a cabal prject file that’s only used for the ci would fix the other historical cruft

vdukhovni · 2020-05-05T23:49:07Z

@RyanGlScott any chance you can help out viktor on how to modernize the Travis ci formula and to what extent we can use newer cabal to work around that build failure on 7.0?

I tried newer cabal (multiple versions) ... no go.

It seems that the real issue is that QuickCheck is too new, and is pulling in text 1.2.4.0 which fails to build with GHC 7.0. We may need an older QuickCheck, though we do have a lower bound of 2.10, I'll try that next, but it may not be old enough...

vdukhovni · 2020-05-05T23:50:56Z

I do recommend trying out the Haskell-ci tool to regenerate a better Travis ci formula.

Also I think a cabal prject file that’s only used for the ci would fix the other historical cruft

I'm sceptical that a stock formula would work here, there are some non-trivial bits in the existing one that the stock formula is unlikely to replicate, but I may take a look at what it generates...

vdukhovni · 2020-05-06T00:05:03Z

QuickCheck 2.10 still pulls in text 1.2.4.0, which still fails to build. I'm strongly inclined to say that CI for ByteString with 7.0 is not worth the effort.

Or at least I don't see why I am the right person to figure this out. This PR is green from 7.4 all the way through 8.10, and only fixes bugs. The master branch also fails with 7.0 and 7.2 and also other versions for which my fixes are needed.

While I'm coming up to speed with GHC/Haskell the language, my proficiency with the rest of the toolchain is still limited. I'd like to see #204 merged, and any work to restore CI for 7.0 and 7.2 (if justified) go into a separate follow-on PR.

cartazio · 2020-05-06T00:14:04Z

https://matrix.hackage.haskell.org/#/package/text
We should figure that out

Huh. Anyways
@hvr @sjakobi @emilypi @chessai @Bodigrim anything missing here ?

Bodigrim · 2020-05-06T00:46:57Z

QuickCheck 2.10 still pulls in text 1.2.4.0, which still fails to build.

Does not seem to be QuickCheck. AFAIU Travis says:

Building text-1.2.4.0...
Preprocessing library text-1.2.4.0...
target `' is not a module name or a source file
Failed to install text-1.2.4.0

ansi-wl-pprint-0.6.9 depends on text-1.2.4.0 which failed to install.
hashable-1.2.5.0 depends on text-1.2.4.0 which failed to install.
nats-1.1.2 depends on text-1.2.4.0 which failed to install.
regex-base-0.94.0.0 depends on text-1.2.4.0 which failed to install.
regex-posix-0.96.0.0 depends on text-1.2.4.0 which failed to install.
semigroups-0.19.1 depends on text-1.2.4.0 which failed to install.
test-framework-0.8.2.0 depends on text-1.2.4.0 which failed to install.
test-framework-hunit-0.3.0.2 depends on text-1.2.4.0 which failed to install.
test-framework-quickcheck2-0.3.0.5 depends on text-1.2.4.0 which failed to install.
unordered-containers-0.2.8.0 depends on text-1.2.4.0 which failed to install.
xml-1.3.14 depends on text-1.2.4.0 which failed to install.

Basically all these packages ask for text < 1.3, so cabal just pulls the latest available text, which is text-1.2.4.0, and fails to build it (with a truly confusing error message! I have no idea what it could mean). As an extreme measure you can probably pin an earlier version of text adding it as a fake dependency of bytestring test suite, guarded by if impl(ghc <=7.2).

Another viable and easier option (which I would personally go for) is to keep building bytestring library on CI, but skip tests for GHC 7.0 and 7.2. Does it look doable?

I mean, it's totally reasonable to deprecate GHC 7.2 in 2020, but in this case one has probably to bump a lower version of base dependency of bytestring.

@vdukhovni I'm sorry for this experience, I very well understand that fixing a critical bug, but waiting for review couple of months and being boggled in CI setup for obsolete GHCs is discouraging. You have all my sympathy.

Bodigrim · 2020-05-06T00:58:42Z

tests/Properties.hs

+    -- coerce to 8-bit alphabet to avoid false-positives
+    = let s8 = C.unpack $ C.pack s
+          _ = l :: Int
+          _ = x :: Int


Just curious, what does this idiom mean? Is it just to fix types of l and x? We can probably throw a type signature for prop_findSubstringsBB for this purpose.

Also, I suggest to keep formating of prop_findSubstringsBB consistent with the next one, prop_findSubstringBB.

Well, I just moved that code without changing it. Your guess about the syntax looks right to me.

Could you do that little cleanup Andrew suggested then I’ll apply this fix to master and then figure out build surgery

Done. I force-pushed an updated and cleaned up set of commits, and perhaps also fixed a less likely, but still possible in principle failure in findSubstring tests that is essentially the same as the one in findSubstrings. When the input is not just 8-bit bytes pack is not injective and there are more matches in the range than in the domain.

vdukhovni · 2020-05-06T01:57:41Z

Another viable and easier option (which I would personally go for) is to keep building bytestring library on CI, but skip tests for GHC 7.0 and 7.2. Does it look doable?

That's the current status of this PR. I've disabled tests for 7.0 and 7.2, and everything else passes. I'm not terribly motivated to fix these, but willing to play along for a bit if it turns out doable...

cartazio · 2020-05-06T14:36:15Z

all understood... maybe the right course of action is i/someone takes your base patch/change set, and then does the CI surgery. given the bandwidth of core folks, dropping from CI needs to be deliberate support work changes, rather than a possible fall through the gaps :)

i can have a go, unless someone else feel they have capacity

vdukhovni · 2020-05-06T16:33:28Z

all understood... maybe the right course of action is i/someone takes your base patch/change set, and then does the CI surgery. given the bandwidth of core folks, dropping from CI needs to be deliberate support work changes, rather than a possible fall through the gaps :)

i can have a go, unless someone else feel they have capacity

Naturally I can't "volunteer" you or anyone else who's not me for the task. Just wanted to mention, if someone is going to look at the build logic, that I haven't figured out how to build bytestring with cabal 3.x without using the legacy v1 commands. Perhaps it also makes sense to figure out what magic it would take to make this package more easily buildable with 3.x.

cartazio · 2020-05-06T17:18:51Z

the answer is gonna be a cabal.project.ci file that we mv cabal.project.ci cabal.project and somethign with that ... we'll look into thi s

vdukhovni · 2020-05-06T17:27:23Z

the answer is gonna be a cabal.project.ci file that we mv cabal.project.ci cabal.project and somethign with that ... we'll look into thi s

Sure, special logic for CI, but even without Travis, I just couldn't get bytestring to build out of the box locally with cabal 3.x without invoking the legacy cabal interface and providing non-default command-line arguments to turn on the "test" flag, ... Just git clone, followed by cabal configure fails with:

'cabal.project.local' file already exists. Now overwriting it.
Resolving dependencies...
cabal: Could not resolve dependencies:
[__0] trying: bench-bytestring-0.1.0.0 (user goal)
[__1] trying: criterion-1.5.6.2 (dependency of bench-bytestring)
[__2] trying: optparse-applicative-0.15.1.0 (dependency of criterion)
[__3] trying: process-1.6.8.0/installed-1.6... (dependency of
optparse-applicative)
[__4] next goal: unix (dependency of process)
[__4] rejecting: unix-2.7.2.2/installed-2.7... (package is broken)
[__4] rejecting: unix-2.7.2.2, unix-2.7.2.1, unix-2.7.2.0, unix-2.7.1.0,
unix-2.7.0.1, unix-2.7.0.0, unix-2.6.0.1, unix-2.6.0.0, unix-2.5.1.1,
unix-2.5.1.0, unix-2.5.0.0, unix-2.4.2.0, unix-2.4.1.0, unix-2.4.0.2,
unix-2.4.0.1, unix-2.4.0.0, unix-2.3.2.0, unix-2.3.1.0, unix-2.3.0.0,
unix-2.2.0.0, unix-2.0 (conflict: process => unix==2.7.2.2/installed-2.7...)
[__4] fail (backjumping, conflict set: process, unix)
Backjump limit reached (currently 4000, change with --max-backjumps or try to
run with --reorder-goals).

vdukhovni · 2020-05-06T18:03:23Z

It looks like one of the issues is blaze-builder 0.3 which wants base < 4.11, but I'm working with GHC 8.8.3. The "blaze-builder" comparison tests look like bitrot, that should/could be removed.

After removing "blaze" benchmarks from BenchAll.hs, and updating the QuickCheck bounds all cabal files to >= 2.10 && <2.15, I got cabal 3.x to configure and build.

Progress, but that's a configuration without tests, I'm in too deep, perhaps someone can suggest how to do that...

sjakobi · 2020-05-06T18:06:42Z

Regarding the benchmarks, #196 contains a solution for the dependency issue there.

vdukhovni · 2020-05-06T18:15:16Z

Regarding the benchmarks, #196 contains a solution for the dependency issue there.

Sure, whether the blaze bits are kept or removed, this too needs be addressed. It seems that too many unaddressed issues are in the backlog, and could all be solved in a CI/toolchain PR that addresses the outstanding cabal issues. But then the PRs should be merged promptly, rather than left to bitrot again.

cartazio · 2020-05-06T19:42:49Z

@sjakobi if I squash and merge this as is, can you prepare a PR ON top that fixes the build and adds back 7 and 7.2?

sjakobi · 2020-05-06T19:50:16Z

@cartazio I'll take a stab at it, but I can't promise how it will work out.

Do you know of any users who want to use the next bytestring release with GHC 7.0 or 7.2?

cartazio · 2020-05-06T19:59:35Z

.travis.yml

@@ -1,8 +1,10 @@
 env:
- - GHCVER=7.0.4 CABALVER=1.16
+ ### GHC 7.0.4 fails to build "text" 1.2.4.0 for QuickCheck
+ # - GHCVER=7.0.4 CABALVER=1.16


can you revert commenting these out so we can see the failures inline?

cartazio · 2020-05-06T19:59:44Z

.travis.yml

+ ### GHC 7.0.4 fails to build "text" 1.2.4.0 for QuickCheck
+ # - GHCVER=7.0.4 CABALVER=1.16
+ ### GHC 7.2.2 fails "hPutBuilder" tests due to encoding roundtrip issues
+ # - GHCVER=7.2.2 CABALVER=1.16


- Add missing other-modules - Update QuickCheck bounds - Use same QuickCheck bounds in tests .cabal file.

hvr · 2020-05-06T20:16:06Z

Do you know of any users who want to use the next bytestring release with GHC 7.0 or 7.2?

I do ;-)

Also dropping "head" which (being 8.7) is no longer worth testing. Noting: - GHC 7.0 fails to build QuickCheck 1.13, because it pulls in "text" 1.2.4.0 which in turn fails to build. - GHC 7.2 fails the "hPutBuilder test", because random Unicode Strings don't roundtrip read/write in GHC 7.2. Fixing this requires limiting the range of test inputs when GHC is too old.

cartazio · 2020-05-06T20:25:35Z

todo: we should add this bug fix to change log

cartazio · 2020-05-06T20:45:29Z

@hvr @sjakobi ok, so the current failures leftover predate this bugfix, are there there any other barriers to merging this in? please tell me so we can progress (and we still need to fix CI of course)

sjakobi · 2020-05-06T20:54:41Z

LGTM, but note that I don't have much maintenance experience with this package.

cartazio · 2020-05-06T20:58:26Z

@hvr ? :)

cartazio · 2020-05-06T20:58:30Z

@hvr ? :)

vdukhovni mentioned this pull request Feb 23, 2020

Combining hPutBuilder + primMapByteStringBounded unsafe with reuse? #203

Closed

sjakobi approved these changes Feb 23, 2020

View reviewed changes

sjakobi mentioned this pull request Feb 23, 2020

Added upper case hex builder support #117

Closed

vdukhovni mentioned this pull request May 5, 2020

Remove findSubstring and findSubstrings #181

Merged

vdukhovni force-pushed the off-by-1-prim-map-bs branch from ae7496b to 3ff987c Compare May 5, 2020 23:04

vdukhovni force-pushed the off-by-1-prim-map-bs branch from 3ff987c to 5b8b9d6 Compare May 5, 2020 23:18

vdukhovni force-pushed the off-by-1-prim-map-bs branch from c6b8b1c to 5b8b9d6 Compare May 6, 2020 00:05

Bodigrim reviewed May 6, 2020

View reviewed changes

cartazio reviewed May 6, 2020

View reviewed changes

hs-viktor added 2 commits May 6, 2020 16:01

Fix findSubstrings CI

98ac311

Cabal file fixes

c885e10

- Add missing other-modules - Update QuickCheck bounds - Use same QuickCheck bounds in tests .cabal file.

vdukhovni force-pushed the off-by-1-prim-map-bs branch from 5b8b9d6 to b723d7b Compare May 6, 2020 20:07

vdukhovni force-pushed the off-by-1-prim-map-bs branch from b723d7b to 6a0235c Compare May 6, 2020 20:24

cartazio merged commit 26ef95d into haskell:master May 6, 2020

This was referenced May 6, 2020

Would be nice to push a 0.10.10.1 release #214

Closed

hPutBuilder test fails with GHC 7.2 #212

Closed

Bodigrim mentioned this pull request Oct 3, 2020

LiquidHaskell integration #288

Open

∞-loop, space==bound in primMapByteStringBounded #204

∞-loop, space==bound in primMapByteStringBounded #204

Conversation

vdukhovni commented Feb 23, 2020 • edited Loading

vdukhovni commented Feb 24, 2020

sjakobi commented Feb 24, 2020

vdukhovni commented Mar 6, 2020

vdukhovni commented May 2, 2020

cartazio commented May 4, 2020

sjakobi commented May 4, 2020

vdukhovni commented May 4, 2020

vdukhovni commented May 5, 2020

vdukhovni commented May 5, 2020

cartazio commented May 5, 2020

cartazio commented May 5, 2020

vdukhovni commented May 5, 2020

vdukhovni commented May 5, 2020

vdukhovni commented May 6, 2020 • edited Loading

cartazio commented May 6, 2020

Bodigrim commented May 6, 2020

Bodigrim May 6, 2020 • edited Loading

Choose a reason for hiding this comment

vdukhovni May 6, 2020

Choose a reason for hiding this comment

cartazio May 6, 2020

Choose a reason for hiding this comment

vdukhovni May 6, 2020 • edited Loading

Choose a reason for hiding this comment

vdukhovni commented May 6, 2020

cartazio commented May 6, 2020

vdukhovni commented May 6, 2020

cartazio commented May 6, 2020

vdukhovni commented May 6, 2020

vdukhovni commented May 6, 2020

sjakobi commented May 6, 2020

vdukhovni commented May 6, 2020

cartazio commented May 6, 2020

sjakobi commented May 6, 2020

cartazio May 6, 2020

Choose a reason for hiding this comment

cartazio May 6, 2020

Choose a reason for hiding this comment

hvr commented May 6, 2020

cartazio commented May 6, 2020

cartazio commented May 6, 2020

sjakobi commented May 6, 2020

cartazio commented May 6, 2020

cartazio commented May 6, 2020

vdukhovni commented Feb 23, 2020 •

edited

Loading

vdukhovni commented May 6, 2020 •

edited

Loading

Bodigrim May 6, 2020 •

edited

Loading

vdukhovni May 6, 2020 •

edited

Loading