Pali Rohár <[email protected]> wrote:
> Hello,
>
> I looked at these issues. And basically there are 3 things which could
> be done independently.
>
> 1. Decide whether C assert() macro would respect _UNICODE and use CRT
> _assert() or CRT _wassert() function. Or if it would not respect
> _UNICODE and always forces usage of UTF-16 _wassert() variant.
>
> 2. Fixing of mingw-w64 _assert() function. Or removal of mingw-w64
> _assert() implementation and let to use just broken DLL implementation.
>
> 3. Changing/Fixing of the mingw-w64 _wassert() emulation.
>
> About application usage. I think it is not important if the recent MS
> doc say that some function (e.g. _assert()) should not be used by
> application or not. Application would always use functions which are
> available and are part of CRT DLL ABI. Saying "do not use" or forcing by
> "removal of symbol" would just cause breaking.
Both `_assert` and `_wassert` are documented, so removing them is totally out
of question. OTOH, I still would suggest applications to not call them
directly. For example, MSVC no longer exposes `_assert`, so any application
which references it will not be able to compile with MSVC at this point.
> Also I think that it is not important of usage of function, macro,
> parameter flag, enum, whatever... is going to be forbidden in header
> file or not. Lot of times I saw that applications are doing thing like
> "#ifndef FLAG\n#define FLAG ...\n#endif" with comment that some older
> SDK or broken SDK do not provide FLAG. And personally I also did it lot
> of times when I figured out that FLAG was not exported.
>
> So the "Expose _O_{W,U16,U8}TEXT macros only for CRTs..." part is not a
> good idea as described above. At least this is what I think.
IMO, if an application compiled against pre-msvcr80.dll calls
_setmode (fd, _O_U8TEXT)
It is a clearly bug in an application. It attempts to use a feature which is
not supported by that CRT; it will compile but will behave differently than
expected.
> Also important is that mingw-w64 header files are for C and C++
> languages but there are usage also for other languages, which directly
> uses mingw-w64-crt libraries. My feeling is that separation of
> mingw-w64-crt and mingw-w64-headers is a good idea to show what is
> provided for linker and what is provided for C/C++ language.
I would prefer if we could catch errors at compilation time whenever possible.
> Now about 1. forward C assert() macro to CRT _wassert() function. I have
> subjective preference to not do it. I understand concerns and I right
> now I do not see any good solution. My feeling is that both are bad.
> I'm looking at this from direction: I explicitly did not set _UNICODE
> so I want from my application (which includes mingw-w64 header files) to
> not emit _UNICODE/UTF-16 symbols, for example for compatibility with
> non-UNICODE systems. MS is for a longer time trying to convince
> developers to use UNICODE/UTF-16 functions and entry points and the
> change which switched assert to _wassert in MS header files was done
> long time ago (when they completely dropped Win9x support). But now I
> think that direction is moving from UTF-16 to UTF-8. I know this is not
> very good argument, just my subjective preference. On the other hand,
> issues which you pointed out are valid which should be solved. And if
> there is no other option how to solve those issues then probably the
> switch is the direction. But before that I would like to try fixing
> other options.
IMO, Windows-only code must use Unicode whenever possible; the only reason not
to is Win9x systems which lack Unicode support.
If someone intends to support only Windows 10 and later, using UTF-8 is
probably the best solution. However, if they want to support pre-Windows 10
systems, they should use UTF-16; otherwise they will be limited to locale's
ANSI code pages.
> Also I do not like the fact that the assert() macro would be forwarded
> to _wassert() function, which for older libs would be emulated by
> mingw-w64 which at the end would call _assert() by doing reallocations
> and conversions back and forward.
From user perspective, it does not really matter how `assert` is implemented;
it could be implemented as simple as macro which calls `fprintf`, `fflush` and
`abort`.
Microsoft docs[1] mention the following detail:
```
The _assert and _wassert functions are internal CRT functions. They help
minimize the code required in your object files to support assertions. We don't
recommend that you call these functions directly.
```
> About 2. removal vs fix of mingw-w64 _assert() function. If switch in 1.
> would happen then I think there is no reason to remove _assert() and I
> would like to try to fix it, not remove.
My proposal is to simply remove the wrapper around CRT `_assert` which we use
to handle Unicode translation modes.
> About 3. fixing of _wassert() emulation. This emulation always uses the
> _assert() function for its implementation. Hence if the _assert stays
> broken then the _wassert() emulation would be also always broken,
> whatever would be done.
Basically, CRT has both `_wassert` and support for Unicode translation modes,
or neither. If we use `_wassert`, we do not have to worry about `_assert` not
working with Unicode translation modes. Otherwise, `_wassert` emulation simply
redirects to `_assert`, while doing its best at converting its arguments.
> So I would take 2. + 3. together and my ideas are:
>
> - _set_error_mode is not available in crtdll.dll, msvcrt10.dll and
> msvcrt20.dll CRT libs; but this is not a problem, I can provide
> working "emulation" of this function which would return correct value
> for _set_error_mode (_REPORT_ERRMODE) call. This is because these
> libraries always prints message to stderr via fprintf; not to GUI.
I wanted to suggest prividing emulation for `_set_error_mode ` as a follow-up,
so we could query current setting from `_wassert` emulation and choose correct
code page for conversion.
> - about cleaning resources if the _(w)assert returns back. I would
> propose to use SEH unwind handler to do it. I have already looked at
> SEH deeply and we can do it. Also longjmp _by default_ on ms builds is
> calling the unwind handler, so I think it would be a good idea to do
> it also in mingw-w64.
Sounds really overcomplicated to me.
> - malloc vs static buffer vs _alloca. Usage of _alloca or variable
> length arrays are mostly means issues in code. And having them in
> "assert" implementation (which is called by application then something
> bad happens) sounds like a trouble. Specially if compiler generates
> code for stack overflow protection for any alloca/VLA usage and calls
> assert if overflow happens. We could have potentially new issues.
> I'm not sure if compilers are doing this now, but I can imagine that
> this is a valid thing to do.
IMO, using `alloca` is totally OK as long as it is used carefully. My proposed
changes limit allocation sizes to `BUFSIZ` (512) and `FILENAME_MAX` (260), and
even try to allocate smaller buffers when we can.
> But with static buffers I understand issue that multiple threads
> cannot at the same time call assert. And when starting to use signal
> handlers, then issue can happen even with one thread when signal
> handler calls assert with a new signal handler and both would want to
> longjump from it based on some condition. In this case the assert
> temporary buffer would be used just by one thread, but by two
> different callers intermixed. So even possible thread local buffer
> would not help here.
>
> - usage of GetACP() for codepage. It is wrong like usage of any other
> option. When printing to console, it is needed to use function
> GetConsoleOutputCP().
From what I know, CRT stdio functions, when write to terminals, convert strings
to code page returned by `GetConsoleOutputCP`. What we must keep in mind is
that it interprets original (narrow) strings using code page for active CRT
locale (`setlocale`).
> - problem with encoding comes from the fact that _compiler_ fills the
> assert message in its own encoding. But at the same time the _assert
> function (provided by DLL) has some encoding contract (with bugs...)
> how it should be called. So I have feeling that here we are mixing
> more separate issues into one functionality and that is why it is in
> this bad shape. gcc has -finput-charset=charset and
> -fexec-charset=charset options (and maybe some other internal options)
> which if I understand correctly can affect the asserted message which
> is then expanded into _assert call. Should not we rather have separate
> mingw-w64 function e.g. _assert_called_from_gcc_option1 (or whatever)
> and then CRT _assert function which is according to the ABI?
Sounds like another overcomplication to me. Basically, all narrow CRT functions
have this issue, and this is why use of Unicode is preferred.
> Because my feeling is that "assert" is a C macro mostly tied with
> compiler nad "_assert" is MS CRT function which follows some ABI and
> these two things needs some layer to be correctly connected. Here we
> are trying to change "_assert" which looks to be wrong. For example
> some other compiler/language can use "_assert" function directly if it
> is not C/C++.
IMO, they should not.
> About your last point C. We can check if the current symbol for e.g.
> _assert points to msvcrt.dll or to some other library. And this check
> can be in the per-CRT library. This should work correctly as forward are
> done at PE loader level. And it will sets symbol to point to different
> library. I think that if it is done in this way it should not have a
> problem. The point is to not use GetProcAddress, but rather to figure
> out HMODULE directly from __imp_<symbol> pointer at runtime. And I think
> that in the same way could be implemented _set_error_mode (_REPORT_ERRMODE)
> call to work reliable in case crtdll.dll/msvcrt20.dll (w)print functions
> are redirected to msvcrt.dll.
Sounds like an option.
> I would like to try to implement alternative solution to these issues.
> Maybe I will figure out that it is not possible, and your change is the
> the best what can be done, even it is not ideal. But that is one of
> scenarios.
>
> If I understand correctly, right now in the mingw-w64 git code we have
> memory leaks in assert and wrong encoding of non-ASCII assert messages
> and damaging of _O_WTEXT/_O_U16TEXT/_O_U8TEXT mode in case the assert is
> ignored and is going to continue. IMHO all those cases are not the
> primary code path, was there a longer time, so should not cause an issue
> if these issues would be fixed/merged later and not immediately.
- Kirill Makurin
[1]
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/assert-macro-assert-wassert
_______________________________________________
Mingw-w64-public mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
