[stdlib] Enable assertions in unit tests #2718
Conversation
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
|
@JoeLoser I think we can merge this PR before we fix everything to avoid adding new bugs in the meantime. |
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
| @@ -47,7 +47,8 @@ | |||
| # Substitute %mojo for just `mojo` itself | |||
| # since we're not supporting `--sanitize` initially | |||
| # to allow running the tests with LLVM sanitizers. | |||
| config.substitutions.insert(0, ("%mojo", "mojo")) | |||
| config.substitutions.insert(0, ("%mojo", "mojo -D MOJO_ENABLE_ASSERTIONS")) | |||
There was a problem hiding this comment.
Thought I'd like us to go a different direction, if possible. I would like the stdlib to build and all tests pass in both modes (mojo and mojo -D MOJO_ENABLE_ASSERTIONS). I recommend tackling this from the CI matrix level and plumbing the variable (-D MOJO_ENABLE_ASSERTIONS) from the matrix into the lit.cfg.py here which feeds into the tests. This is more generic/composes better with other knobs in the future (think sanitizers and the like). Thoughts?
There was a problem hiding this comment.
I don't mind doing this. I believe we'd need to document the rationale as it's not obvious why we need to run the tests twice. Is there a situation where a test wouldn't fail with assertions enabled, but would fail with assertions disabled?
There was a problem hiding this comment.
That is a valid point regarding running the tests twice and what that does to CI time (although they are run in a parallel matrix and the public Mojo repo is using free CI resources). I'm slightly paranoid about a case where our standard library only works with assertions enabled and not without (since without assertions is the default and common case for ends users/non-contributors). One case is an assert that has side-affects (which is bad practice and you shouldn't do, of course, but I've seen these sort of things way too many times in C++ code).
I could be convinced to only run with assertions, but curious to get others input (CC: @laszlokindrat @rparolin).
There was a problem hiding this comment.
I'm not too worried about CI time as it can be two jobs running at the same time. I'm just wondering if it's worth the complexity.
We can always err on the side of caution and run it twice in the CI. We can make running with assertions the default so that the contributors don't run all the tests twice locally, I don't want it to affect the DX. Let's wait for other inputs.
There was a problem hiding this comment.
+1 to not worrying about CI time (and cost) until it actually becomes a problem. For now I mostly agree with @JoeLoser about being paranoid; way too many things are fragile right now. Once things mature we can decide to do a periodic build with/without asserts instead of on every job if it becomes too expensive (similarly to asan).
There was a problem hiding this comment.
This is on my list to enable internally ASAP and then we can adopt this PR. It's a bit complicated since we use different lit configs etc internally. I'll sort it all out though.
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
|
@JoeLoser before deciding if we want to run all tests twice, maybe we can merge this PR first, as to make sure that no new bugs are being introduced? When we have decided what we want to do with the CI we can make a new pull request. I'll admit I'm a bit scared of new bugs every day this PR is not merged 😅 |
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
I hear you. I'll talk with the team during our team meeting tomorrow and circle back. I agree regarding getting this merged in some shape or form so we can iterate on it. |
|
It seems a bug was introduced in |
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
|
In I'm reading through the description and it seems using poison values is fine (but dangerous) as long as they are overwritten afterwards, which is the case in Is the |
…(Dict.items())` (#40974) [External] [stdlib] Fix UB in `reversed(Dict.values())` and `reversed(Dict.items())` Finally found the culprit in the flakyness that plagued us since a few week in the `test_reversed.mojo`. ### The actual bug: When iterating over a list in reverse order, we should start at `len(my_list) - 1` not at `len(my_list)`. That triggered an out of bounds access and thus was undefined behavior. ### The effect on our CI As you know, we have been seeing flakyness lately. It was documented a number of times and always related to `reverse`: * #2866 (comment) * #2369 ### Why was it passing sometimes? This is because there were `Optional[...]` in the List. Thus if the flag of the `Optional` says that no element is present, it's just skipped (the dict doesn't have an entry at this index). So the list of the Dict would often look like this: `["a", "b", "c", "d"] None` but the last `None` is actually memory that we don't have access to. Sometimes it's then skipped in the iteration making the tests pass. Sometimes it would cause segfaults because the test dict worked with strings. Sometimes we would get `wrong variant type` since we don't know what happens to the memory between None check and access. ### Why wasn't it found earlier? First of all, our Dict implementation is too complexe for what it does and thus is very good at hiding bugs. Well we did have `debug_assert` before getting the element of the `List`, but this `debug_assert` looked like this in the dict iterator: ```mojo @parameter if forward: debug_assert( self.index < self.src[]._reserved, "dict iter bounds" ) else: debug_assert(self.index >= 0, "dict iter bounds") ``` So one bound was checked when reading in one direction and the other bound was checked in the other direction. A better `debug_assert` would have been ```mojo debug_assert(0 <= self.index < self.src[]._reserved, "dict iter bounds") ``` When I worked on my PR #2718 the condition `self.index < self.src[]._reserved` didn't trigger anything since it was in the wrong branch, it was never executed. Also before, `__get_ref` didn't have any bounds checks, even when assertions were enabled. A recent commit 8d0870e adds `unsafe_get()` in List and make `__get_ref` use it. It also adds `debug_assert` to `unsafe_get()`, which means that now `__get_ref` has bounds checks if run with assertions enabled. This allowed me to catch the out of bounds access when updating #2718 making the fail deterministic and debuggable. Since we have this, the `debug_assert` in `dict.mojo` isn't necessary anymore. ### Consequences on ongoing work: * This fix have been also added to #2718 * The PR #2701 that we did with @jayzhan211 was actually correct. It was just using `reverse(Dict.items())` which was buggy at the time. After the fix is merged, we can re-revert this PR. * #2794 is not necessary anymore since the implementation by @jayzhan211 was correct. * The real cause of #2866 was found, the issue has already been closed though. * #2369 can be closed for good. * #2832 can be closed for good. ### Closing thoughts * We really need to run the unit tests with assertions enabled and add assertions whenever necessary * The dict implementation is a bit too complicated. For example, `self._reserved` is the length of the internal list. There is no need to store the length of the list twice. Let's drop this variable and use `len(self._entries)` instead. I guess this is a relic of the time when `List` wasn't completely flushed out. If had done so, it would have been ovious that we can't do `my_list.__get_ref(len(my_list))` * Iterating manually over a list like this is bug-prone. The implementation we have especially is, since ```mojo @parameter if forward: self.index += 1 else: self.index -= 1 ``` is done twice in the code, it should only be done once. While there is no bug, code duplication and complexity hides bugs. * We should iterate over the list with a list iterator, not with a custom-made iterator. This will remove a lot of code in the `dict.mojo`. Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com> Closes #2896 MODULAR_ORIG_COMMIT_REV_ID: b65009dc51f1e3027f91b5b61a5b7003cb022b87
|
!sync |
modularbot
added
the
imported-internally
There was a problem hiding this comment.
I just landed this internally. When reworking the commit message for some other internal changes I had to make to make this atomically land, I messed up something needed for Copybara, so the normal automation message didn't post here. This will be available in the next nightly. Thanks for pushing on this, @gabrieldemarmiesse! 🎉
JoeLoser
added
merged-internally
|
Noice! This will definitly allow us to be more confident in our future PRs :) |
Normally I'd think that bit shifting negative values should be UB. I'd vote for removing the |
[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
|
Landed in b0c8971! Thank you for your contribution 🎉 |
This is a PR to keep track of what is left to do. Do not review. I will make one PR per bug. When all bugs are fixed, only the diff about the config change will remain here. And then we can merge this PR.
Related to #2687
I excluded all test files which cannot currently run with assertions enabled.