[vim/vim] Added test of deadly signal SIGTERM (#6055)
Dominique Pellé
Deadly signal should preserve changes in swap file before vim is killed.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/6055
Commit Summary
- Added test of deadly signal SIGTERM
File Changes
- M src/testdir/test_signals.vim (44)
Patch Links:
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
Dominique Pellé
@dpelle commented on this pull request.
In src/testdir/test_signals.vim:
> + endif
+ let cmd = GetVimCommand()
+ if cmd =~ 'valgrind'
+ throw 'Skipped: cannot test signal TERM with valgrind'
+ endif
+
+ let buf = RunVimInTerminal('Xsig_TERM', {'rows': 6})
+ let pid_vim = term_getjob(buf)->job_info().process
+
+ call term_sendkeys(buf, ":call setline(1, 'foo')\n")
+ call WaitForAssert({-> assert_equal('foo', term_getline(buf, 1))})
+
+ call assert_false(filereadable('Xsig_TERM'))
+ exe 'silent !kill -s TERM ' .. pid_vim
+ call WaitForAssert({-> assert_equal('Vim: Caught deadly signal TERM', term_getline(buf, 1))})
+ call WaitForAssert({-> assert_match('Vim: preserving files\.\.\.$', term_getline(buf, 2))})
One thing I noticed, is that when doing kill -s TERM <pid_of_vim>
vim outputs:
Vim: Caught deadly signal TERM
Vim: preserving files...
Vim: Finished.
Notice the odd spacing.
Shouldn't we do this:
diff --git a/src/misc1.c b/src/misc1.c
index 6686a3514..a370a82f8 100644
--- a/src/misc1.c
+++ b/src/misc1.c
@@ -2174,7 +2174,7 @@ preserve_exit(void)
{
if (buf->b_ml.ml_mfp != NULL && buf->b_ml.ml_mfp->mf_fname != NULL)
{
- OUT_STR("Vim: preserving files...\n");
+ OUT_STR("Vim: preserving files...\r\n");
screen_start(); // don't know where cursor is now
out_flush();
ml_sync_all(FALSE, FALSE); // preserve all swap files
@@ -2184,7 +2184,7 @@ preserve_exit(void)
ml_close_all(FALSE); // close all memfiles, without deleting
- OUT_STR("Vim: Finished.\n");
+ OUT_STR("Vim: Finished.\r\n");
getout(1);
}
diff --git a/src/os_unix.c b/src/os_unix.c
index 8424b11a3..596fb0e90 100644
--- a/src/os_unix.c
+++ b/src/os_unix.c
@@ -1083,10 +1083,10 @@ deathtrap SIGDEFARG(sigarg)
// No translation, it may call malloc().
#ifdef SIGHASARG
- sprintf((char *)IObuff, "Vim: Caught deadly signal %s\n",
+ sprintf((char *)IObuff, "Vim: Caught deadly signal %s\r\n",
signal_info[i].name);
#else
- sprintf((char *)IObuff, "Vim: Caught deadly signal\n");
+ sprintf((char *)IObuff, "Vim: Caught deadly signal\r\n");
#endif
// Preserve files and exit. This sets the really_exiting flag to prevent
It would then output the nicer looking:
Vim: Caught deadly signal TERM
Vim: Finished.
Terminated
Codecov
Codecov Report
Merging #6055 into master will increase coverage by
0.02%.
The diff coverage isn/a.
@@ Coverage Diff @@ ## master #6055 +/- ## ========================================== + Coverage 87.09% 87.11% +0.02% ========================================== Files 142 142 Lines 156609 156609 ========================================== + Hits 136397 136430 +33 + Misses 20212 20179 -33
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/term.c | 81.80% <0.00%> (-0.12%) |
⬇️ |
| src/message.c | 88.64% <0.00%> (-0.05%) |
⬇️ |
| src/channel.c | 89.82% <0.00%> (-0.04%) |
⬇️ |
| src/buffer.c | 87.02% <0.00%> (+0.04%) |
⬆️ |
| src/os_unix.c | 69.56% <0.00%> (+0.04%) |
⬆️ |
| src/gui_gtk_x11.c | 57.83% <0.00%> (+0.09%) |
⬆️ |
| src/sign.c | 94.94% <0.00%> (+0.17%) |
⬆️ |
| src/if_xcmdsrv.c | 88.90% <0.00%> (+0.17%) |
⬆️ |
| src/memline.c | 79.97% <0.00%> (+0.26%) |
⬆️ |
| src/window.c | 89.71% <0.00%> (+0.44%) |
⬆️ |
| ... and 1 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered by Codecov. Last update 3b6a6eb...c4bd84e. Read the comment docs.
Dominique Pellé
Despite testing handling of deadly signal, the PR does not improve test coverage,
presumably because coverage is not saved when Vim is killed by a deadly signal.
For example the line that prints Vim: Caught deadly signal %s\n in os_unix.c is
still marked as not covered despite being clearly covered since it's in the test
expectation.
https://codecov.io/gh/vim/vim/src/c4bd84ea68bf4ac8f0210448475d59e0295cdcfb/src/os_unix.c#L1086
I'm not aware of any way of having this code reported as covered.
Bram Moolenaar
Dominique wrote:
> Despite testing handling of deadly signal, the PR does not improve test coverage,
> presumably because coverage is not saved when Vim is killed by a deadly signal.
> For example the line that prints `Vim: Caught deadly signal %s\n` in os_unix.c is
> still marked as not covered despite being clearly covered since it's in the test
> expectation.
>
> https://codecov.io/gh/vim/vim/src/c4bd84ea68bf4ac8f0210448475d59e0295cdcfb/src/os_unix.c#L1086
>
> I'm not aware of any way of having this code reported as covered.
When forcefully terminating the buffer won't be flushed.
Perhaps fflush(NULL) just before terminating would help?
Oh, there also is __gcov_flush(). Would require a -DUSE_GCOV_FLUSH
build flag and an #ifdef.
--
hundred-and-one symptoms of being an internet addict:
89. In addition to your e-mail address being on your business
cards you even have your own domain.
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
Dominique Pellé
@brammool wrote:
Oh, there also is __gcov_flush(). Would require a -DUSE_GCOV_FLUSH
build flag and an #ifdef.
Thanks. Trying it. I see that we already have WE_ARE_PROFILING so
I'm using that rather than USE_GCOV_FLUSH.
Dominique Pellé
Calling __gcov_flush() did not help. Probably it's not signal safe.
So I will revert the call to __gcov_flush().
Dominique Pellé
Tony Mechelynck
vim-dev ML
Bram Moolenaar
It appears that with Travis WE_ARE_PROFILING is not defined. That would explain why adding the flush didn't work. Please try this:
--- vim82/src/os_unix.c 2020-05-10 14:13:58.867609356 +0200
+++ os_unix.c 2020-05-10 20:56:28.739973990 +0200
@@ -3292,6 +3292,10 @@
}
}
+#ifdef USE_GCOV_FLUSH
+extern void __gcov_flush();
+#endif
+
void
mch_exit(int r)
{
@@ -3338,6 +3342,9 @@
}
out_flush();
ml_close_all(TRUE); // remove all memfiles
+#ifdef USE_GCOV_FLUSH
- __gcov_flush();
+#endif
may_core_dump();
#ifdef FEAT_GUI
if (gui.in_use)
and add -DUSE_GCOV_FLUSH in the travis config.
—
You are receiving this because you commented.
Bram Moolenaar
Dominique wrote:
> Calling `__gcov_flush()` did not help. Probably it's not signal safe.
Looking at the coverage info, it does appear to be higher:
https://codecov.io/gh/vim/vim/commits
87.12% compared to 87.09% - 87.10%
I can't try, for me __gcov_flush() does not exist. Perhaps I need
another link argument.
Try putting the call in may_core_dump(), that is the last place we get
to when handling a TERM signal. This also means we don't go through
exit(), which is where the flush normally happens.
--
From "know your smileys":
y:-) Bad toupee
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
You are receiving this because you commented.
Dominique Pellé
I just rebased this PR after patch 8.2.0739 which calls __gcov_flush.
I now see that the deadly signal is marked as covered, good!
I thought I tried something like that earlier which did not work. But
now it works somehow after patch 8.2.0739.
—
You are receiving this because you commented.