[BUG] Enable assertions when testing the stdlib in the CI #2687

gabrieldemarmiesse · 2024-05-16T19:26:22Z

Bug description

Currently, there are out-of-bounds access bugs in the stdlib's tests. We don't know yet if the bugs are coming from the stdlib or their tests but it's not really acceptable, especially since we have a mechanisme in the Mojo language to detect them (MOJO_ENABLE_ASSERTIONS=1)

We should:

Fix all bugs where we get something out of bounds.
Enable in the CI the assertions to ensure that we don't re-introduce new bugs.

See #2677 (comment) for the background about this issue.

Steps to reproduce

Run the unit tests with MOJO_ENABLE_ASSERTIONS=1

System information

CI

The text was updated successfully, but these errors were encountered:

… (#40083) [External] [stdlib] Fix incorrect number of elements in `StaticTuple` We could technically have a check at compile-time but I'm not sure it's worth the effort as `StaticTuple` is deprecated in favor of `InlineArray`. Related to #2687 Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com> Closes #2689 MODULAR_ORIG_COMMIT_REV_ID: b965330e55c49074b61d9603414c033580a9e096

[External] [stdlib] Fix wrong slice check in span Fix part of #2687 both the start and the end of span can be `len(original_string)`, that's not an issue. In the case where the start is equal to the lenght of th string, it just gives an empty span. This is similar to the slicing `my_string[:0]` which also gives an empty span. Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com> Closes #2726 MODULAR_ORIG_COMMIT_REV_ID: 1e08d3d29352ae43b7e833d36f5a3acfa7ebe765

gabrieldemarmiesse · 2024-05-18T22:43:49Z

@JoeLoser I found all of them and made PRs for all fixables bugs. The final big bad boss is a segfault that happens when compiling test_string.mojo with assertion enabled (the compiling step has a segfault, not the runtime step). What's even funnier is that if you try it enough times, it finally compiles XD Also the output of the build log is different every time, making this bug very very weird.

I even got sometimes (I was spamming the command to build the test) this output:

[31693:31693:20240518,223347.693229:ERROR file_io_posix.cc:144] open /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq: No such file or directory (2)
[31693:31693:20240518,223347.693288:ERROR file_io_posix.cc:144] open /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq: No such file or directory (2)
Please submit a bug report to https://github.com/modularml/mojo/issues and include the crash backtrace along with all the relevant source codes.
Stack dump:
0.      Program arguments: mojo build -D MOJO_ENABLE_ASSERTIONS trying_stuff2.mojo
1.      Crash resolving decl body at loc("/projects/open_source/mojo/trying_stuff2.mojo":162:4)
    >> fn test_add() raises:
          ^.................
    >>     var s1 = String("123")
    >>     var s2 = String("abc")
    >>     var s3 = s1 + s2
    >>     assert_equal("123abc", s3)
2.      Crash parsing statement at loc("/projects/open_source/mojo/trying_stuff2.mojo":168:5)
    >>     var s4 = String("x")
           ^...................<
 #0 0x000055c3d79367b8 (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x12937b8)
 #1 0x000055c3d79345de (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x12915de)
 #2 0x000055c3d7936e4d (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x1293e4d)
 #3 0x00007fd780cf3520 (/usr/lib/x86_64-linux-gnu/libc.so.6+0x42520)
 #4 0x000055c3d78ff976 (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x125c976)
 #5 0x000055c3d78fa515 (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x1257515)
 #6 0x000055c3d97179d3 (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x30749d3)
 #7 0x000055c3d9717979 (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x3074979)
 #8 0x000055c3d7d63f39 (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x16c0f39)
 #9 0x000055c3d7d42e2e (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x169fe2e)
#10 0x000055c3d7cd0ca7 (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x162dca7)
#11 0x000055c3d7cd3dd9 (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x1630dd9)
#12 0x000055c3d7cd55dd (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x16325dd)
#13 0x000055c3d7d255b2 (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x16825b2)
#14 0x000055c3d7d17529 (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x1674529)
#15 0x000055c3d7d5def1 (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x16baef1)
#16 0x000055c3d7d52976 (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x16af976)
#17 0x000055c3d7d52676 (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x16af676)
#18 0x000055c3d7cebd58 (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x1648d58)
#19 0x000055c3d7cff3b2 (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x165c3b2)
#20 0x000055c3d7cffac1 (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x165cac1)
#21 0x000055c3d7d093c9 (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x16663c9)
#22 0x000055c3d7d0981f (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x166681f)
#23 0x000055c3d787267c (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x11cf67c)
#24 0x000055c3d7873eb4 (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x11d0eb4)
#25 0x000055c3d786fecb (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x11ccecb)
#26 0x000055c3d786e924 (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x11cb924)
#27 0x00007fd780cdad90 (/usr/lib/x86_64-linux-gnu/libc.so.6+0x29d90)
#28 0x00007fd780cdae40 __libc_start_main (/usr/lib/x86_64-linux-gnu/libc.so.6+0x29e40)
#29 0x000055c3d786e2ae (/root/.modular/pkg/packages.modular.com_nightly_mojo/bin/mojo+0x11cb2ae)
mojo crashed!
Please file a bug report.
/projects/open_source/mojo/trying_stuff2.mojo:1:1: error: leading indentation uses inconsistent whitespace (tabs and spaces) than previous line
# ===----------------------------------------------------------------------=== #
^
/projects/open_source/mojo/trying_stuff2.mojo:1:1: error: statement has excess indentation
# ===----------------------------------------------------------------------=== #
^
/projects/open_source/mojo/trying_stuff2.mojo:1:1: note: indentation should match previous statement
# ===----------------------------------------------------------------------=== #
^
/projects/open_source/mojo/trying_stuff2.mojo:1:1: error: unexpected character
# ===----------------------------------------------------------------------=== #
^
/projects/open_source/mojo/trying_stuff2.mojo:1:1: error: use of unknown declaration 'P'
# ===----------------------------------------------------------------------=== #
^
mojo: error: failed to parse the provided Mojo source module

I'll try to open an issue with a minimal reproducible example later on so that we can actually track this build segfault.

… (#40083) [External] [stdlib] Fix incorrect number of elements in `StaticTuple` We could technically have a check at compile-time but I'm not sure it's worth the effort as `StaticTuple` is deprecated in favor of `InlineArray`. Related to modularml#2687 Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com> Closes modularml#2689 MODULAR_ORIG_COMMIT_REV_ID: b965330e55c49074b61d9603414c033580a9e096

[External] [stdlib] Fix wrong slice check in span Fix part of modularml#2687 both the start and the end of span can be `len(original_string)`, that's not an issue. In the case where the start is equal to the lenght of th string, it just gives an empty span. This is similar to the slicing `my_string[:0]` which also gives an empty span. Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com> Closes modularml#2726 MODULAR_ORIG_COMMIT_REV_ID: 1e08d3d29352ae43b7e833d36f5a3acfa7ebe765

gabrieldemarmiesse · 2024-05-19T13:10:59Z

The blocker segfault is documented here: #2751

[External] [stdlib] Fix out of bounds access in `List.index()` Related to #2687 There were multiple bugs related to clipping there. Long story short, the behavior of `list.index()` in python is this one: given a `start` and `end`, python will look for the element in `my_list[start:end]` and report the result, (`start` is added to the result to give the index with respect to the original list). You can take a look at the description of the `index` method here: https://docs.python.org/3/tutorial/datastructures.html#more-on-lists Since there is slicing semantics applied to `start` and `end`, we should do multiple things: 1) default to the start and the end of the list 2) normalize negative values by doing `+ len(my_list)` 3) clip both start and end between `0` and `len(my_list)` The last step wasn't done correctly. Especially for the `stop` argument where the clipping was applied only when negative values were found (this caused the out of bounds bug in the tests). Effectively ```mojo return end if end > 0 else min(end + size, size) ``` is equivalent to ```mojo return end if end > 0 else end + size ``` since the min is applied only when `end <= 0`. So `end + size <= size` This test can cause some flakyness in our CI: `test_list_a.index(10, start=5, stop=50)` for a list of size 6. The stop was positive, so it was never clipped, thus too many values (out of bounds) were tried, causing some to match, sometimes. Another bug was that because of this condition, `end = 0` means "check until the end of the list" while in python, with the slicing semantics, `end = 0` means "do nothing" (empty slice). ### TL;DR Multiple clipping bugs. Slicing semantics applied to `start` and `stop` now like in Python. No more flakyness in our CI. No more out of bounds access. Corresponding tests added. Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com> Closes #2745 MODULAR_ORIG_COMMIT_REV_ID: 7bad2830dc41d96ae383fc4a8eac9ca3a69581de

…(#40303) [External] [stdlib] Fix wrong debug_assert condition in `InlineList` If the InlineList is of capacity N, then it's possible to initialize it with N values. It's the same as a regular List. Related to #2687 Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com> Closes #2744 MODULAR_ORIG_COMMIT_REV_ID: 8f5093eb1eb79591e9636f9a7a969037bab6dcd1

[External] [stdlib] Fix invalid code point check Related to #2687 See https://stackoverflow.com/questions/27415935/does-unicode-have-a-defined-maximum-number-of-code-points for the correct range. You can also use Python's `chr` function to make sure the range is correct Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com> Closes #2747 MODULAR_ORIG_COMMIT_REV_ID: e93765fdad9876ba4b7838194182bf72c5fb1c71

…D (#40375) [External] [stdlib] Fix incorrect number of variadic arguments in SIMD Fix part of #2687 by changing various SIMD tests that were neither passing `1` or `N` values in the construction of a `SIMD` type (i.e. ones that were passing `k` values where `1 < k < N`). Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com> Closes #2720 MODULAR_ORIG_COMMIT_REV_ID: 20baf2934b7d768523a412755e32ffbfb109c0fd

…str__` method (#40421) [External] [stdlib] Fix: Add null terminator to the buffer inside `__str__` method String really has a hard time working without this null terminator and I feel this PR is a step towards fixing #2687 without requirering controversial changes like the ones in #2691. Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com> Closes #2787 MODULAR_ORIG_COMMIT_REV_ID: 95f438ebbb6e076fe6ffb79debf7cd4fd70248de

ematejska · 2024-05-24T16:41:11Z

@JoeLoser Is the blocker still a problem or is there a work around?

gabrieldemarmiesse · 2024-05-24T16:53:01Z

Yes the blocker is still a problem. In my PR #2718 I enable assertions in all unit tests except test_string.mojo because of this blocker. So even after the PR is merged, we still won't have assertions when testing strings unless the blocking issue is solved.

… (#40083) [External] [stdlib] Fix incorrect number of elements in `StaticTuple` We could technically have a check at compile-time but I'm not sure it's worth the effort as `StaticTuple` is deprecated in favor of `InlineArray`. Related to modularml#2687 Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com> Closes modularml#2689 MODULAR_ORIG_COMMIT_REV_ID: b965330e55c49074b61d9603414c033580a9e096

[External] [stdlib] Fix wrong slice check in span Fix part of modularml#2687 both the start and the end of span can be `len(original_string)`, that's not an issue. In the case where the start is equal to the lenght of th string, it just gives an empty span. This is similar to the slicing `my_string[:0]` which also gives an empty span. Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com> Closes modularml#2726 MODULAR_ORIG_COMMIT_REV_ID: 1e08d3d29352ae43b7e833d36f5a3acfa7ebe765

[External] [stdlib] Fix out of bounds access in `List.index()` Related to modularml#2687 There were multiple bugs related to clipping there. Long story short, the behavior of `list.index()` in python is this one: given a `start` and `end`, python will look for the element in `my_list[start:end]` and report the result, (`start` is added to the result to give the index with respect to the original list). You can take a look at the description of the `index` method here: https://docs.python.org/3/tutorial/datastructures.html#more-on-lists Since there is slicing semantics applied to `start` and `end`, we should do multiple things: 1) default to the start and the end of the list 2) normalize negative values by doing `+ len(my_list)` 3) clip both start and end between `0` and `len(my_list)` The last step wasn't done correctly. Especially for the `stop` argument where the clipping was applied only when negative values were found (this caused the out of bounds bug in the tests). Effectively ```mojo return end if end > 0 else min(end + size, size) ``` is equivalent to ```mojo return end if end > 0 else end + size ``` since the min is applied only when `end <= 0`. So `end + size <= size` This test can cause some flakyness in our CI: `test_list_a.index(10, start=5, stop=50)` for a list of size 6. The stop was positive, so it was never clipped, thus too many values (out of bounds) were tried, causing some to match, sometimes. Another bug was that because of this condition, `end = 0` means "check until the end of the list" while in python, with the slicing semantics, `end = 0` means "do nothing" (empty slice). ### TL;DR Multiple clipping bugs. Slicing semantics applied to `start` and `stop` now like in Python. No more flakyness in our CI. No more out of bounds access. Corresponding tests added. Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com> Closes modularml#2745 MODULAR_ORIG_COMMIT_REV_ID: 7bad2830dc41d96ae383fc4a8eac9ca3a69581de

…(#40303) [External] [stdlib] Fix wrong debug_assert condition in `InlineList` If the InlineList is of capacity N, then it's possible to initialize it with N values. It's the same as a regular List. Related to modularml#2687 Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com> Closes modularml#2744 MODULAR_ORIG_COMMIT_REV_ID: 8f5093eb1eb79591e9636f9a7a969037bab6dcd1

[External] [stdlib] Fix invalid code point check Related to modularml#2687 See https://stackoverflow.com/questions/27415935/does-unicode-have-a-defined-maximum-number-of-code-points for the correct range. You can also use Python's `chr` function to make sure the range is correct Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com> Closes modularml#2747 MODULAR_ORIG_COMMIT_REV_ID: e93765fdad9876ba4b7838194182bf72c5fb1c71

…D (#40375) [External] [stdlib] Fix incorrect number of variadic arguments in SIMD Fix part of modularml#2687 by changing various SIMD tests that were neither passing `1` or `N` values in the construction of a `SIMD` type (i.e. ones that were passing `k` values where `1 < k < N`). Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com> Closes modularml#2720 MODULAR_ORIG_COMMIT_REV_ID: 20baf2934b7d768523a412755e32ffbfb109c0fd

…str__` method (#40421) [External] [stdlib] Fix: Add null terminator to the buffer inside `__str__` method String really has a hard time working without this null terminator and I feel this PR is a step towards fixing modularml#2687 without requirering controversial changes like the ones in modularml#2691. Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com> Closes modularml#2787 MODULAR_ORIG_COMMIT_REV_ID: 95f438ebbb6e076fe6ffb79debf7cd4fd70248de

[External] [stdlib] Enable assertions in unit tests Change the default mode of running the standard library unit tests to run with `-D MOJO_ENABLE_ASSERTIONS`, i.e. assertions enabled. Not all of the tests work with assertions, so appropriate issues are filed to track that work along with a new substitution, `%bare-mojo` for running without assertions enabled. Related to #2687 Closes #2718 MODULAR_ORIG_COMMIT_REV_ID: ddc68f1fd7ae786abe135f254264157d264caa93

gabrieldemarmiesse added bug Something isn't working mojo-repo Tag all issues with this label labels May 16, 2024

This was referenced May 16, 2024

[stdlib] Add the normalize_index function #2677

Closed

[stdlib] Fix incorrect number of elements in StaticTuple #2689

Closed

JoeLoser added help wanted Extra attention is needed mojo-stdlib Tag for issues related to standard library labels May 16, 2024

gabrieldemarmiesse mentioned this issue May 16, 2024

[stdlib] Add more safeguards in the String constructors #2691

Closed

This was referenced May 17, 2024

[stdlib] Enable assertions in unit tests #2718

Closed

[stdlib] Fix incorrect number of variadic arguments in SIMD #2720

Closed

[stdlib] Fix wrong slice check in span #2726

Closed

This was referenced May 18, 2024

[stdlib] Fix wrong debug_assert condition in InlineList #2744

Closed

[stdlib] Fix out of bounds access in List.index() #2745

Closed

[stdlib] Fix invalid code point check #2747

Closed

gabrieldemarmiesse mentioned this issue May 19, 2024

[BUG] Flaky segfault during mojo build with -D MOJO_ENABLE_ASSERTIONS #2751

Open

ematejska removed mojo-stdlib Tag for issues related to standard library help wanted Extra attention is needed labels May 20, 2024

gabrieldemarmiesse mentioned this issue May 22, 2024

[stdlib] Fix: Add null terminator to the buffer inside __str__ method #2787

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUG] Enable assertions when testing the stdlib in the CI #2687

[BUG] Enable assertions when testing the stdlib in the CI #2687

gabrieldemarmiesse commented May 16, 2024 •

edited by ematejska

gabrieldemarmiesse commented May 18, 2024 •

edited

gabrieldemarmiesse commented May 19, 2024

ematejska commented May 24, 2024 •

edited

gabrieldemarmiesse commented May 24, 2024

[BUG] Enable assertions when testing the stdlib in the CI #2687

[BUG] Enable assertions when testing the stdlib in the CI #2687

Comments

gabrieldemarmiesse commented May 16, 2024 • edited by ematejska

Bug description

Steps to reproduce

System information

gabrieldemarmiesse commented May 18, 2024 • edited

gabrieldemarmiesse commented May 19, 2024

ematejska commented May 24, 2024 • edited

gabrieldemarmiesse commented May 24, 2024

gabrieldemarmiesse commented May 16, 2024 •

edited by ematejska

gabrieldemarmiesse commented May 18, 2024 •

edited

ematejska commented May 24, 2024 •

edited