[PATCH] D138135: [WIP][ELF] Support LoongArch

[WÁNG Xuěruì via Phabricator via llvm-commits]

xen0n added inline comments.


================
Comment at: lld/ELF/Arch/LoongArch.cpp:430
+  case R_LARCH_32:
+  case R_LARCH_32_PCREL:
+    write32le(loc, val);
----------------
MQ-mengqing wrote:
> For R_LARCH_32_PCREL, it is better to check whether overflow.
Overflow checking will be polished later. For now I'm focusing on overall correctness which is quite an undertaking already...


================
Comment at: lld/ELF/Arch/LoongArch.cpp:470
+  case R_LARCH_TLS_IE_LO12: {
+    // TODO: checkInt(loc, SignExtend64(hi, bits) >> 12, 20, rel);
+    write32le(loc, setK12(read32le(loc), extractBits(val, 11, 0)));
----------------
MQ-mengqing wrote:
> The R_LARCH_{GOT_LO12, LE_LO12, IE_LO12} are more like R_LARCH_ABS_LO12.
Yeah the operation carried out is the same. I'll coalesce into one category later.


================
Comment at: lld/ELF/Arch/LoongArch.cpp:487
+  case R_LARCH_TLS_GD_HI20: {
+    // TODO: checkInt(loc, SignExtend64(hi, bits) >> 12, 20, rel);
+    write32le(loc, setJ20(read32le(loc), extractBits(val, 31, 12)));
----------------
MQ-mengqing wrote:
> Why so many TODOs here?
> 
Because this patch is clearly marked as `[WIP]`? I'm just posting the in-progress work early in order to not overwhelm everyone later in the review cycle.


================
Comment at: lld/ELF/InputSection.cpp:646
   case R_RELAX_TLS_GD_TO_IE_ABS:
+    // XXX Why???
+    if (config->emachine == EM_LOONGARCH && sym.isTls()) {
----------------
MQ-mengqing wrote:
> Due to loongarch override the GOT(64)_(PC_){LO12, LO20, HI12} in the LD/GD case, the logic is complex.
> I found it is hardly to get the right value form the value of TlsIndex or  the value of GlobalDynamic, when I ported lld to llvm-12 in our internal repository.
> And I tried to use others RelExpr to get them correctly. For GD and LD we should get the GlobalDynamic and for IE we should get the TlsIndex.
> If you do not distinguish them in LoongArch::getRelExpr, it might wrong, or I missed things in llvm-16.
> Is there no tls test in this patch?
Ah. My intuition is unfortunately right, the BFD implementation is hiding too much logic behind innocent-looking relocs... and the logic isn't documented so I can't figure out by only looking at the docs.

For now it seems more `RelExpr`'s are indeed necessary as you said. I'll go through the code once more and try to come up with some solution in the next revision. Thanks for the explanation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138135/new/

https://reviews.llvm.org/D138135