Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LoongArch hi64 calculation error and PCALA should set plt if sym is imported #1131

Closed
MQ-mengqing opened this issue Oct 19, 2023 · 3 comments

Comments

@MQ-mengqing
Copy link

I find when I test mold in ArchLinux, something wrongs.

1,hi64 calculation error

The [63:32] bits should be PC-relative, too. The current codes seems only return val-relative.

diff --git a/elf/arch-loongarch.cc b/elf/arch-loongarch.cc
index b5bd0878..4bc60f57 100644
--- a/elf/arch-loongarch.cc
+++ b/elf/arch-loongarch.cc
@@ -66,12 +66,11 @@ static u64 hi64(u64 val, u64 pc) {
   //
   // Compensating all the sign-extensions is a bit complicated.
   bool x = val & 0x800;
-  bool y = (page(val + 0x800) - page(pc)) & 0x8000'0000;
-
-  if (x && !y)
-    return val - 0x1'0000'0000;
-  if (!x && y)
-    return val + 0x1'0000'0000;
+  val = page(val) - page(pc);
+  if (x)
+    val += 0x1000 - 0x1'0000'0000;
+  if (val & 0x8000'0000)
+    val += 0x1'0000'0000;
   return val;
 }

[1] https://github.com/llvm/llvm-project/blob/main/lld/ELF/Arch/LoongArch.cpp#L158
[2] https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=bfd/elfnn-loongarch.c;#l2304

2, PCALA should set plt if sym is imported

PCALA used for a long call in -mcmodel=extreme mode. The sequence is "la.pcrel $rxx, $ryy, sym; jirl $ra, $rxx, 0"
So if the sym is imported, we also need add NEED_PLT.

@@ -587,6 +586,7 @@ void InputSection<E>::scan_relocations(Context<E> &ctx) {
       scan_dyn_absrel(ctx, sym, rel);
       break;
     case R_LARCH_B26:
+    case R_LARCH_PCALA_HI20:
       if (sym.is_imported)
         sym.flags |= NEEDS_PLT;
       break;
@@ -620,7 +620,6 @@ void InputSection<E>::scan_relocations(Context<E> &ctx) {
     case R_LARCH_ABS_LO12:
     case R_LARCH_ABS64_LO20:
     case R_LARCH_ABS64_HI12:
-    case R_LARCH_PCALA_HI20:
     case R_LARCH_PCALA_LO12:
     case R_LARCH_PCALA64_LO20:
     case R_LARCH_PCALA64_HI12:
@rui314 rui314 closed this as completed in a9ab0ff Oct 19, 2023
@rui314
Copy link
Owner

rui314 commented Oct 19, 2023

Thanks. Should be fixed in the above commit.

@rui314
Copy link
Owner

rui314 commented Oct 19, 2023

Slightly off-topic, but as I pointed out several times, I think you guys want to define a "no check" version of R_LARCH_PCALA_HI20 relocation. Currently, the same relocation is used for the usual code model and the extreme code model, so we can't do overflow check for R_LARCH_PCALA_HI20, meaning that we can't report a relocation overflow error from the linker for that relocation.

I removed the relocation overflow check in the above commit. It's not ideal though.

@MQ-mengqing
Copy link
Author

Thanks for you pointing out it. I have reported it to our internal group.
BTW, we had found that if PCALA_HI20 and PCALA64_{LO20,HI12} are not in the same 4k-page may cause relocation errors. (The edge case is (-2 + 4N)G ). I hope we may fix later by add relocations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants