Re: [PATCH v2] Optimization calculation expression of FOR_MODULES(var)

[Nick Vinson]

You cannot remove sizeof(A) from the expression, as doing so may cause
the updated expression to compute a different answer.

Consider ((H*)var)->size = 64, B = 32, and A = 64.

In the original form, you have the expression (64 + 64 - 1) / 64 * 64 /
32 which equals 2.
In your proposal, you have the expression (64 + 64 - 1) / 32 which equals 3.

This could result in incorrect behavior later on.  If I were Daniel, I
would want to see something that shows the change in behavior does not
result in incorrect behavior.

Regards,
Nicholas Vinson

On 4/11/19 1:39 AM, Milo Wenxiang Niu wrote:
> From: Milo Wenxiang X Niu <address@hidden>
> 
> It's just to remove the common factor: "sizeof (grub_addr_t)"  from the 
> numerator and denominator of the fractional expression of next var.
> Let me explain it:
> Shortly:
>       H: struct grub_module_header ;
>       B: grub_uint32_t ;
>       A: grub_addr_t;
> 
> Thus, original expression can be expressed as:
>       var = (H *)((B*)var) + ( offset_exp ))
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>              (((H*)var)->size + sizeof(A) - 1)    sizeof(A)  |
> offset_exp = --------------------------------- * ----------- |
>                       sizeof(A)                   sizeof(B)  |
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> Remove the common factor "sizeof(A)" of fractional offset_exp, we got the 
> result.
> offset_exp_new = ((H*)->size + sizeof(A) - 1) / sizeof(B)
>                          = ((struct grub_module_header *) var)->size + sizeof 
> (grub_addr_t) -1) / sizeof (grub_uint32_t)
> 
> so:
> var =(H *)(((B*)var) + ( (((H*)var)->size + sizeof(A) - 1) / sizeof(B) ))
> That's what I do.
> 
> Still, the new offset express is meaningfull:
>     *numerator: ((struct grub_module_header *) var)->size + sizeof 
> (grub_addr_t) -1)
>               it present the offset value united by byte.
>       *denominator: sizeof (grub_uint32_t)
>               it's means "struct grub_module_header" aligned by 
> sizeof(grub_uint32_t)
> ---
>  include/grub/kernel.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/grub/kernel.h b/include/grub/kernel.h
> index 133a37c..b257224 100644
> --- a/include/grub/kernel.h
> +++ b/include/grub/kernel.h
> @@ -104,7 +104,7 @@ extern grub_addr_t EXPORT_VAR (grub_modbase);
>    var && (grub_addr_t) var \
>      < (grub_modbase + (((struct grub_module_info *) grub_modbase)->size));   
>  \
>    var = (struct grub_module_header *)                                        
> \
> -    (((grub_uint32_t *) var) + ((((struct grub_module_header *) var)->size + 
> sizeof (grub_addr_t) - 1) / sizeof (grub_addr_t)) * (sizeof (grub_addr_t) / 
> sizeof (grub_uint32_t))))
> +    (((grub_uint32_t *) var) + ((((struct grub_module_header *) var)->size + 
> sizeof (grub_addr_t) - 1) / sizeof (grub_uint32_t))))
>  
>  grub_addr_t grub_modules_get_end (void);
>  
>

signature.asc
Description: OpenPGP digital signature