RE: [ros-kernel] Patch: Teach freeldr registry about multiandexpanded types

[Casper H. <ch...@us...>]

tor, 2002-06-13 kl. 03:10 skrev Brian Palmer:
> > ons, 2002-06-12 kl. 05:44 skrev Brian Palmer:
> > 
> > These are some changes I made to remove the warnings. They are not
> part
> > of Joseph's patch.
> > 
> > > I have a few questions about this:
> > >
> > > reactos.c line 193:
> > > rc = RegQueryValue(hGroupKey, "List", NULL, (PUCHAR)&ValueBuffer,
> > > &BufferSize);
> > >
> > > Shouldn't that be:
> > > rc = RegQueryValue(hGroupKey, "List", NULL, (PUCHAR)ValueBuffer,
> > > &BufferSize);
> > >
> > 
> > They are both okay due to the way the compiler handles char arrays.
> The
> > first makes more sence to me because it takes the address of the array
> > and then casts it to a PUCHAR (ie. it casts a pointer to another
> > pointer). The second casts a char[] array to PUCHAR (ie. an array to
> > pointer cast).
> 
> This is incorrect. ValueBuffer is a pointer. This is how you can take a
> regular pointer and use it like it was an array. So you are passing in
> the address of the pointer to the array. Try printing the values of the
> two addresses and you will see that they are different.
> 
> > > And the same thing in meminit.c line 64. Since BiosMemoryMap is a
> > > pointer to the array, you are passing in the address of the pointer
> to
> > > the array and casting it to type PBIOS_MEMORY_MAP.
> > >
> > 
> > No. BiosMemoryMap is on the stack in MmInitializeMemoryManager(),
> > defined as BIOS_MEMORY_MAP BiosMemoryMap[32]. It is not a pointer.
> > Wether it should be on the stack is debatable because it is retrieved
> > from the BIOS twice (from what I gather), once by the memory manager
> and
> > once by the ReactOS boot code.
> 
> Again, BiosMemoryMap is just a pointer, so all you are putting on the
> stack is 4 bytes. The compiler actually interprets a parameter such as
> this to be just a pointer. That's why if you use sizeof() on a function
> parameter that is an array it will only return 4 instead of the size of
> the array. If you run lint on code that does this it will throw an error
> at you.
> 
> > > I also noticed that this patch changes all the function prototypes
> from
> > > ULONG GetBiosMemoryMap(BIOS_MEMORY_MAP BiosMemoryMap[32]); to ULONG
> > > GetBiosMemoryMap(PBIOS_MEMORY_MAP BiosMemoryMap); Now this would
> > > normally be fine except that that function returns an array of
> exactly
> > > 32 items since the size of the memory map is not known before the
> > > function call.
> > 
> > Yes that is a bit unsafe. Maybe we should rename BIOS_MEMORY_MAP to
> > BIOS_MEMORY_MAP_ENTRY and create a new type for the bios memory map?
> > 
> > typedef BIOS_MEMORY_MAP_ENTRY BIOS_MEMORY_MAP[32];
> > typedef BIOS_MEMORY_MAP *PBIOS_MEMORY_MAP;
> > 
> > That way we can avoid some type casts.
> 

I just confirmed that the addresses are equal with this little program.

#include <stdio.h>
#include <windows.h>

typedef struct
{
	ULONG	BaseAddressLow;
	ULONG	BaseAddressHigh;
	ULONG	LengthLow;
	ULONG	LengthHigh;
	ULONG	Type;
	ULONG	Reserved;
} PACKED BIOS_MEMORY_MAP, *PBIOS_MEMORY_MAP;

int
main(int argc, char *argv[])
{
  BIOS_MEMORY_MAP BiosMemoryMap[32];

  printf("BiosMemoryMap @ 0x%lx\n", (PUCHAR)&BiosMemoryMap);
  printf("BiosMemoryMap @ 0x%lx\n", (PUCHAR)BiosMemoryMap);
  printf("sizeof(BiosMemoryMap) = %d\n", sizeof(BiosMemoryMap));

  return 0;
}

Gives for me:

BiosMemoryMap @ 0x22fc60
BiosMemoryMap @ 0x22fc60
sizeof(BiosMemoryMap) = 768

The ability to cast arrays to pointers without using & is a compiler
trick. A dangerous trick IMHO.

> I think I will just create a structure called BIOS_MEMORY_MAP_ARRAY. It
> will have one member of type BIOS_MEMORY_MAP which is an array of 32
> items. And I'll change this function to take a PBIOS_MEMORY_MAP_ARRAY
> pointer as a parameter. What do you think?

A just as good solution IMO.

Casper