[2/2] tcg: Jump after always false condition

`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(-)

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