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

[Daniel Kiper]

On Thu, Apr 11, 2019 at 07:25:35AM -0700, Nick Vinson wrote:
> 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.

I dived deeper into the issue. It looks that Nick is right. AIUI you
cannot cancel sizeof(A) due to specifics of C integer division. So,
I get back my RB and I am not going to apply this patch.

Daniel

> 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);