Patchwork [10/11] ppc: avoid write only variables

login
register
mail settings
Submitter Blue Swirl
Date Oct. 6, 2010, 9:34 p.m.
Message ID <AANLkTimOkYH5r8jUdgC5xjnkaJjdMvFXrZMCcvPb3Vip@mail.gmail.com>
Download mbox | patch
Permalink /patch/66982/
State New
Headers show

Comments

Blue Swirl - Oct. 6, 2010, 9:34 p.m.
Compiling with GCC 4.6.0 20100925 produced warnings:
/src/qemu/target-ppc/op_helper.c: In function 'helper_icbi':
/src/qemu/target-ppc/op_helper.c:351:14: error: variable 'tmp' set but
not used [-Werror=unused-but-set-variable]
/src/qemu/target-ppc/op_helper.c: In function 'do_6xx_tlb':
/src/qemu/target-ppc/op_helper.c:3805:28: error: variable 'EPN' set
but not used [-Werror=unused-but-set-variable]
/src/qemu/target-ppc/op_helper.c: In function 'do_74xx_tlb':
/src/qemu/target-ppc/op_helper.c:3838:28: error: variable 'EPN' set
but not used [-Werror=unused-but-set-variable]

Fix by making the variable declarations and their uses also conditional
to debug definition. Delete tmp.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 target-ppc/op_helper.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)
Markus Armbruster - Oct. 7, 2010, 12:33 p.m.
Alexander Graf <agraf@suse.de> writes:

> On 06.10.2010, at 23:34, Blue Swirl wrote:
>
>> Compiling with GCC 4.6.0 20100925 produced warnings:
>> /src/qemu/target-ppc/op_helper.c: In function 'helper_icbi':
>> /src/qemu/target-ppc/op_helper.c:351:14: error: variable 'tmp' set but
>> not used [-Werror=unused-but-set-variable]
>> /src/qemu/target-ppc/op_helper.c: In function 'do_6xx_tlb':
>> /src/qemu/target-ppc/op_helper.c:3805:28: error: variable 'EPN' set
>> but not used [-Werror=unused-but-set-variable]
>> /src/qemu/target-ppc/op_helper.c: In function 'do_74xx_tlb':
>> /src/qemu/target-ppc/op_helper.c:3838:28: error: variable 'EPN' set
>> but not used [-Werror=unused-but-set-variable]
>> 
>> Fix by making the variable declarations and their uses also conditional
>> to debug definition. Delete tmp.
>
> Maybe it would make more sense to get those LOG_* macros into static inline functions. But for the issue at hand, the solution looks good to me.

Or simply (void)EPN.
Blue Swirl - Oct. 7, 2010, 6:59 p.m.
On Wed, Oct 6, 2010 at 9:39 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 06.10.2010, at 23:34, Blue Swirl wrote:
>
>> Compiling with GCC 4.6.0 20100925 produced warnings:
>> /src/qemu/target-ppc/op_helper.c: In function 'helper_icbi':
>> /src/qemu/target-ppc/op_helper.c:351:14: error: variable 'tmp' set but
>> not used [-Werror=unused-but-set-variable]
>> /src/qemu/target-ppc/op_helper.c: In function 'do_6xx_tlb':
>> /src/qemu/target-ppc/op_helper.c:3805:28: error: variable 'EPN' set
>> but not used [-Werror=unused-but-set-variable]
>> /src/qemu/target-ppc/op_helper.c: In function 'do_74xx_tlb':
>> /src/qemu/target-ppc/op_helper.c:3838:28: error: variable 'EPN' set
>> but not used [-Werror=unused-but-set-variable]
>>
>> Fix by making the variable declarations and their uses also conditional
>> to debug definition. Delete tmp.
>
> Maybe it would make more sense to get those LOG_* macros into static inline functions. But for the issue at hand, the solution looks good to me.

Perhaps all conditionally enabled debug printf stuff should be
transformed into tracepoints? That would be more flexible than
uncommenting DEBUG_foobar. Would it help with bitrot too?

> Signed-off-by: Alexander Graf <agraf@suse.de>

Thanks.
Alexander Graf - Oct. 7, 2010, 8:08 p.m.
Am 07.10.2010 um 20:59 schrieb Blue Swirl <blauwirbel@gmail.com>:

> On Wed, Oct 6, 2010 at 9:39 PM, Alexander Graf <agraf@suse.de> wrote:
>> 
>> On 06.10.2010, at 23:34, Blue Swirl wrote:
>> 
>>> Compiling with GCC 4.6.0 20100925 produced warnings:
>>> /src/qemu/target-ppc/op_helper.c: In function 'helper_icbi':
>>> /src/qemu/target-ppc/op_helper.c:351:14: error: variable 'tmp' set but
>>> not used [-Werror=unused-but-set-variable]
>>> /src/qemu/target-ppc/op_helper.c: In function 'do_6xx_tlb':
>>> /src/qemu/target-ppc/op_helper.c:3805:28: error: variable 'EPN' set
>>> but not used [-Werror=unused-but-set-variable]
>>> /src/qemu/target-ppc/op_helper.c: In function 'do_74xx_tlb':
>>> /src/qemu/target-ppc/op_helper.c:3838:28: error: variable 'EPN' set
>>> but not used [-Werror=unused-but-set-variable]
>>> 
>>> Fix by making the variable declarations and their uses also conditional
>>> to debug definition. Delete tmp.
>> 
>> Maybe it would make more sense to get those LOG_* macros into static inline functions. But for the issue at hand, the solution looks good to me.
> 
> Perhaps all conditionally enabled debug printf stuff should be
> transformed into tracepoints? That would be more flexible than
> uncommenting DEBUG_foobar. Would it help with bitrot too?

I haven't looked at the qemu tracepoints framework yet, but some of this debug stuff is in hot paths and only very rarely used. So unless it is overhead free, I'd prefer debug function stubs.

Alex

> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> Thanks.
Blue Swirl - Oct. 8, 2010, 3:44 p.m.
On Thu, Oct 7, 2010 at 8:08 PM, Alexander Graf <agraf@suse.de> wrote:
>
> Am 07.10.2010 um 20:59 schrieb Blue Swirl <blauwirbel@gmail.com>:
>
>> On Wed, Oct 6, 2010 at 9:39 PM, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06.10.2010, at 23:34, Blue Swirl wrote:
>>>
>>>> Compiling with GCC 4.6.0 20100925 produced warnings:
>>>> /src/qemu/target-ppc/op_helper.c: In function 'helper_icbi':
>>>> /src/qemu/target-ppc/op_helper.c:351:14: error: variable 'tmp' set but
>>>> not used [-Werror=unused-but-set-variable]
>>>> /src/qemu/target-ppc/op_helper.c: In function 'do_6xx_tlb':
>>>> /src/qemu/target-ppc/op_helper.c:3805:28: error: variable 'EPN' set
>>>> but not used [-Werror=unused-but-set-variable]
>>>> /src/qemu/target-ppc/op_helper.c: In function 'do_74xx_tlb':
>>>> /src/qemu/target-ppc/op_helper.c:3838:28: error: variable 'EPN' set
>>>> but not used [-Werror=unused-but-set-variable]
>>>>
>>>> Fix by making the variable declarations and their uses also conditional
>>>> to debug definition. Delete tmp.
>>>
>>> Maybe it would make more sense to get those LOG_* macros into static inline functions. But for the issue at hand, the solution looks good to me.
>>
>> Perhaps all conditionally enabled debug printf stuff should be
>> transformed into tracepoints? That would be more flexible than
>> uncommenting DEBUG_foobar. Would it help with bitrot too?
>
> I haven't looked at the qemu tracepoints framework yet, but some of this debug stuff is in hot paths and only very rarely used. So unless it is overhead free, I'd prefer debug function stubs.

There is zero overhead with 'nop' backend, with other backends there
is some overhead. I think only 'nop' backend should be used on
production versions, so this should be comparable to current debug
stuff.

Patch

diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
index 45f1655..348844f 100644
--- a/target-ppc/op_helper.c
+++ b/target-ppc/op_helper.c
@@ -348,15 +348,13 @@  void helper_dcbz_970(target_ulong addr)

 void helper_icbi(target_ulong addr)
 {
-    uint32_t tmp;
-
     addr &= ~(env->dcache_line_size - 1);
     /* Invalidate one cache line :
      * PowerPC specification says this is to be treated like a load
      * (not a fetch) by the MMU. To be sure it will be so,
      * do the load "by hand".
      */
-    tmp = ldl(addr);
+    ldl(addr);
     tb_invalidate_page_range(addr, addr + env->icache_line_size);
 }

@@ -3802,16 +3800,23 @@  void helper_tlbie (target_ulong addr)
 /* PowerPC 602/603 software TLB load instructions helpers */
 static void do_6xx_tlb (target_ulong new_EPN, int is_code)
 {
-    target_ulong RPN, CMP, EPN;
+    target_ulong RPN, CMP;
+#ifdef DEBUG_SOFTWARE_TLB
+    target_ulong EPN;
+#endif
     int way;

     RPN = env->spr[SPR_RPA];
     if (is_code) {
         CMP = env->spr[SPR_ICMP];
+#ifdef DEBUG_SOFTWARE_TLB
         EPN = env->spr[SPR_IMISS];
+#endif
     } else {
         CMP = env->spr[SPR_DCMP];
+#ifdef DEBUG_SOFTWARE_TLB
         EPN = env->spr[SPR_DMISS];
+#endif
     }
     way = (env->spr[SPR_SRR1] >> 17) & 1;
     LOG_SWTLB("%s: EPN " TARGET_FMT_lx " " TARGET_FMT_lx " PTE0 " TARGET_FMT_lx
@@ -3835,12 +3840,17 @@  void helper_6xx_tlbi (target_ulong EPN)
 /* PowerPC 74xx software TLB load instructions helpers */
 static void do_74xx_tlb (target_ulong new_EPN, int is_code)
 {
-    target_ulong RPN, CMP, EPN;
+    target_ulong RPN, CMP;
+#ifdef DEBUG_SOFTWARE_TLB
+    target_ulong EPN;
+#endif
     int way;

     RPN = env->spr[SPR_PTELO];
     CMP = env->spr[SPR_PTEHI];
+#ifdef DEBUG_SOFTWARE_TLB
     EPN = env->spr[SPR_TLBMISS] & ~0x3;
+#endif
     way = env->spr[SPR_TLBMISS] & 0x3;
     LOG_SWTLB("%s: EPN " TARGET_FMT_lx " " TARGET_FMT_lx " PTE0 " TARGET_FMT_lx
               " PTE1 " TARGET_FMT_lx " way %d\n", __func__, new_EPN, EPN, CMP,