[vim/vim] Fix that setvbuf causes assertion error (PR #12467)
K.Takata
When Vim is compiled with debug mode on MSVC, the following command causes an assertion error:
> vimd -e -s -c q
Debug Assertion Failed!
Program: C:\work\vim-msvc\src\vim32d.dll
File: minkernel\crts\ucrt\src\appcrt\stdio\setvbuf.cpp
Line: 55
Expression: 2 <= buffer_size_in_bytes && buffer_size_in_bytes <= INT_MAX
This is because MSVC's setvbuf() doesn't accept 0 as the 4th parameter. https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/setvbuf?view=msvc-170
Additionally, it seems that passing NULL to the 2nd parameter is no-op in some C libraries.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/12467
Commit Summary
- dea07cf Fix that setvbuf causes assertion error
File Changes
(1 file)
- M src/main.c (5)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christ van Willegen
When Vim is compiled with debug mode on MSVC, the following command causes an assertion error:
vim-dev ML
Op di 30 mei 2023 08:00 schreef K.Takata ***@***.***>:
> When Vim is compiled with debug mode on MSVC, the following command causes
> an assertion error:
>
Christ van Willegen
>
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
codecov[bot]
Codecov Report
Merging #12467 (dea07cf) into master (c9fbd25) will increase coverage by
0.65%.
The diff coverage is100.00%.
@@ Coverage Diff @@ ## master #12467 +/- ## ========================================== + Coverage 82.06% 82.72% +0.65% ========================================== Files 160 150 -10 Lines 193588 180400 -13188 Branches 43472 40548 -2924 ========================================== - Hits 158877 149242 -9635 + Misses 21853 18200 -3653 - Partials 12858 12958 +100
| Flag | Coverage Δ | |
|---|---|---|
| huge-clang-none | 82.72% <100.00%> (+<0.01%) |
⬆️ |
| linux | 82.72% <100.00%> (+<0.01%) |
⬆️ |
| mingw-x64-HUGE | ? |
|
| mingw-x86-HUGE | ? |
|
| windows | ? |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/main.c | 84.72% <100.00%> (+0.75%) |
⬆️ |
... and 143 files with indirect coverage changes
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Bram Moolenaar
Making the buffer a global variable should avoid a warning for leaked memory.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
K.Takata
@k-takata pushed 1 commit.
- 03fb2d2 Free the buffer when EXITFREE is defined
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
K.Takata
Updated to free the buffer when EXITFREE is defined.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Christian Brabandt
@chrisbra commented on this pull request.
In src/main.c:
> @@ -436,6 +442,19 @@ main
#endif // NO_VIM_MAIN
#endif // PROTO
+#if defined(EXITFREE) || defined(PROTO)
+ void
+free_vbuf(void)
+{
+ if (s_vbuf != NULL)
+ {
+ setvbuf(stdout, NULL, _IONBF, 0);
setvbuf(stdout, NULL, _IONBF, 0);
Is this call required? Because now you are introducing the same problem that this PR is fixing? Shouldn't size be larger than 1?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
K.Takata
@k-takata commented on this pull request.
In src/main.c:
> @@ -436,6 +442,19 @@ main
#endif // NO_VIM_MAIN
#endif // PROTO
+#if defined(EXITFREE) || defined(PROTO)
+ void
+free_vbuf(void)
+{
+ if (s_vbuf != NULL)
+ {
+ setvbuf(stdout, NULL, _IONBF, 0);
Before freeing s_vbuf we should stop using it in setvbuf().
So I use _IONBF (not _IOLBF) here to stop buffering.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Christian Brabandt
@chrisbra commented on this pull request.
In src/main.c:
> @@ -436,6 +442,19 @@ main
#endif // NO_VIM_MAIN
#endif // PROTO
+#if defined(EXITFREE) || defined(PROTO)
+ void
+free_vbuf(void)
+{
+ if (s_vbuf != NULL)
+ {
+ setvbuf(stdout, NULL, _IONBF, 0);
okay, but what about the size then? I think 0 is not valid according to your link.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
K.Takata
@k-takata commented on this pull request.
In src/main.c:
> @@ -436,6 +442,19 @@ main
#endif // NO_VIM_MAIN
#endif // PROTO
+#if defined(EXITFREE) || defined(PROTO)
+ void
+free_vbuf(void)
+{
+ if (s_vbuf != NULL)
+ {
+ setvbuf(stdout, NULL, _IONBF, 0);
The assertion error relates only to _IOFBF and _IOLBF, not to _IONBF.
_IONBFNo buffer is used, regardless ofbufferorsize.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Christian Brabandt
@chrisbra commented on this pull request.
In src/main.c:
> @@ -436,6 +442,19 @@ main
#endif // NO_VIM_MAIN
#endif // PROTO
+#if defined(EXITFREE) || defined(PROTO)
+ void
+free_vbuf(void)
+{
+ if (s_vbuf != NULL)
+ {
+ setvbuf(stdout, NULL, _IONBF, 0);
ah, thanks. Makes sense
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
K.Takata
@k-takata commented on this pull request.
In src/main.c:
> @@ -436,6 +442,19 @@ main
#endif // NO_VIM_MAIN
#endif // PROTO
+#if defined(EXITFREE) || defined(PROTO)
+ void
+free_vbuf(void)
+{
+ if (s_vbuf != NULL)
+ {
+ setvbuf(stdout, NULL, _IONBF, 0);
If you have installed MSVC, you should be able to find the source code of setvbuf() here:
C:\Program Files (x86)\Windows Kits\10\Source\<BuildNumber>\ucrt\stdio\setvbuf.cpp
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Bram Moolenaar
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.