Fix JavaScript bounds checking

Fixes #23123 (closed)

This MR fixes JavaScript bounds checking for the -fcheck-prim-bounds flag with byte arrays by:

• checking for the len field that is used to track byte array lengths, rather than the built-in JavaScript length field that is used by regular arrays
• checking for range operations that use a size-zero range (effectively no-ops)

Additionally, the js_broken(23123) ("JS bounds checking broken") for the CheckBoundsOK test introduced in !9629 (closed) is changed to js_broken(21142) ("RuntimeRep requirements for novel Backends"), since the test is still failing on Addr representation.

added 100 commits

• fe03d242...cd00e321 - 99 commits from branch master
• 75ea4767 - JS: fix bounds checking

Compare with previous version

marked this merge request as ready

changed the description

added javascript label

This doesn't look finished because there are 7 failing tests still:

Unexpected failures:
   /builds/ghc/ghc/tmp/ghctest-pg8kapvo/test   spaces/testsuite/tests/codeGen/should_fail/CheckBoundsCompareByteArray2.run       CheckBoundsCompareByteArray2 [bad exit code (0)] (normal)
   /builds/ghc/ghc/tmp/ghctest-pg8kapvo/test   spaces/testsuite/tests/codeGen/should_fail/CheckBoundsCompareByteArray3.run       CheckBoundsCompareByteArray3 [bad exit code (0)] (normal)
   /builds/ghc/ghc/tmp/ghctest-pg8kapvo/test   spaces/testsuite/tests/codeGen/should_fail/CheckBoundsReadInt64Array.run          CheckBoundsReadInt64Array [bad exit code (0)] (normal)
   /builds/ghc/ghc/tmp/ghctest-pg8kapvo/test   spaces/testsuite/tests/codeGen/should_fail/CheckBoundsReadWord64Array.run         CheckBoundsReadWord64Array [bad exit code (0)] (normal)
   /builds/ghc/ghc/tmp/ghctest-pg8kapvo/test   spaces/testsuite/tests/codeGen/should_fail/CheckBoundsReadWord8ArrayAsWord32.run  CheckBoundsReadWord8ArrayAsWord32 [bad exit code (1)] (normal)
   /builds/ghc/ghc/tmp/ghctest-pg8kapvo/test   spaces/testsuite/tests/codeGen/should_fail/CheckOverlapCopyAddrToByteArray.run    CheckOverlapCopyAddrToByteArray [bad exit code (0)] (normal)
   /builds/ghc/ghc/tmp/ghctest-pg8kapvo/test   spaces/testsuite/tests/codeGen/should_fail/CheckOverlapCopyByteArray.run          CheckOverlapCopyByteArray [bad exit code (0)] (normal)

marking as draft.

Can you also add more description to the commit message and make sure the ticket number is also in the commit message?

marked this merge request as draft

requested review from @doyougnu

Could you share the failure log of this test?

If we really want to punt on correctly bounds-checking readAddrArray# and friends, we should either:

• entirely remove the bounds-checking logic for them from GHC.StgToJS.Prim, or
• guard the specific problematic bits of this test with #if !defined(javascript_HOST_ARCH)

I don't agree that guarding is the correct solution here. Splitting the test (it does too much already) would allow better granularity for js_broken and inform us via unexpected passes when the behaviour changes.

Un-resolving.

If you want to re-structure the test, fine. But leaving the entire test marked broken is pretty bad as there is no other test that tries to detect false positives.

In fact, there is still at least one false-positive bug in this patch that the non-Addr# bits of this test would detect if the failure wasn't being hidden.

changed this line in version 10 of the diff

added 1 commit

• db5327f0 - JS: fix bounds checking (Issue 23123)

Compare with previous version

added 13 commits

• db5327f0...3ba77b36 - 12 commits from branch master
• fd18dfce - JS: fix bounds checking (Issue 23123)

Compare with previous version

added 1 commit

• 243bb9f9 - Apply 1 suggestion(s) to 1 file(s)

Compare with previous version

added 1 commit

• 53fbf777 - Revert bounds checking end index for JavaScript IndexByteArrayOp_Word8As*...

Compare with previous version

added 32 commits

• 53fbf777...df1a5811 - 31 commits from branch master
• 01eb5675 - JS: fix bounds checking (Issue 23123)

Compare with previous version

marked this merge request as ready

resolved all threads

requested review from @hsyl20