The way cxx bridge works, it doesn't recognize any types from another module as
being shared cxx bridge types with generations native to both C++ and Rust,
meaning every module that was going to use function pointers would have to
define its own `c_void` type (because cxx bridge doesn't recognize any of
libc::c_void, std::ffi::c_void, or autocxx::c_void).
FFI on other platforms has long used the equivalent of `uint8_t *` as an
alternative to `void *` for code where `void` was not available or was
undesirable for some reason. We can join the club - this way we can always use
`* {const|mut} u8` in our rust code and `uint8_t *` in our C++ code to pass
around parameters or values over the C abi.
I needed to rename some types already ported to rust so they don't clash with
their still-extant cpp counterparts. Helper ffi functions added to avoid needing
to dynamically allocate an FdMonitorItem for every fd (we use dozens per basic
prompt).
I ported some functions from cpp to rust that are used only in the backend but
without removing their existing cpp counterparts so cpp code can continue to use
their version of them (`wperror` and `make_detached_pthread`).
I ran into issues porting line-by-line logic because rust inverts the behavior
of `std::remove_if(..)` by making it (basically) `Vec::retain_if(..)` so I
replaced bools with an explict enum to make everything clearer.
I'll port the cpp tests for this separately, for now they're using ffi.
Porting closures was ugly. It's nothing hard, but it's very ugly as now each
capturing lambda has been changed into an explicit struct that contains its
parameters (that needs to be dynamically allocated), a standalone callback
(member) function to replace the lambda contents, and a separate trampoline
function to call it from rust over the shared C abi (not really relevant to
x86_64 w/ its single calling convention but probably needed on other platforms).
I don't like that `fd_monitor.rs` has its own `c_void`. I couldn't find a way to
move that to `ffi.rs` but still get cxx bridge to consider it a shared POD.
Every time I moved it to a different module, it would consider it to be an
opaque rust type instead. I worry this means we're going to have multiple
`c_void1`, `c_void2`, etc. types as we continue to port code to use function
pointers.
Also, rust treats raw pointers as foreign so you can't do `impl Send for * const
Foo` even if `Foo` is from the same module. That necessitated a wrapper type
(`void_ptr`) that implements `Send` and `Sync` so we can move stuff between
threads.
The code in fd_monitor_t has been split into two objects, one that is used by
the caller and a separate one associated with the background thread (this is
made nice and clean by rust's ownership model). Objects not needed under the
lock (i.e. accessed by the background thread exclusively) were moved to the
separate `BackgroundFdMonitor` type.
If EINTR caused by SIGINT is encountered while writing to the
`fd_output_stream_t` output fd, mark the output stream as errored and return
false to the caller but do not visibly complain.
Addressing the outstanding TODO notwithstanding, this is needed to avoid
littering the tty with spurious errors when the user hits Ctrl-C to abort a
long-running builtin's output (w/ the primary example being `history`).
This reverts commit 3d8f98c395.
In addition to the issues mentioned on the GitHub page for this commit,
it also broke the CentOS 7 build.
Note one can locally test the CentOS 7 build via:
./docker/docker_run_tests.sh ./docker/centos7.Dockerfile
Be more careful with sign extension issues stemming from the differences in how
an untyped literal is promoted to an integer vs how a typed (and signed) `char`
is promoted to an integer.
Also convert some `const[expr] static xxx` to `const[expr] xxx` where it makes
sense to let the compiler deduce on its own whether or not to allocate storage
for a constant variable rather than imposing our view that it should have STATIC
storage set aside for it.
A few call sites were not making use of the `XXX_LEN` definitions and were
calling `strlen(XXX)` - these have been updated to use `const_strlen(XXX)`
instead.
I'm not sure if any toolchains will have raise any issues with these changes...
CI will tell!
Let's hope this doesn't causes build failures for e.g. musl: I just
know it's good on macOS and our Linux CI.
It's been a long time.
One fix this brings, is I discovered we #include assert.h or cassert
in a lot of places. If those ever happen to be in a file that doesn't
include common.h, or we are before common.h gets included, we're
unawaringly working with the system 'assert' macro again, which
may get disabled for debug builds or at least has different
behavior on crash. We undef 'assert' and redefine it in common.h.
Those were all eliminated, except in one catch-22 spot for
maybe.h: it can't include common.h. A fix might be to
make a fish_assert.h that *usually* common.h exports.
Posix allows this as an alternative with the same semantics for read.
Found in conjunction with #9067.
Should be no functional difference on other systems.
A command like "printf nonewline | sed s/x/y/" does not print a
concluding newline, whereas "printf nnl | string replace x y" does.
This is an edge case -- usually the user input does have a newline at
the end -- but it seems still better for this command to just forward
the user's data.
Teach most string subcommands to check if stdin is missing the trailing
newline, and stop adding one in that case.
This does not apply when input is read from commandline arguments.
* Most subcommands stop adding the final newline, because they don't
really care about newlines, so besides their normal processing,
they just want to preserve user input. They are:
* string collect
* string escape/unescape
* string join¹
* string lower/upper
* string pad
* string replace
* string repeat
* string sub
* string trim
* string match keeps adding the newline, following "grep". Additionally,
for string match --regex, it's important to output capture groups
separated by newlines, resulting in multiple output lines for an
input line. So it is not obvious where to leave out the newline.
* string split/split0 keep adding the newline for the same reason --
they are meant to output multiple elements for a single input line.
¹) string join0 is not changed because it already printed a trailing
zero byte instead of the trailing newline. This is consistent
with other tools like "find -print0".
Closes#3847
This finds the first broken component, to help people figure out where
they misspelt something.
E.g.
```
echo foo >/usr/lob/systemd/system/machines.target.wants/var-lib-machines.mount
```
will now show:
```
warning: Path '/usr/lob' does not exist
```
which would help with seeing that it should be "/usr/lib".
clang-tidy wrongly sees an std::move to a const ref parameter and
believes it to be pointless. The copy constructor however is deleted.
Signed-off-by: Rosen Penev <rosenp@gmail.com>
With something like
```
history | head -n 1
```
this would error "write: Broken pipe", which is just annoying. There
is no *problem* here, `head` closes this on purpose.
Fixes#7924.
This correctly sets $status when a builtin succeeds but its output fails;
for example if the output is redirected to a file and that write fails.
Fixes#7857
io_buffer_t is a buffer that fills itself by reading from a file
descriptor (typically a pipe). When the file descriptor is widowed, the
operation completes, and it reports completion by marking a
`std::promise<void>`. The "main thread" waits for this by waiting on the
promise's future. However TSan was reporting that the future's destructor
races with its promise's wait method. It's not obvious if this is valid,
but we can fix it by keeping the promise alive until the io_buffer_t is
deallocated.
This fixes the TSan issues reported under
`complete_background_fillthread_and_take_buffer` for #7681 (but there
are other unresolved issues).
This concerns how fish prevents its own fds from interfering with
user-defined fd redirections, like `echo hi >&5`. fish has historically
done this by tracking all user defined redirections when running a job,
and ensuring that pipes are not assigned the same fds. However this is
annoying to pass around - it means that we have to thread user-defined
redirections into pipe creation.
Take a page from zsh and just ensure that all pipes we create have fds in
the "high range," which here means at least 10. The primary way to do this
is via the F_DUPFD_CLOEXEC syscall, which also sets CLOEXEC, so we aren't
invoking additional syscalls in the common case. This will free us from
having to track which fds are in user-defined redirections.
Previously we sometimes wanted to access an io_buffer_t to append to it
directly, but that's no longer true; all we really care about is its
separated_buffer_t. Make io_bufferfill_t::finish return the
separated_buffer directly, simplifying call sites. No user visible changes
expected here.
This concerns builtins writing to an io_buffer_t. io_buffer_t is how fish
captures output, especially in command substitutions:
set STUFF (string upper stuff)
Recall that io_buffer_t fills itself by reading from an fd (typically
connected to stdout of the command). However if our command is a builtin,
then we can write to the buffer directly.
Prior to this change, when a builtin anticipated writing to an
io_buffer_t, it would first write into an internal buffer, and then after
the builtin was finished, we would copy it to the io_buffer_t. This was
because we didn't have a polymorphic receiver for builtin output: we
always buffered it and then directed it to the io_buffer_t or file
descriptor or stdout or whatever.
Now that we have polymorphpic io_streams_t, we can notice ahead of time
that the builtin output is destined for an internal buffer and have it
just write directly to that buffer. This saves a buffering step, which is
a nice simplification.
This removes the 100 msec timeout from io_buffer_t. We no longer need to
periodically wake up to check if a command substitution is finished,
because we get explicitly poked when that happens.
io_buffer_t is used to buffer output from a command substitution, so we
can split it into arguments. Typically io_buffer_t reads from its pipe
until it gets EOF and then stops reading. However it may be that the
cmdsub ends but EOF is not delivered because the stdout of the cmdsub
escaped with a background process.
Prior to this change we would wake up every 100 msec (select timeout) to
check if the cmdsub is finished. However this 100 msec adds latency if a
background process is launched from e.g. fish_prompt.
Switch to the new poke() function. Now when the cmdsub is finished, it
pokes its item, which explicitly wakes it up. This removes the extra
latency.
Fixes#7559
In preparation for fixing #7559, add a function poke_item to fd_monitor.
fd_monitor has a list of file descriptors, and invokes a callback when an
fd becomes readable. With this change, we assign each item a unique ID and
return it when the item is added; the ID may then be used to invoke the
callback explicitly.
The idea is that we can stop reading from the pipe associated with the
cmdsub when the job is finished, even if the pipe is still open.
It was possible though unlikely for make_autoclose_pipes to close only
one side of pipe, if it fails to find a new fd. This would result in an
fd leak. Ensure that doesn't happen.
builtins output to stdout and stderr via io_streams_t. Prior to this fix, it
contained an output_stream_t which just wraps a buffer. So all builtin output
went to this buffer (except for eval).
Switch output_stream_t to become a new abstract class which can output to a
buffer, file descriptor, or nowhere. This allows for example `string` to stream
its output as it is produced, instead of buffering it.
Currently fish aborts execution mid-pipeline if a file redirection
failed, which can leave the shell in a broken state (job abandoned after
giving control of the terminal to an already-executed job in the
pipeline).
This patch replaces a failed fd with a closed fd and continues execution
if the affected process wasn't the first in the pipeline.
While this is a hack to address the regression behind fish-shell/#7038
introduced in d62576c, it can also be argued that this behavior is
actually more correct... right?
Closes#7038.
Prior to this fix, builtin_eval would direct output to the io_chain of the
job. The problem is with pipes: `builtin_eval` might happily attempt to
write unlimited output to the write end of a pipe, but the corresponding
reading process has not yet been launched. This results in deadlock.
The fix is to buffer all the output from `builtin_eval`. This is not fun
but the best that can be done until we have real concurrent processes.
Fixes#6806
This switches bufferfills from using an exclusively-owned thread, to
sharing an fd_monitor. This allows multiple bufferfills to all use the same
thread.
Sometimes we must spawn a new thread, to avoid the risk of deadlock.
Ensure we always spawn a thread in those cases. In particular this
includes the fillthread.
