[Crash-utility] [PATCH] Remove the VOID_PTR facilitator macro

Thu Jan 27 18:26:26 UTC 2011

Hi Dave,

>>> On 1/27/2011 at 08:41 AM, in message
<748696339.157170.1296142883615.JavaMail.root at zmail05.collab.prod.int.phx2.redha
.com>, Dave Anderson <anderson at redhat.com> wrote: 

> 
> ----- Original Message -----
>> I've been wondering about the intended use of this facilitator macro
>> for some time and concluded that it is plainly wrong. It should
>> take a pointer value from a buffer, but what is the use of a pointer
>> that pointed to something on the _target_ machine? It cannot be
>> meaningfully dereferenced on the host.
>> 
>> Okay, pointer arithmetics is done in one place, but IMO it is better
>> to explicitly state the object size there, because that's how things
>> are done elsewhere in the code, so it is less surprising for anybody
>> reading the code.
>> 
>> The irony is underlined by the fact that all three uses of the macro
>> need a subsequent typecast to ulong, which proves the whole concept
>> flawed.
> 
> Probably so.  It's been so long since I wrote that I don't recall
> my original intent, other than maybe paranoia that some architecture
> in the future would have pointers and longs of different sizes.
> 
> I'll leave the macro in defs.h though, since it's possible that 
> somebody's extension module uses it.
> 
> BTW, was there a reason that you changed this?:
> 
>                  if (si->c_flags & SLAB_CFLGS_BUFCTL)
> -                        obj = si->s_mem + ((next - si->s_index) * si->c_offset);
> +                        obj = si->s_mem + (next - si->s_index) * si->c_offset;

It wasn't my patch so I'm only offering my ACK for the original change and my reason is that multiply has higher operator precedence than add (and subtract) in normal arithmetic so the outer parentheses were never necessary anyway. They didn't create an error but they weren't needed either. The inner parentheses are required.

-- 
David Mair.

> 
> Dave
> 
>> ---
>> defs.h | 1 -
>> memory.c | 42 +++++++++++++++++++++---------------------
>> 2 files changed, 21 insertions(+), 22 deletions(-)
>> 
>> --- a/defs.h
>> +++ b/defs.h
>> @@ -1760,7 +1760,6 @@ struct builtin_debug_table {
>> #define ULONG_PTR(ADDR) *((ulong **)((char *)(ADDR)))
>> #define USHORT(ADDR) *((ushort *)((char *)(ADDR)))
>> #define SHORT(ADDR) *((short *)((char *)(ADDR)))
>> -#define VOID_PTR(ADDR) *((void **)((char *)(ADDR)))
>> 
>> struct node_table {
>> int node_id;
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -26,8 +26,8 @@ struct meminfo { /* general pu
>> ulong c_offset;
>> ulong c_num;
>> ulong s_mem;
>> - void *s_freep;
>> - ulong *s_index;
>> + ulong s_freep;
>> + ulong s_index;
>> ulong s_inuse;
>> ulong cpucached_cache;
>> ulong cpucached_slab;
>> @@ -139,7 +139,7 @@ static int next_kpage(ulong, ulong *);
>> static ulong last_vmalloc_address(void);
>> static ulong next_vmlist_vaddr(ulong);
>> static int next_identity_mapping(ulong, ulong *);
>> -static int vm_area_page_dump(ulong, ulong, ulong, ulong, void *,
>> +static int vm_area_page_dump(ulong, ulong, ulong, ulong, ulong,
>> struct reference *);
>> static int dump_swap_info(ulong, ulong *, ulong *);
>> static void swap_info_init(void);
>> @@ -3138,7 +3138,7 @@ vm_area_dump(ulong task, ulong flag, ulo
>> ulong vma;
>> ulong vm_start;
>> ulong vm_end;
>> - void *vm_next, *vm_mm;
>> + ulong vm_next, vm_mm;
>> char *dentry_buf, *vma_buf, *file_buf;
>> ulong vm_flags;
>> ulong vm_file, inode;
>> @@ -3202,7 +3202,7 @@ vm_area_dump(ulong task, ulong flag, ulo
>> !DO_REF_SEARCH(ref))
>> fprintf(fp, vma_header);
>> 
>> - for (found = FALSE; vma; vma = (ulong)vm_next) {
>> + for (found = FALSE; vma; vma = vm_next) {
>> 
>> if ((flag & PHYSADDR) && !DO_REF_SEARCH(ref))
>> fprintf(fp, "%s", vma_header);
>> @@ -3211,9 +3211,9 @@ vm_area_dump(ulong task, ulong flag, ulo
>> BZERO(buf1, BUFSIZE);
>> vma_buf = fill_vma_cache(vma);
>> 
>> - vm_mm = VOID_PTR(vma_buf + OFFSET(vm_area_struct_vm_mm));
>> + vm_mm = ULONG(vma_buf + OFFSET(vm_area_struct_vm_mm));
>> vm_end = ULONG(vma_buf + OFFSET(vm_area_struct_vm_end));
>> - vm_next = VOID_PTR(vma_buf + OFFSET(vm_area_struct_vm_next));
>> + vm_next = ULONG(vma_buf + OFFSET(vm_area_struct_vm_next));
>> vm_start = ULONG(vma_buf + OFFSET(vm_area_struct_vm_start));
>> vm_flags = SIZE(vm_area_struct_vm_flags) == sizeof(short) ?
>> USHORT(vma_buf+ OFFSET(vm_area_struct_vm_flags)) :
>> @@ -3335,7 +3335,7 @@ vm_area_page_dump(ulong vma,
>> ulong task,
>> ulong start,
>> ulong end,
>> - void *mm,
>> + ulong mm,
>> struct reference *ref)
>> {
>> physaddr_t paddr;
>> @@ -3347,7 +3347,7 @@ vm_area_page_dump(ulong vma,
>> char buf3[BUFSIZE];
>> char buf4[BUFSIZE];
>> 
>> - if ((ulong)mm == symbol_value("init_mm"))
>> + if (mm == symbol_value("init_mm"))
>> return FALSE;
>> 
>> if (!ref || DO_REF_DISPLAY(ref))
>> @@ -9720,9 +9720,9 @@ dump_slab(struct meminfo *si)
>> return;
>> }
>> 
>> - si->s_freep = VOID_PTR(si->slab_buf + OFFSET(kmem_slab_s_s_freep));
>> + si->s_freep = ULONG(si->slab_buf + OFFSET(kmem_slab_s_s_freep));
>> si->s_inuse = ULONG(si->slab_buf + OFFSET(kmem_slab_s_s_inuse));
>> - si->s_index = ULONG_PTR(si->slab_buf + OFFSET(kmem_slab_s_s_index));
>> + si->s_index = ULONG(si->slab_buf + OFFSET(kmem_slab_s_s_index));
>> s_offset = USHORT(si->slab_buf + OFFSET(kmem_slab_s_s_offset));
>> 
>> if (!(si->flags & ADDRESS_SPECIFIED)) {
>> @@ -9850,7 +9850,7 @@ dump_slab_percpu_v2(struct meminfo *si)
>> static void
>> gather_slab_free_list(struct meminfo *si)
>> {
>> - ulong *next, obj;
>> + ulong next, obj;
>> ulong expected, cnt;
>> 
>> BNEG(si->addrlist, sizeof(ulong) * (si->c_num+1));
>> @@ -9885,16 +9885,16 @@ gather_slab_free_list(struct meminfo *si
>> */
>> 
>> if (si->c_flags & SLAB_CFLGS_BUFCTL)
>> - obj = si->s_mem + ((next - si->s_index) * si->c_offset);
>> + obj = si->s_mem + (next - si->s_index) * si->c_offset;
>> else
>> - obj = (ulong)next - si->c_offset;
>> + obj = next - si->c_offset;
>> 
>> si->addrlist[cnt] = obj;
>> 
>> if (si->flags & ADDRESS_SPECIFIED) {
>> if (INSLAB(next, si) &&
>> - (si->spec_addr >= (ulong)next) &&
>> - (si->spec_addr < (ulong)(next + 1))) {
>> + (si->spec_addr >= next) &&
>> + (si->spec_addr < next + sizeof(ulong))) {
>> si->found = KMEM_BUFCTL_ADDR;
>> return;
>> }
>> @@ -9909,7 +9909,7 @@ gather_slab_free_list(struct meminfo *si
>> si->errors++;
>> }
>> 
>> - readmem((ulong)next, KVADDR, &next, sizeof(void *),
>> + readmem(next, KVADDR, &next, sizeof(ulong),
>> "s_freep chain entry", FAULT_ON_ERROR);
>> } while (next);
>> 
>> @@ -10057,7 +10057,7 @@ static void
>> dump_slab_objects(struct meminfo *si)
>> {
>> int i, j;
>> - ulong *next;
>> + ulong next;
>> int on_free_list;
>> ulong cnt, expected;
>> ulong bufctl, obj;
>> @@ -11561,7 +11561,7 @@ next_upage(struct task_context *tc, ulon
>> int found;
>> char *vma_buf;
>> ulong vm_start, vm_end;
>> - void *vm_next;
>> + ulong vm_next;
>> 
>> if (!tc->mm_struct)
>> return FALSE;
>> @@ -11575,12 +11575,12 @@ next_upage(struct task_context *tc, ulon
>> 
>> vaddr = VIRTPAGEBASE(vaddr) + PAGESIZE(); /* first possible page */
>> 
>> - for (found = FALSE; vma; vma = (ulong)vm_next) {
>> + for (found = FALSE; vma; vma = vm_next) {
>> vma_buf = fill_vma_cache(vma);
>> 
>> vm_start = ULONG(vma_buf + OFFSET(vm_area_struct_vm_start));
>> vm_end = ULONG(vma_buf + OFFSET(vm_area_struct_vm_end));
>> - vm_next = VOID_PTR(vma_buf + OFFSET(vm_area_struct_vm_next));
>> + vm_next = ULONG(vma_buf + OFFSET(vm_area_struct_vm_next));
>> 
>> if (vaddr <= vm_start) {
>> *nextvaddr = vm_start;
>> 
>> --
>> Crash-utility mailing list
>> Crash-utility at redhat.com
>> https://www.redhat.com/mailman/listinfo/crash-utility
> 
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility