rampitec marked 2 inline comments as done. rampitec 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), ---------------- jhenderson wrote: > 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). Added in D85683. 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
