[PATCH] D130238: [LoongArch] Parse LoongArch base ABI in ObjectYAML and llvm-readobj

Thu Jul 21 19:11:23 PDT 2022

SixWeining added inline comments.

================
Comment at: llvm/test/Object/LoongArch/elf-flags.yaml:62-67
+# YAML-LP64S:   Flags:           [ EF_LOONGARCH_BASE_ABI_LP64S ]
+# YAML-LP64F:   Flags:           [ EF_LOONGARCH_BASE_ABI_LP64F ]
+# YAML-LP64D:   Flags:           [ EF_LOONGARCH_BASE_ABI_LP64D ]
+# YAML-ILP32S:   Flags:           [ EF_LOONGARCH_BASE_ABI_ILP32S ]
+# YAML-ILP32F:   Flags:           [ EF_LOONGARCH_BASE_ABI_ILP32F ]
+# YAML-ILP32D:   Flags:           [ EF_LOONGARCH_BASE_ABI_ILP32D ]
----------------
jhenderson wrote:
> We don't need strict whitespace testing here, so let's neaten this all up.
Yes. Thanks.

================
Comment at: llvm/test/Object/LoongArch/elf-flags.yaml:1
+# RUN: yaml2obj --docnum=1 %s > %t-lp64s
+# RUN: llvm-readobj -h %t-lp64s | FileCheck --check-prefix=READOBJ-LP64S %s
----------------
jhenderson wrote:
> SixWeining wrote:
> > jhenderson wrote:
> > > 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.
> > 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?
> 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.
Thanks. I think the MIPS approach makes sense.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130238