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

Weird _invalid_parameter failures in _STL_ASSERT #2335

Closed
libbooze opened this issue Nov 12, 2021 · 25 comments
Closed

Weird _invalid_parameter failures in _STL_ASSERT #2335

libbooze opened this issue Nov 12, 2021 · 25 comments
Labels
invalid This issue is incorrect or by design

Comments

@libbooze
Copy link

libbooze commented Nov 12, 2021

This is very confusing, I did not mess with installation, looks like just internal header inclusion is missing, but then it makes no sense that everything is not broken.
image

Also it seems this is broken for somebody else too:
microsoft/onnxruntime#9735

For helping people search: error is this:

'_invalid_parameter': is not a member of '`global namespace'

@libbooze
Copy link
Author

update: Intellisense was just slow, it works now, and properly shows the problematic expand, so I guess problem is not the macro, but the expansion itself.
image

@libbooze libbooze changed the title Weird undefined _STL_ASSERT failures Weird _invalid_parameter failures in _STL_ASSERT Nov 12, 2021
@fsb4000
Copy link
Contributor

fsb4000 commented Nov 12, 2021

Could you give a some example which gives the compilation error?
I'll try on my machine.

@libbooze
Copy link
Author

libbooze commented Nov 12, 2021

Could you give a some example which gives the compilation error? I'll try on my machine.

Unfortunately it is a closed source large project that I can not share. Linked gitlab issue seems to be reproducible since it is in some open source msft github project.

Author there explained his steps:
microsoft/onnxruntime#9735

note: that issue is on W11, I am on W10, so I guess that is to show OS version is not the issue(probably)

@libbooze
Copy link
Author

Also it is weird that it seems like _invalid_parameter is actually missing from sources... I can not find it except when used in macro definition

image

maybe doing

git log -S _invalid_parameter

on this repo might show some recent removals/moves...

@fsb4000
Copy link
Contributor

fsb4000 commented Nov 12, 2021

I modified _STL_ASSERT and _invalid_parameter recently: #2054 ...

About _invalid_parameter.
It's UCRT function.

Install Windows SDK and look into the file: C:\Program Files (x86)\Windows Kits\10\Source\10.0.22000.0\ucrt\misc\invalid_parameter.cpp

@fsb4000
Copy link
Contributor

fsb4000 commented Nov 12, 2021

Did you make some modifications with _DEBUG macro?
If I

  1. undefine _DEBUG
  2. include some C header
  3. define _DEBUG
  4. include some C++ header
    I got the error.

C:\Dev\STL\playground>cl /EHsc test.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.31.30818 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

test.cpp
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.31.30818\include\xmemory(154): error C2039: '_invalid_parameter': is not a member of '`global namespace''
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.31.30818\include\xmemory(154): error C3861: '_invalid_parameter': identifier not found
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.31.30818\include\xmemory(164): error C2039: '_invalid_parameter': is not a member of '`global namespace''
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.31.30818\include\xmemory(164): error C3861: '_invalid_parameter': identifier not found
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.31.30818\include\xmemory(1199): error C2039: '_invalid_parameter': is not a member of '`global namespace''
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.31.30818\include\xmemory(1199): error C3861: '_invalid_parameter': identifier not found
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.31.30818\include\random(1927): error C2039: '_invalid_parameter': is not a member of '`global namespace''
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.31.30818\include\random(1927): error C3861: '_invalid_parameter': identifier not found
test.cpp(7): error C2039: '_invalid_parameter': is not a member of '`global namespace''
test.cpp(7): error C3861: '_invalid_parameter': identifier not found

C:\Dev\STL\playground>type test.cpp
#undef _DEBUG
#include <corecrt.h>
#define _DEBUG

#include <random>
void test() {
  _STL_ASSERT(false, "hi");
}

int main() {
  test();
}

I can't reproduce the bug without witchcraft with the _DEBUG macro...

@libbooze
Copy link
Author

I modified _STL_ASSERT and _invalid_parameter recently: #2054 ...

About _invalid_parameter. It's UCRT function.

Install Windows SDK and look into the file: C:\Program Files (x86)\Windows Kits\10\Source\10.0.22000.0\ucrt\misc\invalid_parameter.cpp

I am a bit confused(sorry not a professional level lib developer). Shouldn't STL be self-contained, so should I not be able to at least see the forward declaration of _invalid_parameter in my VS install folder?

Or STL depends on the Windows SDK being installed also?

As for _DEBUG macro(your next comment) I will try to investigate, but my projects are not trivial, so I cant promise results.

@fsb4000
Copy link
Contributor

fsb4000 commented Nov 12, 2021

STL depends on UCRT and some other Windows libs.

You can try forward declare the function, maybe it will work as workaround:

#ifdef _DEBUG
    _ACRTIMP void __cdecl _invalid_parameter(
        _In_opt_z_ wchar_t const*,
        _In_opt_z_ wchar_t const*,
        _In_opt_z_ wchar_t const*,
        _In_       unsigned int,
        _In_       uintptr_t
        );
#endif

@libbooze
Copy link
Author

libbooze commented Nov 12, 2021

STL depends on UCRT and some other Windows libs.

Sorry, my mistake did not know about this, but on the good news side:
I noticed that one of problematic builds uses pybind and I noticed pybind 2.6 undefs _DEBUG, would that cause this problems maybe?

image

@fsb4000
Copy link
Contributor

fsb4000 commented Nov 12, 2021

Yes, it will.
As I said, you can try to forward declare the function
after
#define _DEBUG

and before
#undef PYBIND11_DEBUG_MARKER

I hope it will fix the issue...

@libbooze
Copy link
Author

libbooze commented Nov 12, 2021

Yes, it will.
As I said, you can try to forward declare the function before that. I hope it will fix the issue...

OK, also just confirmed that your fix probably works(I tested only 1 project).

I did not know where to get _ACRTIMP from so I just did dllimport(I guess that is fine for users of library) and changed the uintptr_t to uint64 since I also did not know where is that defined and I only use 64b builds....

#include <sal.h>

#ifdef _DEBUG

__declspec(dllimport) void __cdecl _invalid_parameter(

    _In_opt_z_ wchar_t const*,

    _In_opt_z_ wchar_t const*,

    _In_opt_z_ wchar_t const*,

    _In_ unsigned int,

    _In_ unsigned __int64);

#endif

Anyways I am not STL expert STL so I have no idea if what pybind does is legal, that is for you and the team to discuss, I am happy we narrowed it down, so you can close this bug as a not caused by STL or whatever you decide...

Thanks for the help.

@fsb4000
Copy link
Contributor

fsb4000 commented Nov 12, 2021

that is for you and the team to discuss

Yes, you are correct.

Thanks for the help.

You're welcome.

@libbooze
Copy link
Author

For future visitors: forward declaration fixes the issue of compile, but I still get linker issues. It is possible I got the wrong signature, but even when I tried to match the signature in windows headers I still got link failures.

@fsb4000
Copy link
Contributor

fsb4000 commented Nov 12, 2021

#ifndef _ACRTIMP
    #if defined _CRTIMP && !defined _VCRT_DEFINED_CRTIMP
        #define _ACRTIMP _CRTIMP
    #elif !defined _CORECRT_BUILD && defined _DLL
        #define _ACRTIMP __declspec(dllimport)
    #else
        #define _ACRTIMP
    #endif
#endif

_invalid_parameter is located at ucrtbased.

изображение

@libbooze
Copy link
Author

libbooze commented Nov 12, 2021

#ifndef _ACRTIMP
    #if defined _CRTIMP && !defined _VCRT_DEFINED_CRTIMP
        #define _ACRTIMP _CRTIMP
    #elif !defined _CORECRT_BUILD && defined _DLL
        #define _ACRTIMP __declspec(dllimport)
    #else
        #define _ACRTIMP
    #endif
#endif

_invalid_parameter is located at ucrtbased.

It is possible that my project does not link it, or is it linked implicitly whenever I use MSFT STL?

I tried your suggestion, I get:

<myfile_name>.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) void __cdecl _invalid_parameter(wchar_t const *,wchar_t const *,wchar_t const *,unsigned int,unsigned __int64)" (_imp?_invalid_parameter@@YAXPEB_W00I_K@Z) referenced in function "class std::basic_string<char,struct std::char_traits,class std::allocator > __cdecl std::operator+<char,struct std::char_traits,class std::allocator >(class std::basic_string<char,struct std::char_traits,class std::allocator > &&,class std::basic_string<char,struct std::char_traits,class std::allocator > &&)" (??$?HDU?$char_traits@D@std@@v?$allocator@D@1@@std@@ya?AV?$basic_string@DU?$char_traits@D@std@@v?$allocator@D@2@@0@$$QEAV10@0@Z)

my code is:


#ifdef _DEBUG

 

#ifndef _ACRTIMP

#if defined _CRTIMP && !defined _VCRT_DEFINED_CRTIMP

#define _ACRTIMP _CRTIMP

#elif !defined _CORECRT_BUILD && defined _DLL

#define _ACRTIMP __declspec(dllimport)

#else

#define _ACRTIMP

#endif

#endif

 

_ACRTIMP void __cdecl _invalid_parameter(

    _In_opt_z_ wchar_t const*,

    _In_opt_z_ wchar_t const*,

    _In_opt_z_ wchar_t const*,

    _In_ unsigned int,

    _In_ unsigned __int64);

#endif

When I remove _ACRTIMP from function signature I get inconsistent dll linkage error/warning, so it seems that compiler has already seen this function before... weird.

@fsb4000
Copy link
Contributor

fsb4000 commented Nov 12, 2021

Try add the macro :

#define _STL_CRT_SECURE_INVALID_PARAMETER(expr) _CRT_SECURE_INVALID_PARAMETER(expr)

and probably you can delete everything that we added before...

@libbooze
Copy link
Author

and probably you can delete everything that we added before...

It worked. Again thank you very much.

Out of curiosity: is this a workaround for pybind undefined behavior or is it a fix for STL?

@fsb4000
Copy link
Contributor

fsb4000 commented Nov 12, 2021

is this a workaround for pybind undefined behavior or is it a fix for STL?

You linked to release UCRT, this is why the function is not found.
macro _DEBUG says to STL that it's debug mode so it used the function because we think it's available.

With _CRT_SECURE_INVALID_PARAMETER we don't use _invalid_parameter directly. And just use what UCRT use for debug message. And because UCRT in Release the macro disapear and everything works.
Technically, it's revert the commit: https://github.com/microsoft/STL/pull/2054/files

We started passing parameters to the debug message box in a different way because with ranges and namespaces function name might be very big. Read the discussion: #2054

@libbooze
Copy link
Author

You linked to release UCRT, this is why the function is not found.

I was trying to build Debug build, I presume pybind messed stuff up then...

@CaseyCarter
Copy link
Member

To be clear: redefinition of _DEBUG is unsupported. The macro exists so the compiler can communicate to the runtime libraries whether we're currently building Debug or Release. It is in no way intended to be a customization point that user code manipulates to change behavior.

If pybind is touching _DEBUG, then you should strongly suggest that they stop doing so =)

I'm going to close this as invalid - I see no STL bug here - but feel free to the continue discussion and attempts to find an even-more-egregious hack to workaround the problems this egregious hack is causing if you like.

@CaseyCarter CaseyCarter added the invalid This issue is incorrect or by design label Nov 12, 2021
@libbooze
Copy link
Author

To be clear: redefinition of _DEBUG is unsupported.

Thank you Casey, I guessed so since it starts with _ but was not sure since it is a macro.

Will open pybind bug.

I wish C++ compilers knew about special macros and disallowed/warned on redefinition, but then again I wish C++ had so many thing...

@fsb4000
Copy link
Contributor

fsb4000 commented Nov 13, 2021

I wish C++ compilers knew about special macros and disallowed/warned on redefinition

@libbooze , clang warns about it: https://gcc.godbolt.org/z/cnE3jeaKf

@libbooze
Copy link
Author

I wish C++ compilers knew about special macros and disallowed/warned on redefinition

@libbooze , clang warns about it: https://gcc.godbolt.org/z/cnE3jeaKf

Very nice, if somebody from MSFT is reading: if your compiler dev friends are bored ;) this would be a nice warning to add.

@fsb4000
Copy link
Contributor

fsb4000 commented Nov 13, 2021

@libbooze

I don't think the issue is reading by anyone besides me, Casey and you :)

But I found the same requested feature at DevCom by https://github.com/tiagomacarios: https://developercommunity.visualstudio.com/t/Add-a-checker-to-flag-functions-in-the-r/1524874

You can upvote it. I upvoted. (I am "Игорь Жуков" there)

Maybe it will get enough upvotes for the developers to start implementing it. :)

@henryiii
Copy link

If pybind is touching _DEBUG, then you should strongly suggest that they stop doing so =)

We have to undefine _DEBUG during the inclusion of Python.h; we have done this on MSVC 2015-2019 for many years. The undefine happens only when the C-only sources of Python are included, and is immediately restored after the Python define.

If we don't, then Python.h requires linking to a debug build of Python, which users often do not have. They are interested in debugging their own code, not CPython, and there's often no debug component distributed to link to even if they knew how.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This issue is incorrect or by design
Projects
None yet
Development

No branches or pull requests

4 participants