Ananth N Mavinakayanahalli wrote:
[snip]
>>> I get this warning:
>>>
>>> kernel/kprobes.c: In function ‘collect_garbage_slots’:
>>> kernel/kprobes.c:215: warning: comparison is always false due to limited
>>> range of data type
>>>
>>> when building 2.6.20-rc5 on powerpc. Not strange though, since the
>>> powerpc compiler is more unforgiving than most other arch compilers. The
>>> line in question is:
>>>
>>> if (kip->slot_used[i] == -1 &&
>>>
>>> and kip->slot_used is a char array and -1 is apparently an invalid char
>>> assignment. Is there a way to get rid of the warning?
>> As far as I searched, on powerpc and arm, the default type of char is
>> 'unsigned char' whereas that is 'signed' on most other arch. So, your
>> compiler warned "due to limited range of data type".
>>
>> I think a good solution is that we just replace -1 with 2.
>> Could you test the patch attached to this mail?
>
> Thanks for taking care of this, the patch fixes the warning.
>
> I think however, instead of having magic numbers 0, 1, 2, why not an
> enum? It makes the code easier to read. Comments?
It's a good idea. Same thing was pointed out by my boss too.
Here is a patch which uses an enum to express the slot status.
Thanks,
Signed-off-by: Masami Hiramatsu <[email protected]>
---
This patch replaces the magic numbers with an enum, and
gets rid of a warning on the specific architectures (ex. powerpc)
on which the compiler considers 'char' as 'unsigned char'.
kernel/kprobes.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
Index: linux-2.6.20-rc5/kernel/kprobes.c
===================================================================
--- linux-2.6.20-rc5.orig/kernel/kprobes.c
+++ linux-2.6.20-rc5/kernel/kprobes.c
@@ -87,6 +87,12 @@ struct kprobe_insn_page {
int ngarbage;
};
+enum kprobe_slot_state {
+ SLOT_CLEAN = 0,
+ SLOT_DIRTY = 1,
+ SLOT_USED = 2,
+};
+
static struct hlist_head kprobe_insn_pages;
static int kprobe_garbage_slots;
static int collect_garbage_slots(void);
@@ -130,8 +136,8 @@ kprobe_opcode_t __kprobes *get_insn_slot
if (kip->nused < INSNS_PER_PAGE) {
int i;
for (i = 0; i < INSNS_PER_PAGE; i++) {
- if (!kip->slot_used[i]) {
- kip->slot_used[i] = 1;
+ if (kip->slot_used[i] == SLOT_CLEAN) {
+ kip->slot_used[i] = SLOT_USED;
kip->nused++;
return kip->insns + (i * MAX_INSN_SIZE);
}
@@ -163,8 +169,8 @@ kprobe_opcode_t __kprobes *get_insn_slot
}
INIT_HLIST_NODE(&kip->hlist);
hlist_add_head(&kip->hlist, &kprobe_insn_pages);
- memset(kip->slot_used, 0, INSNS_PER_PAGE);
- kip->slot_used[0] = 1;
+ memset(kip->slot_used, SLOT_CLEAN, INSNS_PER_PAGE);
+ kip->slot_used[0] = SLOT_USED;
kip->nused = 1;
kip->ngarbage = 0;
return kip->insns;
@@ -173,7 +179,7 @@ kprobe_opcode_t __kprobes *get_insn_slot
/* Return 1 if all garbages are collected, otherwise 0. */
static int __kprobes collect_one_slot(struct kprobe_insn_page *kip, int idx)
{
- kip->slot_used[idx] = 0;
+ kip->slot_used[idx] = SLOT_CLEAN;
kip->nused--;
if (kip->nused == 0) {
/*
@@ -212,7 +218,7 @@ static int __kprobes collect_garbage_slo
continue;
kip->ngarbage = 0; /* we will collect all garbages */
for (i = 0; i < INSNS_PER_PAGE; i++) {
- if (kip->slot_used[i] == -1 &&
+ if (kip->slot_used[i] == SLOT_DIRTY &&
collect_one_slot(kip, i))
break;
}
@@ -232,7 +238,7 @@ void __kprobes free_insn_slot(kprobe_opc
slot < kip->insns + (INSNS_PER_PAGE * MAX_INSN_SIZE)) {
int i = (slot - kip->insns) / MAX_INSN_SIZE;
if (dirty) {
- kip->slot_used[i] = -1;
+ kip->slot_used[i] = SLOT_DIRTY;
kip->ngarbage++;
} else {
collect_one_slot(kip, i);