FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fixes #2568. MSVC optimizations causing segmentation faults in optimized builds. by sg-james · Pull Request #2628 · xtensor-stack/xtensor · GitHub

  1. xtensor-stack
  2. xtensor

Repository navigation

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

Merged
tdegeus merged 4 commits into
xtensor-stack:masterfrom
sg-james:2568-msvc-optimizer-bug-workaround
Jan 9, 2023
ConversationCommits (4)ChecksFiles changed
Merged

fixes #2568. MSVC optimizations causing segmentation faults in optimized builds.#2628
tdegeus merged 4 commits into
xtensor-stack:masterfrom
sg-james:2568-msvc-optimizer-bug-workaround

Conversation

sg-james commented Jan 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 commented Jan 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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 force-pushed the 2568-msvc-optimizer-bug-workaround branch from 780a8ed to 6f46297 Compare January 6, 2023 17:25

sg-james commented Jan 6, 2023

Copy link
Copy Markdown
Contributor Author

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

tdegeus commented Jan 6, 2023

Copy link
Copy Markdown
Member

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 commented Jan 6, 2023

Copy link
Copy Markdown
Contributor Author

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

fixes xtensor-stack#2568. Fixes MSVC optimizations causing segmentati…
7e9029c 
…on faults in optimized (/O1 /O2 builds.

- this should fix 0-sized xarrays, xtensors (xt::xtensor<size_t, 0>), xt::argmax()
Add test case for argmax which results in xtensor<size_t, 0>
24bf6ee
sg-james force-pushed the 2568-msvc-optimizer-bug-workaround branch from 6f46297 to 24bf6ee Compare January 6, 2023 19:08

SylvainCorlay commented Jan 6, 2023

Copy link
Copy Markdown
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 commented Jan 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 force-pushed the 2568-msvc-optimizer-bug-workaround branch from 26523e5 to 14a54c5 Compare January 6, 2023 23:09
tdegeus reviewed Jan 7, 2023
View reviewed changes
Comment thread docs/source/compilers.rst Outdated

tdegeus commented Jan 7, 2023

Copy link
Copy Markdown
Member

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

tdegeus mentioned this pull request Jan 7, 2023
Closed
Jobsecond referenced this pull request in Jobsecond/audio-slicer-cpp Jan 7, 2023
find argmin using C++ STL methods
661174d
tdegeus merged commit 0ba5963 into xtensor-stack:master Jan 9, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tdegeus tdegeus left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Segmentation fault 0-size xtensor on Windows argmax crashes when compiled using Visual Studio compiler with O1/O2 optimizations xsort tests failing

3 participants

Back | FazBrowse Home | New Git URL