[Lldb-commits] [PATCH] D145462: [LLDB][ObjectFileELF] Support LoongArch64 in ApplyReloctions

Tue Mar 7 22:43:48 PST 2023

SixWeining added inline comments.

================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2623
+        (is_signed &&
+         ((int64_t)value > INT32_MAX && (int64_t)value < INT32_MIN))) {
+      Log *log = GetLog(LLDBLog::Modules);
----------------
DavidSpickett wrote:
> Should this be `||` not `&&`?
Yes I think so. This should be an error in original code but not introduced this time. Do you mind I include the fix in current patch or in a separate one?

================
Comment at: lldb/test/Shell/ObjectFile/ELF/loongarch64-relocations.yaml:10
+# CHECK-NEXT: )
+
+--- !ELF
----------------
DavidSpickett wrote:
> Please add a comment to explain briefly how this proves the relocations are being processed. As it is I don't see how the content of the data section proves that any work was done, besides us not crashing.
> 
> Or the other way around, if the relocations were not applied what would we see here?
No problem.

Actually I just add this test like what aarch64 and i386 did before. Let me simplify the test and make it more readable. Thanks.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145462