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

fixes #2568. MSVC optimizations causing segmentation faults in optimized builds. #2628

Merged

Conversation

sg-james
Copy link
Contributor

@sg-james sg-james commented Jan 6, 2023

Works around an msvc optimizer bug which occurs when shape.size() == 0.

Surrounding the problematic region to prevent illegal access that occurs on windows MSVC optimized builds (/O1, /O2).

  • allows construction of 0-sized xarray and xtensors (e.g. xt::xtensor<size_t, 0>)
  • also fixes xt::argmax() crashing

Checklist

  • The title and commit message(s) are descriptive.
  • Small commits made to fix your PR have been squashed to avoid history pollution.
  • Tests have been added for new features or bug fixes.
  • API of new functions and classes are documented.

Description

Fixes #2551
Fixes #2568
Fixes #2596

@tdegeus
Copy link
Member

tdegeus commented Jan 6, 2023

Thanks a lot!

It could be nice to do the workaround only on MSVC (on potentially only on the affected version) using some pre-processor statement?

@sg-james sg-james force-pushed the 2568-msvc-optimizer-bug-workaround branch from 780a8ed to 6f46297 Compare January 6, 2023 17:25
@sg-james
Copy link
Contributor Author

sg-james commented Jan 6, 2023

pushed with msvc guards. I also added a test which I think covers all 3 issues.

@tdegeus
Copy link
Member

tdegeus commented Jan 6, 2023

I would have probably only added

        #if defined(_MSC_VER) && (1931 <= _MSC_VER)
             // Workaround MSVC compiler optimization bug, xtensor#2568
             if (shape.size() == 0) {
                 return static_cast<std::size_t>(data_size);
             }
        #endif 

to keep the implementation cleaner. That should work too no?

@sg-james
Copy link
Contributor Author

sg-james commented Jan 6, 2023

oh. yes, that's much better. I'll fix that up

…on faults in optimized (/O1 /O2 builds.

- this should fix 0-sized xarrays, xtensors (xt::xtensor<size_t, 0>), xt::argmax()
@sg-james sg-james force-pushed the 2568-msvc-optimizer-bug-workaround branch from 6f46297 to 24bf6ee Compare January 6, 2023 19:08
@SylvainCorlay
Copy link
Member

This is amazing!

It may be worth adding a comment and a reference to this compiler issue in https://xtensor.readthedocs.io/en/latest/compilers.html.

@sg-james
Copy link
Contributor Author

sg-james commented Jan 6, 2023

Would something like the following do? (not so familiar with .rst)

Visual Studio 2022 (19.31+) workaround inline compiler optimization bug
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

In `xstrides.hpp`, added an early return inside `compute_strides` when ``shape.size() == 0`` to 
prevent a run time crash from occuring. Without this guard statement, instructions from inside the 
for loop were somehow being reached, despite being logically unreachable. 
Original issue  `here. <https://github.com/xtensor-stack/xtensor/issues/2568>`_ 
Upstream issue `here. <https://developercommunity.visualstudio.com/t/Runtime-Crash-msvc--1931-with-optimiz/10134617>`_

@sg-james sg-james force-pushed the 2568-msvc-optimizer-bug-workaround branch from 26523e5 to 14a54c5 Compare January 6, 2023 23:09
docs/source/compilers.rst Outdated Show resolved Hide resolved
@tdegeus
Copy link
Member

tdegeus commented Jan 7, 2023

Great! Good to go. Thanks a lot for getting to the bottom of this

Jobsecond referenced this pull request in Jobsecond/audio-slicer-cpp Jan 7, 2023
@tdegeus tdegeus merged commit 0ba5963 into xtensor-stack:master Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants