[Qemu-devel] [PATCH] arm/translate-a64: mark path as unreachable to eliminate warning

[Qemu-devel] [PATCH] arm/translate-a64: mark path as unreachable to eliminate warning

[Emilio G. Cota]

Fixes the following warning when compiling with gcc 5.4.0 with -O1
optimizations and --enable-debug:

target/arm/translate-a64.c: In function ‘aarch64_tr_translate_insn’:
target/arm/translate-a64.c:2361:8: error: ‘post_index’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
     if (!post_index) {
        ^
target/arm/translate-a64.c:2307:10: note: ‘post_index’ was declared here
     bool post_index;
          ^
target/arm/translate-a64.c:2386:8: error: ‘writeback’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
     if (writeback) {
        ^
target/arm/translate-a64.c:2308:10: note: ‘writeback’ was declared here
     bool writeback;
          ^

Note that idx comes from selecting 2 bits, and therefore its value
can be at most 3.

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 target/arm/translate-a64.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index caca05a..625ef2d 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -2340,28 +2340,30 @@ static void disas_ldst_reg_imm9(DisasContext *s, uint32_t insn,
     switch (idx) {
     case 0:
     case 2:
         post_index = false;
         writeback = false;
         break;
     case 1:
         post_index = true;
         writeback = true;
         break;
     case 3:
         post_index = false;
         writeback = true;
         break;
+    default:
+        g_assert_not_reached();
     }
 
     if (rn == 31) {
         gen_check_sp_alignment(s);
     }
     tcg_addr = read_cpu_reg_sp(s, rn, 1);
 
     if (!post_index) {
         tcg_gen_addi_i64(tcg_addr, tcg_addr, imm9);
     }
 
     if (is_vector) {
         if (is_store) {
             do_fp_st(s, rt, tcg_addr, size);
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH] arm/translate-a64: mark path as unreachable to eliminate warning

[Philippe Mathieu-Daudé]

On 11/07/2017 05:46 PM, Emilio G. Cota wrote:
> Fixes the following warning when compiling with gcc 5.4.0 with -O1
> optimizations and --enable-debug:
> 
> target/arm/translate-a64.c: In function ‘aarch64_tr_translate_insn’:
> target/arm/translate-a64.c:2361:8: error: ‘post_index’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>      if (!post_index) {
>         ^
> target/arm/translate-a64.c:2307:10: note: ‘post_index’ was declared here
>      bool post_index;
>           ^
> target/arm/translate-a64.c:2386:8: error: ‘writeback’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>      if (writeback) {
>         ^
> target/arm/translate-a64.c:2308:10: note: ‘writeback’ was declared here
>      bool writeback;
>           ^
> 
> Note that idx comes from selecting 2 bits, and therefore its value
> can be at most 3.
> 
> Signed-off-by: Emilio G. Cota <cota@braap.org>

Acked-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/arm/translate-a64.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index caca05a..625ef2d 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -2340,28 +2340,30 @@ static void disas_ldst_reg_imm9(DisasContext *s, uint32_t insn,
>      switch (idx) {
>      case 0:
>      case 2:
>          post_index = false;
>          writeback = false;
>          break;
>      case 1:
>          post_index = true;
>          writeback = true;
>          break;
>      case 3:
>          post_index = false;
>          writeback = true;
>          break;
> +    default:
> +        g_assert_not_reached();
>      }
>  
>      if (rn == 31) {
>          gen_check_sp_alignment(s);
>      }
>      tcg_addr = read_cpu_reg_sp(s, rn, 1);
>  
>      if (!post_index) {
>          tcg_gen_addi_i64(tcg_addr, tcg_addr, imm9);
>      }
>  
>      if (is_vector) {
>          if (is_store) {
>              do_fp_st(s, rt, tcg_addr, size);
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH] arm/translate-a64: mark path as unreachable to eliminate warning

[Peter Maydell]

On 8 November 2017 at 12:37, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 11/07/2017 05:46 PM, Emilio G. Cota wrote:
>> Fixes the following warning when compiling with gcc 5.4.0 with -O1
>> optimizations and --enable-debug:
>>
>> target/arm/translate-a64.c: In function ‘aarch64_tr_translate_insn’:
>> target/arm/translate-a64.c:2361:8: error: ‘post_index’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>      if (!post_index) {
>>         ^
>> target/arm/translate-a64.c:2307:10: note: ‘post_index’ was declared here
>>      bool post_index;
>>           ^
>> target/arm/translate-a64.c:2386:8: error: ‘writeback’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>      if (writeback) {
>>         ^
>> target/arm/translate-a64.c:2308:10: note: ‘writeback’ was declared here
>>      bool writeback;
>>           ^
>>
>> Note that idx comes from selecting 2 bits, and therefore its value
>> can be at most 3.
>>
>> Signed-off-by: Emilio G. Cota <cota@braap.org>

Applied to target-arm.next, thanks. (It's a bit sad that
gcc can't figure out that the result of x & 3 is constrained
to [0,3], but there you go.)

> Acked-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

By the way, Philippe, what are you intending to convey with
an acked-by tag? Usually "acked-by" means "I'm the maintainer
for this area and I haven't actually reviewed this but I don't
object to it"...

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH] arm/translate-a64: mark path as unreachable to eliminate warning

[Philippe Mathieu-Daudé]

Hi Peter,

On 11/13/2017 08:26 AM, Peter Maydell wrote:
> On 8 November 2017 at 12:37, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 11/07/2017 05:46 PM, Emilio G. Cota wrote:
>>> Fixes the following warning when compiling with gcc 5.4.0 with -O1
>>> optimizations and --enable-debug:
>>>
>>> target/arm/translate-a64.c: In function ‘aarch64_tr_translate_insn’:
>>> target/arm/translate-a64.c:2361:8: error: ‘post_index’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>>      if (!post_index) {
>>>         ^
>>> target/arm/translate-a64.c:2307:10: note: ‘post_index’ was declared here
>>>      bool post_index;
>>>           ^
>>> target/arm/translate-a64.c:2386:8: error: ‘writeback’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>>      if (writeback) {
>>>         ^
>>> target/arm/translate-a64.c:2308:10: note: ‘writeback’ was declared here
>>>      bool writeback;
>>>           ^
>>>
>>> Note that idx comes from selecting 2 bits, and therefore its value
>>> can be at most 3.
>>>
>>> Signed-off-by: Emilio G. Cota <cota@braap.org>
> 
> Applied to target-arm.next, thanks. (It's a bit sad that
> gcc can't figure out that the result of x & 3 is constrained
> to [0,3], but there you go.)
> 
>> Acked-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> By the way, Philippe, what are you intending to convey with
> an acked-by tag? Usually "acked-by" means "I'm the maintainer
> for this area and I haven't actually reviewed this but I don't
> object to it"...

I still feel new into the FOSS community and am happy to learn and
understand from such comments :)
Reading thru the list I wondered what means this tag, asked to some
maintainers on IRC and read the Linux kernel "Submitting patches guide"
[1] bullet 12) "When to use Acked-by":

'''
Acked-by: is often used by the maintainer of the affected code when that
maintainer neither contributed to nor forwarded the patch.

Acked-by: is not as formal as Signed-off-by:. It is a record that the
acker has at least reviewed the patch and has indicated acceptance.
Hence patch mergers will sometimes manually convert an acker’s “yep,
looks good to me” into an Acked-by: (but note that it is usually better
to ask for an explicit ack).

Acked-by: does not necessarily indicate acknowledgement of the entire
patch. For example, if a patch affects multiple subsystems and has an
Acked-by: from one subsystem maintainer then this usually indicates
acknowledgement of just the part which affects that maintainer’s code. [...]
'''

It is not obvious that the 2nd case is restricted to maintainer, but now
than you explain it I understand.

I intended to express "I don't think this is the best way to solve this
problem, however this is probably the best fix when freezed for a
Release, and since I did have reviewed this in details (and tested it)
but I don't object to it, instead of signing with my Reviewed-by tag I
lower it to an Acked-by".

Next time I'll keep it simple and use a R-b :)

Thanks to take time to correct me!

Regards,

Phil.

[1]
https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html#when-to-use-acked-by-and-cc