| Message ID | 20231214233055.2505387-3-sam@rfc1149.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove unreachable code and move label after unreachable condition | expand |
On Thu, 14 Dec 2023 at 23:32, Samuel Tardieu <sam@rfc1149.net> wrote: > > `buf_rw` is always `NULL` when jumping to the `fail` label. Move the > label `down` after the `if (buf_rw) { ... }` statement. > > Signed-off-by: Samuel Tardieu <sam@rfc1149.net> > --- > tcg/region.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tcg/region.c b/tcg/region.c > index 6d657e8c33..691a726eae 100644 > --- a/tcg/region.c > +++ b/tcg/region.c > @@ -596,10 +596,10 @@ static int alloc_code_gen_buffer_splitwx_memfd(size_t size, Error **errp) > > fail_rx: > error_setg_errno(errp, errno, "failed to map shared memory for execute"); > - fail: > if (buf_rw) { > munmap(buf_rw, size); > } > + fail: > if (fd >= 0) { > close(fd); > } It's also the case that fd is always -1 when we jump to the 'fail' label, so if we're moving it down then we should move it past that as well. At this point you might as well make the check after qemu_memfd_alloc() just be if (buf_rw == NULL) { return -1; } and drop the 'fail:' label entirely. And then we know that in this code path buf_rw must be non-NULL and fd must be >= 0, so the fail_rx: codepath doesn't need to explicitly test those. So, well, all of this is definitely removing dead code, but on the other hand it's also moving away from the coding-style pattern the function has at the moment, which is "there is a fail-and-exit codepath which is robust against wherever you might choose to jump to it, and so if we need to add new code to this function then it also can jump to 'fail' without any further updates to that error-exit path". Instead we end up with an "every error-exit check does its own tidyup" idiom. For the sake of not having a static checker say "this is technically dead code", is that worth doing, or does it make the code a little less readable and less amenable to future modification? I'm not sure... -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > So, well, all of this is definitely removing dead > code, but on the other hand it's also moving away > from the coding-style pattern the function has at > the moment, which is "there is a fail-and-exit > codepath which is robust against wherever you might > choose to jump to it, and so if we need to add new > code to this function then it also can jump to 'fail' > without any further updates to that error-exit path". > Instead we end up with an "every error-exit check > does its own tidyup" idiom. For the sake of not having > a static checker say "this is technically dead code", > is that worth doing, or does it make the code a little > less readable and less amenable to future modification? > I'm not sure... Hi Peter. I see your point and I agree with you. Perhaps we could get the best of both worlds by: - renaming `fail_rx` as `fail`, so that we get a unique exit block — not only will the compiler optimize the jump if it can, and this is the slow path anyway - adding a one-line comment saying that `buf_rx` is always `MAP_FAILED` – that will let people know that they might need to add a cleanup if they add another jump to `fail` - calling `error_setg_errno()` at the right place before jumping to `fail` I will produce a v2 to make this proposal clearer. Sam
diff --git a/tcg/region.c b/tcg/region.c index 6d657e8c33..691a726eae 100644 --- a/tcg/region.c +++ b/tcg/region.c @@ -596,10 +596,10 @@ static int alloc_code_gen_buffer_splitwx_memfd(size_t size, Error **errp) fail_rx: error_setg_errno(errp, errno, "failed to map shared memory for execute"); - fail: if (buf_rw) { munmap(buf_rw, size); } + fail: if (fd >= 0) { close(fd); }
`buf_rw` is always `NULL` when jumping to the `fail` label. Move the label `down` after the `if (buf_rw) { ... }` statement. Signed-off-by: Samuel Tardieu <sam@rfc1149.net> --- tcg/region.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)