Eliminating grub_size_t

Eliminating grub_size_t

[Pavel Roskin]

Hello!

I wonder if we would be better off without grub_size_t.  I cannot think
of any code that could use it legitimately.

The ordinary size_t is used to represent the result of sizeof, i.e. size
of a structure.  There is no need for grub to support data structures
exceeding 4 gigabytes.  If we want to support more memory, that's fine,
but that would involve other types that can hold the pointer values,
such as long.

size_t has different size on 32-bit and 64-bit systems, but we should
strive to make the userspace utilities work like the bootloader, so that
possible problems can be detected early and debugged easily.

Besides, we cannot even print size_t in grub_printf(), and I don't think
we should.

grub_size_t should be replaced with int or grub_uint32_t.  size_t can be
used in pure userspace code to call functions that need it.

-- 
Regards,
Pavel Roskin

Re: Eliminating grub_size_t

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 1459 bytes --]

El mar, 01-07-2008 a las 22:14 -0400, Pavel Roskin escribió:
> Hello!
> 
> I wonder if we would be better off without grub_size_t.  I cannot think
> of any code that could use it legitimately.
> 
> The ordinary size_t is used to represent the result of sizeof, i.e. size
> of a structure.  There is no need for grub to support data structures
> exceeding 4 gigabytes.  If we want to support more memory, that's fine,
> but that would involve other types that can hold the pointer values,
> such as long.
I'm not sure if I'm getting you right. Are you suggesting that we "undo"
the someinteger->size_t conversion that caused so many headaches in many
32->64 bit ports? Machine-dependant types like size_t and ptrdiff_t are
here to help us, not to haunt us. What is the exact problem with size_t
in GRUB right now? I agree that grub_size_t is redundant, though.
> 
> size_t has different size on 32-bit and 64-bit systems, but we should
> strive to make the userspace utilities work like the bootloader, so that
> possible problems can be detected early and debugged easily.
I didn't understand this. What do you mean with "US working like the
bootloader?"
> 
> Besides, we cannot even print size_t in grub_printf(), and I don't think
> we should.
1. that can be worked out
2. why not?
> 
> grub_size_t should be replaced with int or grub_uint32_t.  size_t can be
> used in pure userspace code to call functions that need it.
> 

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: Eliminating grub_size_t

[Pavel Roskin]

Quoting Javier Martín <lordhabbit@gmail.com>:

> El mar, 01-07-2008 a las 22:14 -0400, Pavel Roskin escribió:
>> Hello!
>>
>> I wonder if we would be better off without grub_size_t.  I cannot think
>> of any code that could use it legitimately.
>>
>> The ordinary size_t is used to represent the result of sizeof, i.e. size
>> of a structure.  There is no need for grub to support data structures
>> exceeding 4 gigabytes.  If we want to support more memory, that's fine,
>> but that would involve other types that can hold the pointer values,
>> such as long.
> I'm not sure if I'm getting you right. Are you suggesting that we "undo"
> the someinteger->size_t conversion that caused so many headaches in many
> 32->64 bit ports? Machine-dependant types like size_t and ptrdiff_t are
> here to help us, not to haunt us. What is the exact problem with size_t
> in GRUB right now? I agree that grub_size_t is redundant, though.

No real problem.  There are some warnings in fs/reiserfs.c about it.   
But I think removing or redefining grub_size_t would be the cleanest  
solution.

>> size_t has different size on 32-bit and 64-bit systems, but we should
>> strive to make the userspace utilities work like the bootloader, so that
>> possible problems can be detected early and debugged easily.
> I didn't understand this. What do you mean with "US working like the
> bootloader?"

I mean that if, say, GRUB fails to read reiserfs, I'd like to be able  
to reproduce the problem in grub-fstest even if I'm compiling it on  
x86_64.

>> Besides, we cannot even print size_t in grub_printf(), and I don't think
>> we should.
> 1. that can be worked out
> 2. why not?

We want to save space, not to add code just for a good feeling that  
out printf is a s good as the one in glibc.

-- 
Regards,
Pavel Roskin

Re: Eliminating grub_size_t

[Isaac Dupree]

>>> size_t has different size on 32-bit and 64-bit systems, but we should
>>> strive to make the userspace utilities work like the bootloader, so that
>>> possible problems can be detected early and debugged easily.
>> I didn't understand this. What do you mean with "US working like the
>> bootloader?"
> 
> I mean that if, say, GRUB fails to read reiserfs, I'd like to be able to 
> reproduce the problem in grub-fstest even if I'm compiling it on x86_64.

In this case, so we're producing a 32-bit, pc grub image.  To have a 
similar effect in grub-fstest, we'd need to define grub_size_t to be a 
32-bit quantity when compiling that too, am I right?  Is there any 
reason not to just have grub-fstest try to imitate whatever the 
bootloader image decides it needs?  So if some platform requires a 
64-bit bootloader and we're running on 32-bits, we may need a 64-bit 
grub_size_t in both places (well, this is maybe not likely to work 
entirely, but GCC can generate the operations -- or we could just use 32 
bit for grub-fstest then if we think it's the least-nonsensical thing to 
do in that hypothetical situation).

-Isaac

Re: Eliminating grub_size_t

[Pavel Roskin]

Quoting Isaac Dupree <id@isaac.cedarswampstudios.org>:

>> I mean that if, say, GRUB fails to read reiserfs, I'd like to be   
>> able to reproduce the problem in grub-fstest even if I'm compiling   
>> it on x86_64.
>
> In this case, so we're producing a 32-bit, pc grub image.  To have a
> similar effect in grub-fstest, we'd need to define grub_size_t to be a
> 32-bit quantity when compiling that too, am I right?  Is there any
> reason not to just have grub-fstest try to imitate whatever the
> bootloader image decides it needs?  So if some platform requires a
> 64-bit bootloader and we're running on 32-bits, we may need a 64-bit
> grub_size_t in both places (well, this is maybe not likely to work
> entirely, but GCC can generate the operations -- or we could just use
> 32 bit for grub-fstest then if we think it's the least-nonsensical
> thing to do in that hypothetical situation).

I don't think we need 64-bit grub_size_t anywhere, even in 64-bit bootloaders.

-- 
Regards,
Pavel Roskin

Re: Eliminating grub_size_t

[Vesa Jääskeläinen]

Pavel Roskin wrote:
> Quoting Javier Martín <lordhabbit@gmail.com>:
> 
>> El mar, 01-07-2008 a las 22:14 -0400, Pavel Roskin escribió:
>>> Hello!
>>>
>>> I wonder if we would be better off without grub_size_t.  I cannot think
>>> of any code that could use it legitimately.
>>>
>>> The ordinary size_t is used to represent the result of sizeof, i.e. size
>>> of a structure.  There is no need for grub to support data structures
>>> exceeding 4 gigabytes.  If we want to support more memory, that's fine,
>>> but that would involve other types that can hold the pointer values,
>>> such as long.
>> I'm not sure if I'm getting you right. Are you suggesting that we "undo"
>> the someinteger->size_t conversion that caused so many headaches in many
>> 32->64 bit ports? Machine-dependant types like size_t and ptrdiff_t are
>> here to help us, not to haunt us. What is the exact problem with size_t
>> in GRUB right now? I agree that grub_size_t is redundant, though.
> 
> No real problem.  There are some warnings in fs/reiserfs.c about it.  
> But I think removing or redefining grub_size_t would be the cleanest 
> solution.

If reiserfs is using it in wrong place, fix the reiserfs. If you are 
reading some file system variable, then you should use grub_uintN_t to 
specify storage size in bits.

size_t is usually used as common index or offset (or size) to some 
buffer. size_t is returned by sizeof(). It is meant to be optimal size 
for platform. Eg. on 64bit memory bus it is 64bit and on 32bit memory 
bus it is 32bit. What grub is doing here is just defining yet another 
type for the same thing.

Google for size_t if you want to find out more about it.

Re: Eliminating grub_size_t

[Pavel Roskin]

On Wed, 2008-07-02 at 20:46 +0300, Vesa Jääskeläinen wrote:

> If reiserfs is using it in wrong place, fix the reiserfs. If you are 
> reading some file system variable, then you should use grub_uintN_t to 
> specify storage size in bits.

OK, I'll have another look at the code.

> size_t is usually used as common index or offset (or size) to some 
> buffer. size_t is returned by sizeof(). It is meant to be optimal size 
> for platform. Eg. on 64bit memory bus it is 64bit and on 32bit memory 
> bus it is 32bit. What grub is doing here is just defining yet another 
> type for the same thing.
> 
> Google for size_t if you want to find out more about it.

I know what it is.  I believe int should be as good as size_t for most
purposes is we are not working with very large structures or read
gigabytes of data from files at once.

-- 
Regards,
Pavel Roskin

Re: Eliminating grub_size_t

[Marco Gerards]

Pavel Roskin <proski@gnu.org> writes:

> On Wed, 2008-07-02 at 20:46 +0300, Vesa Jääskeläinen wrote:
>
>> If reiserfs is using it in wrong place, fix the reiserfs. If you are 
>> reading some file system variable, then you should use grub_uintN_t to 
>> specify storage size in bits.
>
> OK, I'll have another look at the code.
>
>> size_t is usually used as common index or offset (or size) to some 
>> buffer. size_t is returned by sizeof(). It is meant to be optimal size 
>> for platform. Eg. on 64bit memory bus it is 64bit and on 32bit memory 
>> bus it is 32bit. What grub is doing here is just defining yet another 
>> type for the same thing.
>> 
>> Google for size_t if you want to find out more about it.
>
> I know what it is.  I believe int should be as good as size_t for most
> purposes is we are not working with very large structures or read
> gigabytes of data from files at once.

Perhaps, but it doesn't hurt either.  I think it is a good thing to
have a type such that it is clear what kind of variable is used.

--
Marco

Re: Eliminating grub_size_t

[Pavel Roskin]

On Thu, 2008-07-03 at 20:02 +0200, Marco Gerards wrote:
> > I know what it is.  I believe int should be as good as size_t for most
> > purposes is we are not working with very large structures or read
> > gigabytes of data from files at once.
> 
> Perhaps, but it doesn't hurt either.  I think it is a good thing to
> have a type such that it is clear what kind of variable is used.

I mean, we can have the type, but make it 32-bit on all systems.
Anyway, the warnings have been fixed.  In some cases, grub_size_t was
used for offsets, which is wrong because we want to support large files
on 32-bit systems.

I think I'll try to make grub_size_t 32-bit everywhere and see if it's
going to make any difference or help discover some issues.

-- 
Regards,
Pavel Roskin

Re: Eliminating grub_size_t

[Vesa Jääskeläinen]

Pavel Roskin wrote:
> On Thu, 2008-07-03 at 20:02 +0200, Marco Gerards wrote:
>>> I know what it is.  I believe int should be as good as size_t for most
>>> purposes is we are not working with very large structures or read
>>> gigabytes of data from files at once.
>> Perhaps, but it doesn't hurt either.  I think it is a good thing to
>> have a type such that it is clear what kind of variable is used.

> I think I'll try to make grub_size_t 32-bit everywhere and see if it's
> going to make any difference or help discover some issues.

Why? I would let it be optimal type for current memory bus width.

Re: Eliminating grub_size_t

[Pavel Roskin]

On Thu, 2008-07-03 at 21:42 +0300, Vesa Jääskeläinen wrote:

> > I think I'll try to make grub_size_t 32-bit everywhere and see if it's
> > going to make any difference or help discover some issues.
> 
> Why? I would let it be optimal type for current memory bus width.

int is supposed to be the optimal type, and int is 32-bit on 64-bit
platforms we support.  As far as I know, the x86_64 machine code
actually defaults to 32-bit for all arguments except memory addresses.

-- 
Regards,
Pavel Roskin

Re: Eliminating grub_size_t

[Vesa Jääskeläinen]

Pavel Roskin wrote:
> On Thu, 2008-07-03 at 21:42 +0300, Vesa Jääskeläinen wrote:
> 
>>> I think I'll try to make grub_size_t 32-bit everywhere and see if it's
>>> going to make any difference or help discover some issues.
>> Why? I would let it be optimal type for current memory bus width.
> 
> int is supposed to be the optimal type, and int is 32-bit on 64-bit
> platforms we support.  As far as I know, the x86_64 machine code
> actually defaults to 32-bit for all arguments except memory addresses.

And size_t is kinda connected to memory addresses. Do you agree :) ?

Re: Eliminating grub_size_t

[Pavel Roskin]

On Thu, 2008-07-03 at 22:07 +0300, Vesa Jääskeläinen wrote:

> And size_t is kinda connected to memory addresses. Do you agree :) ?

Yes.  However, size_t should hold the maximal structure size, and we can
limit it to 4 (or even 2) gigabytes.  You can think of it as of the size
of a contiguous chunk of memory.  Difference between pointers to
different chunks doesn't have to fit size_t or ptrdiff_t.

Let's see: size_t is used:

in sizeof - OK to limit
in malloc - OK to limit
in strlen - OK to limit
in memcpy - OK to limit
in file I/O - OK to limit

We should not be limiting file and partition sizes and memory addresses,
but it's OK to limit the size of memory that is used at once, including
reading from files.  That's the whole reason why off_t may be longer
than size_t.

-- 
Regards,
Pavel Roskin

Re: Eliminating grub_size_t

[Marco Gerards]

Pavel Roskin <proski@gnu.org> writes:

> On Thu, 2008-07-03 at 20:02 +0200, Marco Gerards wrote:
>> > I know what it is.  I believe int should be as good as size_t for most
>> > purposes is we are not working with very large structures or read
>> > gigabytes of data from files at once.
>> 
>> Perhaps, but it doesn't hurt either.  I think it is a good thing to
>> have a type such that it is clear what kind of variable is used.
>
> I mean, we can have the type, but make it 32-bit on all systems.
> Anyway, the warnings have been fixed.  In some cases, grub_size_t was
> used for offsets, which is wrong because we want to support large files
> on 32-bit systems.
>
> I think I'll try to make grub_size_t 32-bit everywhere and see if it's
> going to make any difference or help discover some issues.

Please don't.  I'd rather stick to integers, such change will only
slow down GRUB with no gain.

--
Marco

Re: Eliminating grub_size_t

[Pavel Roskin]

On Thu, 2008-07-03 at 20:56 +0200, Marco Gerards wrote:
> Pavel Roskin <proski@gnu.org> writes:

> > I think I'll try to make grub_size_t 32-bit everywhere and see if it's
> > going to make any difference or help discover some issues.
> 
> Please don't.  I'd rather stick to integers, such change will only
> slow down GRUB with no gain.

Integers are 32-bit on all platforms we support.  Moreover, long is
32-bit on all platforms we support except SPARC, which is basically on
life support (I didn't look at the x86_64 EFI changes yet).

Anyway, I'd like to stop this thread until I have specific code.
Otherwise, I have to explain what I mean instead of showing it.

-- 
Regards,
Pavel Roskin