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 <printer...@aliyun.com> > > > > 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); _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
