| Message ID | 20210328224913.799167-1-raj.khem@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V: Restore the typcast to 64bit type | expand |
On Mär 28 2021, Khem Raj wrote: > this makes the type promotions clear and explicit > It was already typecasted to long but was accidentally dropped in [1] Note that long is 32-bit on riscv32, so the title is misleading (there was no cast to 64-bit type before). The cast really was a no-op, as it didn't change the resulting type. The issue is that the types of sym_addr, target_section_addr and offset are Elf_Addr (aka Elf32_Addr), which is an unsigned type (image_target->vaddr_offset is signed, but not wider than Elf32_Addr, so it doesn't change the overall type). Thus when the result of the expression is extended to grub_int64_t it is always a positive value instead of the desired sign-extended one. > which stated to cause failures on riscv32 as reported in [2] Most likely the missing addend masked the error. Andreas.
On Mon, Mar 29, 2021 at 3:24 AM Andreas Schwab <schwab@suse.de> wrote: > > On Mär 28 2021, Khem Raj wrote: > > > this makes the type promotions clear and explicit > > It was already typecasted to long but was accidentally dropped in [1] > > Note that long is 32-bit on riscv32, so the title is misleading (there > was no cast to 64-bit type before). The cast really was a no-op, as it > didn't change the resulting type. The issue is that the types of > sym_addr, target_section_addr and offset are Elf_Addr (aka Elf32_Addr), > which is an unsigned type (image_target->vaddr_offset is signed, but not > wider than Elf32_Addr, so it doesn't change the overall type). Thus > when the result of the expression is extended to grub_int64_t it is > always a positive value instead of the desired sign-extended one. > This is right I think, I initially typecasted it to long and then changed to using grub_int64_t and could have done better on commit message. Do you prefer a new revision with better commit msg. > > which stated to cause failures on riscv32 as reported in [2] > > Most likely the missing addend masked the error. yes that's likely since addressed where it fails are 0xfffff.... > > Andreas. > > -- > Andreas Schwab, SUSE Labs, schwab@suse.de > GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 > "And now for something completely different."
On Mon, Mar 29, 2021 at 11:33:11AM -0700, Khem Raj wrote: > On Mon, Mar 29, 2021 at 3:24 AM Andreas Schwab <schwab@suse.de> wrote: > > On Mär 28 2021, Khem Raj wrote: > > > this makes the type promotions clear and explicit > > > It was already typecasted to long but was accidentally dropped in [1] > > > > Note that long is 32-bit on riscv32, so the title is misleading (there > > was no cast to 64-bit type before). The cast really was a no-op, as it > > didn't change the resulting type. The issue is that the types of > > sym_addr, target_section_addr and offset are Elf_Addr (aka Elf32_Addr), > > which is an unsigned type (image_target->vaddr_offset is signed, but not > > wider than Elf32_Addr, so it doesn't change the overall type). Thus > > when the result of the expression is extended to grub_int64_t it is > > always a positive value instead of the desired sign-extended one. > > > > This is right I think, I initially typecasted it to long and then > changed to using grub_int64_t > and could have done better on commit message. Do you prefer a new revision with > better commit msg. If long is 32-bit on riscv32 should not we cast to grub_int32_t then? And then next question: Why off is defined as grub_int64_t? Daniel
On Tue, Mar 30, 2021 at 12:29:56AM +0200, Daniel Kiper wrote: > On Mon, Mar 29, 2021 at 11:33:11AM -0700, Khem Raj wrote: > > On Mon, Mar 29, 2021 at 3:24 AM Andreas Schwab <schwab@suse.de> wrote: > > > On Mär 28 2021, Khem Raj wrote: > > > > this makes the type promotions clear and explicit > > > > It was already typecasted to long but was accidentally dropped in [1] > > > > > > Note that long is 32-bit on riscv32, so the title is misleading (there > > > was no cast to 64-bit type before). The cast really was a no-op, as it > > > didn't change the resulting type. The issue is that the types of > > > sym_addr, target_section_addr and offset are Elf_Addr (aka Elf32_Addr), > > > which is an unsigned type (image_target->vaddr_offset is signed, but not > > > wider than Elf32_Addr, so it doesn't change the overall type). Thus > > > when the result of the expression is extended to grub_int64_t it is > > > always a positive value instead of the desired sign-extended one. > > > > > > > This is right I think, I initially typecasted it to long and then > > changed to using grub_int64_t > > and could have done better on commit message. Do you prefer a new revision with > > better commit msg. > > If long is 32-bit on riscv32 should not we cast to grub_int32_t then? > > And then next question: Why off is defined as grub_int64_t? Ping? I want get that fixed in 2.06 release... Daniel
diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c index 00f49ccaa..ac677d03d 100644 --- a/util/grub-mkimagexx.c +++ b/util/grub-mkimagexx.c @@ -1242,7 +1242,7 @@ SUFFIX (relocate_addrs) (Elf_Ehdr *e, struct section_metadata *smd, */ sym_addr += addend; - off = sym_addr - target_section_addr - offset - image_target->vaddr_offset; + off = (grub_int64_t)sym_addr - target_section_addr - offset - image_target->vaddr_offset; switch (ELF_R_TYPE (info)) {
this makes the type promotions clear and explicit It was already typecasted to long but was accidentally dropped in [1] which stated to cause failures on riscv32 as reported in [2] [1] https://git.savannah.gnu.org/cgit/grub.git/commit/?id=2bf40e9e5be9808b17852e688eead87acff14420 [2] https://savannah.gnu.org/bugs/index.php?60283 Signed-off-by: Khem Raj <raj.khem@gmail.com> Cc: Andreas Schwab <schwab@suse.de> Cc: Daniel Kiper <daniel.kiper@oracle.com> Cc: Chester Lin <clin@suse.com> Cc: Nikita Ermakov <arei@altlinux.org> Cc: Alistair Francis <alistair.francis@wdc.com> --- util/grub-mkimagexx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)