This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][ObjectFileELF] Support LoongArch64 in ApplyReloctions
ClosedPublic

Authored by SixWeining on Mar 6 2023, 7:51 PM.

Details

Summary

Currently ApplyReloctions() deals with different archs' relocation types
together (in a single switch() {..}). I think it is incorrect because
different relocation types of different archs may have same enum values.
For example:
R_LARCH_32 and R_X86_64_64 are both 1;
R_LARCH_64 and R_X86_64_PC32 are both 2.

This patch handles each arch in seperate switch() to solve the enum
values conflict issue.

And a new test is added for LoongArch64.

Diff Detail

Event Timeline

SixWeining created this revision.Mar 6 2023, 7:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2023, 7:51 PM
SixWeining requested review of this revision.Mar 6 2023, 7:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2023, 7:51 PM
SixWeining edited the summary of this revision. (Show Details)Mar 6 2023, 7:52 PM

I see from /usr/include/elf.h on my system that the numbers do restart for each architecture. We got away with it for the set of relocations we were looking at. I like the new structure, it is clearer especially for the signed checks.

I presume the definitions for the relocations actually come from llvm/include/llvm/BinaryFormat/ELFRelocs/LoongArch.def, so this will build fine with a toolchain that doesn't know about LoongArch yet.

lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
2623

Should this be || not &&?

DavidSpickett added inline comments.Mar 7 2023, 2:11 AM
lldb/test/Shell/ObjectFile/ELF/loongarch64-relocations.yaml
11

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?

SixWeining added inline comments.Mar 7 2023, 10:43 PM
lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
2623

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?

lldb/test/Shell/ObjectFile/ELF/loongarch64-relocations.yaml
11

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.

Address @DavidSpickett's comments.

  1. Fix the && issue.
  2. Make the test more readable.
DavidSpickett accepted this revision.Mar 8 2023, 1:17 AM

LGTM.

lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
2623

In this patch is fine.

This revision is now accepted and ready to land.Mar 8 2023, 1:17 AM