[PATCH 0/2] Remove unreachable code and move label after unreachable condition

[PATCH 0/2] Remove unreachable code and move label after unreachable condition

[Samuel Tardieu]

Unreachable code in an error handling block is listed in issue
https://gitlab.com/qemu-project/qemu/-/issues/2030.

After removing this code, the `fail` label is now immediately followed
by a test whose condition can never be true when coming explicitly
via this label. Moving the label down preserves the fall-through
case while avoiding testing an always false condition.

Samuel Tardieu (2):
  tcg: Remove unreachable code
  tcg: Jump after always false condition

 tcg/region.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

-- 
2.42.0

[PATCH 1/2] tcg: Remove unreachable code

[Samuel Tardieu]

The `fail_rx`/`fail` block is only entered while `buf_rx` is equal to
its initial value `MAP_FAILED`. The `munmap(buf_rx, size);` was never
executed.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2030
Signed-off-by: Samuel Tardieu <sam@rfc1149.net>
---
 tcg/region.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tcg/region.c b/tcg/region.c
index 86692455c0..6d657e8c33 100644
--- a/tcg/region.c
+++ b/tcg/region.c
@@ -597,9 +597,6 @@ 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_rx != MAP_FAILED) {
-        munmap(buf_rx, size);
-    }
     if (buf_rw) {
         munmap(buf_rw, size);
     }
-- 
2.42.0

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

[Samuel Tardieu]

`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);
     }
-- 
2.42.0

Re: [PATCH 1/2] tcg: Remove unreachable code

[Peter Maydell]

On Thu, 14 Dec 2023 at 23:32, Samuel Tardieu <sam@rfc1149.net> wrote:
>
> The `fail_rx`/`fail` block is only entered while `buf_rx` is equal to
> its initial value `MAP_FAILED`. The `munmap(buf_rx, size);` was never
> executed.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2030
> Signed-off-by: Samuel Tardieu <sam@rfc1149.net>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Peter Maydell]

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

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

[Samuel Tardieu]

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
-- 
Samuel Tardieu