-
-
Notifications
You must be signed in to change notification settings - Fork 175
Segmentation fault message #2516
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
base: master
Are you sure you want to change the base?
Conversation
andychu
left a comment
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.
This looks like a good start, thanks
It is possible to add a spec test for it? We could kill the child process like in spec/builtin-kill
Does this message happen in both batch mode and interactive mode?
core/process.py
Outdated
|
|
||
|
|
||
| _SIGNAL_MESSAGES = { | ||
| SIGSEGV: 'Segmentation fault', |
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.
Hm do other shells hard-code these strings?
Or is there something like strerror() for errno with these strings?
Sometimes we have to look at the C source of other shells, as for the default path
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.
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.
Ah thanks for looking it up
Although I asked Claude and it shows that Python 3 has strsignal(), which is a POSIX 2008 API
Bash predates this API -- so it has this comment:
/* siglist.c -- signal list for those machines that don't have one. */
But unforunately our Python 2 build doesn't have a binding for it
https://docs.python.org/3/library/signal.html#signal.strsignal
https://claude.ai/share/5c6e5003-67c6-46fb-989b-0db63fc8d4ce
Could you try adding a binding in libc.c and libc.pyi , and you can test it with libc_test.py ?
build/py.sh pylibc
builds it and run tests
I think the binding could be similar to the one for strerror() in pyext/posixmodule.c
i.e. it just does a bit of arg parsing and so forth
PyDoc_STRVAR_remove(posix_strerror__doc__,
"strerror(code) -> string\n\n\
Translate an error code to a message string.");
static PyObject *
posix_strerror(PyObject *self, PyObject *args)
{
int code;
char *message;
if (!PyArg_ParseTuple(args, "i:strerror", &code))
return NULL;
message = strerror(code);
if (message == NULL) {
PyErr_SetString(PyExc_ValueError,
"strerror() argument out of range");
return NULL;
}
return PyString_FromString(message);
}
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 can try that out
|
Some testing about what other shells do hm only bash does it? interesting And it's not just interactive |
|
This is interactive bash ... |
|
Interactive zsh So zsh only does it interactively ? But bash does it in both? hm same with |
|
I guess we can do whatever is simpler - it seems simpler to do the same thing in interactive and batch |
|
Also here is the binding from Python 3 in I usually try to adapt a similar example, so between those 2 examples, we should be able to make a Python 2 binding for |
|
@andychu check now... |
andychu
left a comment
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.
Looks great, glad you got it working!
Now we can simplify the code
There will probably be a C++ error once that happens, because C++ also needs strsignal()
But I can help with that
| # Not testing errno case | ||
|
|
||
| def testStrsignal(self): | ||
| self.assertEqual('Segmentation fault', libc.strsignal(11)) |
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.
Could do signal.SIGTERM to be more readable
core/process.py
Outdated
| SIGABRT: 'Aborted', | ||
| SIGILL: 'Illegal instruction', | ||
| SIGFPE: 'Floating point exception', | ||
| SIGBUS: 'Bus error', |
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 should remove this dict now, and use libc.strsignal()!
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 should remove this dict now, and use libc.strsignal()!
Done
f81c044 to
a0b7a56
Compare
|
This looks good, thanks! How about adding a spec test for it? That will make it easier to port to C++, so we can see that it does the same thing I think something like this should work Maybe in |
Okay |
|
The test is tickling the right behavior: But it needs a better assertion It is failing with OSH in Python, and passing with OSH in C++ It should be the other way around Sometimes it may be useful to redirect Using It would be nice if the test passes with bash too |
|
Hm yeah other shells don't print https://op.oilshell.org/uuu/github-jobs/10636/ovm-tarball.wwz/_tmp/spec/osh-py/background.html |
|
Oh and if you haven't already seen it, the commands to reproduce the spec test failures are here: https://github.com/oils-for-unix/oils/wiki/Oils-Dev-Cheat-Sheet |
|
Hm the spec tests should still be failing locally for you ? e.g. And I think Github sends you a message when the CI fails? Right now it is failing -- bash and OSH now pass , but the other 2 shells need to be marked https://op.oilshell.org/uuu/github-jobs/10649/ovm-tarball.wwz/_tmp/spec/osh-py/background.html https://github.com/oils-for-unix/oils/wiki/Spec-Tests And then also we will need to allow the C++ failure for now, like this Sometimes the spec tests are half the problem, but they are important! Thanks for working on this |
Attends to this issue #722
close #722