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

[PATCH v2 0/2] tcg: Jump after always false 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.

Changes from v1:
- Add a comment explaining that `buf_rx` does not require cleanup
- Use a unique cleanup path for the function by setting `errno` before
  jumping to the cleanup block.

Samuel Tardieu (2):
  tcg: Remove unreachable code
  tcg: Make the cleanup-on-error path unique

 tcg/region.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

-- 
2.42.0

[PATCH v2 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 tcg/region.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tcg/region.c b/tcg/region.c
index 86692455c0..467e51cf6f 100644
--- a/tcg/region.c
+++ b/tcg/region.c
@@ -597,9 +597,7 @@ 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);
-    }
+    /* buf_rx is always equal to MAP_FAILED here and does not require cleanup */
     if (buf_rw) {
         munmap(buf_rw, size);
     }
-- 
2.42.0

[PATCH v2 2/2] tcg: Make the cleanup-on-error path unique

[Samuel Tardieu]

By calling `error_setg_errno()` before jumping to the cleanup-on-error
path at the `fail` label, the cleanup path is clearer.

Signed-off-by: Samuel Tardieu <sam@rfc1149.net>
---
 tcg/region.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tcg/region.c b/tcg/region.c
index 467e51cf6f..478ec051c4 100644
--- a/tcg/region.c
+++ b/tcg/region.c
@@ -584,7 +584,9 @@ static int alloc_code_gen_buffer_splitwx_memfd(size_t size, Error **errp)
 
     buf_rx = mmap(NULL, size, host_prot_read_exec(), MAP_SHARED, fd, 0);
     if (buf_rx == MAP_FAILED) {
-        goto fail_rx;
+        error_setg_errno(errp, errno,
+                         "failed to map shared memory for execute");
+        goto fail;
     }
 
     close(fd);
@@ -594,8 +596,6 @@ static int alloc_code_gen_buffer_splitwx_memfd(size_t size, Error **errp)
 
     return PROT_READ | PROT_WRITE;
 
- fail_rx:
-    error_setg_errno(errp, errno, "failed to map shared memory for execute");
  fail:
     /* buf_rx is always equal to MAP_FAILED here and does not require cleanup */
     if (buf_rw) {
-- 
2.42.0

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

[Peter Maydell]

On Tue, 19 Dec 2023 at 18:23, Samuel Tardieu <sam@rfc1149.net> wrote:
>
> 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.
>
> Changes from v1:
> - Add a comment explaining that `buf_rx` does not require cleanup
> - Use a unique cleanup path for the function by setting `errno` before
>   jumping to the cleanup block.
>
> Samuel Tardieu (2):
>   tcg: Remove unreachable code
>   tcg: Make the cleanup-on-error path unique
>
>  tcg/region.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)

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

RTH: I'm assuming you'll take this via your tcg tree.

thanks
-- PMM

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

[Richard Henderson]

On 12/20/23 05:22, Samuel Tardieu wrote:
> 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.
> 
> Changes from v1:
> - Add a comment explaining that `buf_rx` does not require cleanup
> - Use a unique cleanup path for the function by setting `errno` before
>    jumping to the cleanup block.
> 
> Samuel Tardieu (2):
>    tcg: Remove unreachable code
>    tcg: Make the cleanup-on-error path unique
> 
>   tcg/region.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
> 

Queued, thanks.


r~