Page MenuHomePhabricator

[LoongArch 2/6] Add ELF machine flag and relocs for upcoming LoongArch target
ClosedPublic

Authored by SixWeining on Dec 16 2021, 12:48 AM.

Details

Summary

This patch adds necessary definitions for LoongArch ELF files, including
relocation types. Also adds initial support to ELFYaml, llvm-objdump,
and llvm-readobj in order to work with LoongArch ELFs.

Depends on D115857

Diff Detail

Event Timeline

SixWeining created this revision.Dec 16 2021, 12:48 AM
SixWeining requested review of this revision.Dec 16 2021, 12:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2021, 12:48 AM

Two nits. Otherwise looks good. Your comment references llvm-objdump, but there's nothing testing llvm-objdump here. I imagine it's just a case of "it just works", due to the Object library changes?

I haven't checked that the LOONGARCH definitions match up with any official spec. Do you have anything you could cite for these values i.e. ELF generic-abi mailing list for the EM value, and some psABI doc for the relocations?

Not that I think this particular change needs it, but has there been any particular buy-in from the community for integrating this architecture at all? (In this case, I don't think it's necessary, but for wider changes, it probably is).

Please note that I won't be online until the 4th of January, after today's work, so may be slow to respond to anything.

llvm/include/llvm/BinaryFormat/ELFRelocs/LoongArch.def
2

I imagine that this is copied from other files, but this blank line can be deleted.

I'm not sure why we don't include a license header at the top here, but I presume that's the case in the other files too?

46

You can reduce the amount of spaces, to make it slightly easier to line things up. I've highlighted this line, as it's the longest, but you can then move all the other numbers to match up.

llvm/test/tools/llvm-readobj/ELF/reloc-types-elf-loongarch64.test
1 ↗(On Diff #394776)

You can drop "elf" from the title of this test, since it's already in the ELF subdirectory.

llvm/unittests/Object/ELFObjectFileTest.cpp
295

Please clang-format your newly added code.

llvm/include/llvm/BinaryFormat/ELFRelocs/LoongArch.def
2
  1. OK, I will delete the first blank line.
  2. About the license header absence I think maybe the reason is this file is not a 'standalone' source or header file but a file that would be included by other files.
46

No problem. I will change that.

llvm/test/tools/llvm-readobj/ELF/reloc-types-elf-loongarch64.test
1 ↗(On Diff #394776)

Good suggestion. But we just follow other existing files naming method in the same directory. Maybe we can change all these files in a seperate patch later.

Two nits. Otherwise looks good. Your comment references llvm-objdump, but there's nothing testing llvm-objdump here. I imagine it's just a case of "it just works", due to the Object library changes?

I haven't checked that the LOONGARCH definitions match up with any official spec. Do you have anything you could cite for these values i.e. ELF generic-abi mailing list for the EM value, and some psABI doc for the relocations?

Not that I think this particular change needs it, but has there been any particular buy-in from the community for integrating this architecture at all? (In this case, I don't think it's necessary, but for wider changes, it probably is).

Please note that I won't be online until the 4th of January, after today's work, so may be slow to respond to anything.

Thanks @jhenderson and I have updated this revision to address your comments.
It is worth mentioning that this change is based on the LoongArch ELF ABI: https://loongson.github.io/LoongArch-Documentation/LoongArch-ELF-ABI-EN.html which has been upstreamed to binutils.
And this is the RFC proposing LoongArch to LLVM.

SixWeining retitled this revision from [LoongArch 2/n] Add ELF machine flag and relocs for upcoming LoongArch target to [LoongArch 2/5] Add ELF machine flag and relocs for upcoming LoongArch target.Dec 17 2021, 7:55 PM

This looks good to me too.

I will wait for more people to have a look at it before approving, especially during the long break until January.

xen0n added inline comments.Dec 19 2021, 11:26 PM
llvm/include/llvm/BinaryFormat/ELFRelocs/LoongArch.def
6

It may be better to point to the documentation source code with exact commit hash, because unfortunately there seems to be no versioning in the rendered website, so future revisions to the documentation could render this comment out-dated.

Also minor nit: it should probably say "from the LoongArch ELF psABI", both because the values are not modified here downstream, and to fully spell out the documentation title.

llvm/unittests/Object/ELFObjectFileTest.cpp
292

MachineTestForLoongArch for proper casing?

293

From my cursory look, it seems the 2nd and 4th elements are for verifying big-endian cases, yet LoongArch is little-endian-only according to documentation. So do you want to ignore big-endian cases and fix here to say elfNN-unknown instead?

Thanks for your comments @xen0n and I will update the revision to address them.

llvm/include/llvm/BinaryFormat/ELFRelocs/LoongArch.def
6

Thanks. That's reasonable and I will change it.

llvm/unittests/Object/ELFObjectFileTest.cpp
292

OK, I will change it.

293

Yes, LoongArch is little-endian only. But I think it doesn't mean LoongArch should only support identifing little-endian ELF files. For example, on little endian x86, the llvm-readelf tool can even recognize big-endian ELF file.

$ arch
x86_64
$ cat a.yaml 
--- !ELF
FileHeader:
  Class:   ELFCLASS64
  Data:    ELFDATA2MSB
  Type:    ET_REL
  Machine: EM_X86_64
$ yaml2obj a.yaml -o a.o
$ llvm-readelf -h a.o
ELF Header:
  Magic:   7f 45 4c 46 02 02 01 00 00 00 00 00 00 00 00 00
  Class:                             ELF64
  Data:                              2's complement, big endian
  Version:                           1 (current)
  OS/ABI:                            UNIX - System V
  ABI Version:                       0
  Type:                              REL (Relocatable file)
  Machine:                           Advanced Micro Devices X86-64
  Version:                           0x1
  ...

Adding owners of recent new targets to help onboarding this new target.

address comments of @xen0n about the psABI doc reference

SixWeining retitled this revision from [LoongArch 2/5] Add ELF machine flag and relocs for upcoming LoongArch target to [LoongArch 2/6] Add ELF machine flag and relocs for upcoming LoongArch target.Dec 22 2021, 5:08 PM
MaskRay added a subscriber: grimar.Jan 3 2022, 4:56 PM
MaskRay added inline comments.
llvm/test/tools/llvm-readobj/ELF/reloc-types-elf-loongarch64.test
1 ↗(On Diff #394776)

I will do the file renames for other reloc-types-elf-* tests (D71203) @grimar

jhenderson accepted this revision.Jan 5 2022, 2:24 AM

LGTM, but worth giving it a bit longer for others to finish commenting on too.

llvm/test/tools/llvm-readobj/ELF/reloc-types-elf-loongarch64.test
1 ↗(On Diff #394776)

(just noting to make sure that if @MaskRay does this first, to make sure you rename this file before it lands)

This revision is now accepted and ready to land.Jan 5 2022, 2:24 AM
SixWeining added inline comments.Jan 5 2022, 5:12 PM
llvm/test/tools/llvm-readobj/ELF/reloc-types-elf-loongarch64.test
1 ↗(On Diff #394776)

Thanks @jhenderson and @MaskRay.

I just checked that other files has already been renamed in this commit: d9cf9bd4b3c30221a2ec348cdfb01a24d84927b6. Now I will rename the loongarch test file in this patch.

SixWeining updated this revision to Diff 397747.Jan 5 2022, 5:17 PM

rename reloc-types-elf-loongarch64.test to reloc-types-loongarch64.test

MaskRay accepted this revision.Jan 5 2022, 7:40 PM
xen0n accepted this revision.Jan 5 2022, 9:55 PM
jhenderson accepted this revision.Jan 6 2022, 12:51 AM

LGTM again.

rengolin accepted this revision.Jan 6 2022, 1:30 AM
myhsu accepted this revision.Jan 6 2022, 4:08 AM
This revision was landed with ongoing or failed builds.Feb 10 2022, 2:23 AM
This revision was automatically updated to reflect the committed changes.

Hi it seems like this patch caused an error in the lldb build machines:

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/include/llvm/BinaryFormat/ELF.h:877:1: error: expected identifier
#include "ELFRelocs/LoongArch.def"
^
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/include/llvm/BinaryFormat/ELF.h:877:1: error: expected '}'
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/include/llvm/BinaryFormat/ELF.h:876:6: note: to match this '{'
enum {

^

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/include/llvm/BinaryFormat/ELF.h:876:7: error: expected ';' after enum
enum {

^
;

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/include/llvm/BinaryFormat/ELF.h:876:1: warning: declaration does not declare anything [-Wmissing-declarations]
enum {
^
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/include/llvm/BinaryFormat/ELF.h:877:1: fatal error: import of module 'LLVM_BinaryFormat.ELFRelocs.LoongArch' appears within namespace 'llvm::ELF'
#include "ELFRelocs/LoongArch.def"
^
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/include/llvm/BinaryFormat/ELF.h:27:1: note: namespace 'llvm::ELF' begins here
namespace ELF {
^
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/lib/BinaryFormat/AMDGPUMetadataVerifier.cpp:14:10: fatal error: could not build module 'LLVM_BinaryFormat'
#include "llvm/BinaryFormat/AMDGPUMetadataVerifier.h"
~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

link to bot here: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/41227/

Please fix or revert your patch as soon as possible!

Hi it seems like this patch caused an error in the lldb build machines:

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/include/llvm/BinaryFormat/ELF.h:877:1: error: expected identifier
#include "ELFRelocs/LoongArch.def"
^

I'm not sure how that's possible, given that that the code here isn't doing anything different than any other backend.

The only thing I could think about was the extra newline on LoogArch's enum defs that wasn't present in any other arch.

So I committed 695b629edd03 removing that newline. It's a wild guess, it's not relevant and it shouldn't hurt, but if that's what it was (because macros are stupid), then let me know.

If not, then we need to understand how you're building it because the other bots are fine and that is a build failure.

Ex. https://lab.llvm.org/buildbot/#/builders/188/builds/9576

So I committed 695b629edd03 removing that newline. It's a wild guess, it's not relevant and it shouldn't hurt, but if that's what it was (because macros are stupid), then let me know.

It did not work.

Can you explain how you build and why is this only breaking on your bot?

If I'm making a wild guess, it doesn't look like LoongArch.def is mentioned as a textual header in module.modulemap.

(The LLDB bot builds with -DLLVM_ENABLE_MODULES=On)

(The LLDB bot builds with -DLLVM_ENABLE_MODULES=On)

Ah, I see. I'll try that. Just did a standard LLDB build and it was fine. :)

If I'm making a wild guess, it doesn't look like LoongArch.def is mentioned as a textual header in module.modulemap.

So, I can reproduce the error now, but I'm not sure which one of the many modulemap file you're referring to.

I can't find any other ELFRelocs file in any of the many modulemap files in any of the monorepo projects.

Hi I think I know what to do and am trying to fix it myself. Thanks for trying!

Hi I think I know what to do and am trying to fix it myself. Thanks for trying!

Awesome, thanks!

Please post it here so we can track, too.

Closed with 957b24ca9f1e

Thanks for fixing the build error. I just submit a new patch D119519 to sort the textual headers by alphabetical order.