jhenderson added inline comments.
================ Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1844 LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX1030), + LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX1031), LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_XNACK), ---------------- rampitec wrote: > jhenderson wrote: > > Where's the dedicated llvm-readobj test for this? Please add one. If there > > exists one for the other AMDGPU flags, extend that one, otherwise, it might > > be best to write one for the whole set at once. > It is in the lvm/test/CodeGen/AMDGPU/elf-header-flags-mach.ll above along > with all other targets. I emphasise "dedicated". llvm/test/CodeGen... is not the place for tests for llvm-readobj code. Such tests belong in llvm/test/tools/llvm-readobj. To me, the CodeGen test looks like a test of the llc behaviour of generating the right value. It uses llvm-readobj, and therefore only incidentally tests the dumping behaviour. Imagine a situation where we decided to add support for this to llvm-objdump, and then somebody decides to switch the test over to using llvm-objdump to match (maybe it "looks better" that way). That would result in this code in llvm-readobj being untested. My opinion, and one I think is shared with others who maintain llvm-readobj (amongst other things) is that you need direct testing for llvm-readobj behaviour within llvm-readobj tests to avoid such situations. This would therefore mean that this change needs two tests: 1) A test in llvm/test/tools/llvm-readobj/ELF which uses a process (e.g. yaml2obj) to create the ELF header with the required flag, to ensure the dumping behaviour is correct. 2) A test in llvm/test/CodeGen which tests that llc produces the right output value in the ELF header flags field (which of course would use llvm-readobj currently, but could use anything to verify). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85337/new/ https://reviews.llvm.org/D85337 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
