| Message ID | 1510087611-1851-1-git-send-email-cota@braap.org |
|---|---|
| State | New |
| Headers | show |
| Series | arm/translate-a64: mark path as unreachable to eliminate warning | expand |
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); >
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
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
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);
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(+)