-
Notifications
You must be signed in to change notification settings - Fork 768
[SYCL] Add STL wrapper for crtdbg.h
and define _CrtDbgReport
#18213
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
Conversation
sycl/include/sycl/stl_wrappers/array
Outdated
|
||
// Include real STL <array> header - the next one from the include search | ||
// directories. | ||
// Define variable templated _CrtDbgReport with MSVC to avoid the following |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, that does not seem like a reliable solution. We still don't have control over the real <array>
content and tomorrow it may start using some new error reporting mechanism which won't be covered by this.
I think that the proper way of not hitting issues like this from uncontrolled STL implementations is stop using them in our device headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to provide as much support for using STL in user's code as possible. Using it ourselves and making fixes like that is the best way to ensure the smoothest user experience possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to provide as much support for using STL in user's code as possible.
Do we document any guarantees anywhere? I'm not aware of any extensions and the core SYCL spec explicitly says that everything that is not documented is not guaranteed to be supported.
What is our plan for cases when MSVC headers are updated in an incompatible way in between our releases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/supported/C-CXX-StandardLibrary.rst and your argument applies to them as well. MSVC can change their implementation of any of the "documented/supported" APIs at any point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/supported/C-CXX-StandardLibrary.rst and your argument applies to them as well. MSVC can change their implementation of any of the "documented/supported" APIs at any point.
That is true, but the document explicitly only document a concrete subset of functions and 95% of them are implemented in the library. We do take a similar risk, but it is lower. And we have officially taken it by documenting the support.
Here we are somewhat in the gray area where we don't document the support for std::array
, but still try to use it and jump through the hoops to make it work, but what's the point if we don't document it to be supported?
I think that we should either document it to justify those tricks, or just make our implementation more robust by explicitly saying that we won't support that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "gray area" is a feature and not a bug :) As for documenting, I'd prefer to do it once we are more confident that it actually works. So maybe implement this, use std::array
internally, and if we don't have any bug reports for the next release, then document it as supported (i.e. treat the current state as "experimental").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for documenting, I'd prefer to do it once we are more confident that it actually works.
And how do we ensure that? std::array
has 20 different methods, do we use them all in our implementation to ensure that they work?
I still find our approach weird. If we want to support std::array
, let's write dedicated tests for it and fix any bugs, but I'm not sure if it worth continue to use it in our own implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to provide as much support for using STL in user's code as possible. Using it ourselves and making fixes like that is the best way to ensure the smoothest user experience possible.
FYI: #18252
std::array
and define _CrtDbgReport
crtdbg.h
and define _CrtDbgReport
Signed-off-by: Agarwal, Udit <[email protected]>
@@ -2,15 +2,12 @@ | |||
// the constructor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@uditagarwal97, I think the comment requires updates too.
// Test to isolate sycl::vec bug due to use of std::array in
// the constructor.
Wrapping around because want to define
_CrtDbgReport
beforecrtdbg.h
header is included. Defining variable templated_CrtDbgReport
overrides the use of variable argument_CrtDbgReport
function, that declared incrtdbg.h
.With variable argument
_CrtDbgReport
,llvm-spirv
throws the following error:Fixes CMPLRLLVM-66787