Page MenuHomePhabricator

[LoongArch] Parse LoongArch base ABI in ObjectYAML and llvm-readobj
ClosedPublic

Authored by SixWeining on Jul 21 2022, 3:00 AM.

Diff Detail

Event Timeline

SixWeining created this revision.Jul 21 2022, 3:00 AM
SixWeining requested review of this revision.Jul 21 2022, 3:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 3:00 AM

Is https://github.com/loongson/LoongArch-Documentation/pull/47 (using 0x1, 0x2, 0x3 for ILP32S/F/D and use EI_CLASS to distinguish LP64 and ILP32) dead?

Where you've added the contents to different files, you seeme to have added them at differenet relative places. For ease of reading, it would be better to keep platform-specific code in the same order in each file (I acknowledge it isn't perfect as it stands anyway, but we should avoid making it worse). Specifically, in same places, you've made your changes before the corresponding AVR block, and in others you've made them after. It don't care about the exact ordering, as long as it's consistent across the files.

llvm/test/Object/LoongArch/elf-flags.yaml
1 ↗(On Diff #446407)

I don't think the Object library is the place for this testing, as the test tests functionality in ObjectYAML and llvm-readobj code, not in the Object library. I think you should split the test into two tests as follows:

  1. In test/ObjectYAML, have a test that uses yaml2obj to generate object files that are converted back to YAML using obj2yaml. Check the resultant YAML contains the appropriate flags.
  2. In test/tools/llvm-readobj, use yaml2obj to generate object files, and use llvm-readobj (and llvm-readelf) to inspect them.
62–67 ↗(On Diff #446407)

As you're only testing the flags, you don't need most of these lines. Just the Flags line would be enough.

Add referenced documentation in comment.

Is https://github.com/loongson/LoongArch-Documentation/pull/47 (using 0x1, 0x2, 0x3 for ILP32S/F/D and use EI_CLASS to distinguish LP64 and ILP32) dead?

Thanks.
I add some comments in the e_flags definition just now. If that PR get merged, let me update the definition.

SixWeining marked an inline comment as done.

Remove unnecessary check lines.

llvm/test/Object/LoongArch/elf-flags.yaml
1 ↗(On Diff #446407)

Thanks. But seems other archs also put the test in the same place. And I saw no platform-specific test in test/ObjectYAML. What should I do?

62–67 ↗(On Diff #446407)

Sounds good. Thanks.

jhenderson added inline comments.Jul 21 2022, 6:29 AM
llvm/test/Object/LoongArch/elf-flags.yaml
62–67 ↗(On Diff #446441)

We don't need strict whitespace testing here, so let's neaten this all up.

1 ↗(On Diff #446407)

I might look into fixing the other testing at some point to be in the right place. A quick look shows testing of MIPS e_flags in tools/obj2yaml, so perhaps the ob2yaml aspect can go there? The test would be named something like loong-arch-eflags.yaml, for similarity.

However, I don't feel strongly about this, so if you prefer to stick with existing precedent, that's fine.

SixWeining marked 2 inline comments as done.

Place the test to tools/llvm-readobj and tools/obj2yaml.

SixWeining added inline comments.Jul 21 2022, 7:11 PM
llvm/test/Object/LoongArch/elf-flags.yaml
62–67 ↗(On Diff #446441)

Yes. Thanks.

1 ↗(On Diff #446407)

Thanks. I think the MIPS approach makes sense.

jhenderson added inline comments.Jul 22 2022, 12:14 AM
llvm/test/tools/llvm-readobj/ELF/loongarch-eflags.test
4

Minor nit: we're tending to move away from piping to a file in favour of using -o to write the output to that file. The logic is that it's easier to copy and paste the command from the test output to reproduce the test run.

65

Didn't see this earlier, sorry, but yaml2obj has an option (which might not have been available when existing tests were written) to allow text substitution within the YAML, a bit like FileCheck's -D option. Approximate example (not tested, but should be enough of an idea to get it to work):

# RUN: yaml2obj %s -o %t-lp64s -DFLAG=LP64S

...

--- !ELF
FileHeader:
  Class:           ELFCLASS64
  Data:            ELFDATA2LSB
  Type:            ET_EXEC
  Machine:         EM_LOONGARCH
  Flags:           [ EF_LOONGARCH_BASE_ABI_[[FLAG]] ]

This will allow you to only have one YAML block.

65

Missed that there was different Class values in some of the YAML blocks, but that can be solved in the same manner with a different variable.

llvm/test/tools/obj2yaml/ELF/loongarch-eflags.yaml
4

Same comments as the other test re. -D and -o options.

SixWeining marked 4 inline comments as done.

Use -D and -o options for yaml2obj.

llvm/test/tools/llvm-readobj/ELF/loongarch-eflags.test
4

OK. Thanks.

65

Thanks. Yes, this can reduce many lines.

This revision is now accepted and ready to land.Jul 22 2022, 1:08 AM
MaskRay accepted this revision.Jul 22 2022, 12:11 PM

Looks great!

llvm/include/llvm/BinaryFormat/ELF.h
920

Add a comma for the last element so that when a new one is added, the contributor doesn't have to touch the mask line.

llvm/test/tools/llvm-readobj/ELF/loongarch-eflags.test
28

Align Flags with ] 2 lines below. For readability, indent EF_LOONGARCH_BASE_ABI_LP64S by 2 spaces

llvm/test/tools/obj2yaml/ELF/loongarch-eflags.yaml
31

You can delete ...

SixWeining marked 3 inline comments as done.

indent

llvm/test/tools/obj2yaml/ELF/loongarch-eflags.yaml
31

Yes. And the same as llvm/test/tools/llvm-readobj/ELF/loongarch-eflags.test. Thanks.

This revision was landed with ongoing or failed builds.Jul 25 2022, 5:41 AM
This revision was automatically updated to reflect the committed changes.