[PATCH] D144301: [dwarfdump][AMDGPU] Support EF_AMDGPU_MACH_NONE

[Scott Linder via Phabricator via llvm-commits]

scott.linder marked 7 inline comments as done.
scott.linder added a comment.

In D144301#4160749 <https://reviews.llvm.org/D144301#4160749>, @jhenderson wrote:

> It looks like this code is testing code in libObject, but the testing is in llvm-dwarfdump. Should the tests be moved to test/Object?

I can add more in test/Object, but the change was originally about providing a better experience in llvm-dwarfdump, so it seemed like the right place. There are no unittests of the RelocationResolver APIs that I can find, which would be the other appropriate place.



================
Comment at: llvm/lib/Object/RelocationResolver.cpp:262
+/// we use isUnknownAMDGPU to check for this case.
+static bool isUnknownAMDGPU(const ObjectFile &Obj) {
+  if (const auto *ELFObj = dyn_cast<ELFObjectFileBase>(&Obj))
----------------
jhenderson wrote:
> I think this function is a bit incorrectly named. Strictly-speaking, this will return true/false only based on the EM* value, so it doesn't say anything about whether the object is a "known" AMDGPU version. Perhaps should just be named `isAMDGPUObject` or similar?
Agreed, I changed the name to `isAMDGPU`, but retained the exposition in the doc-comment.


================
Comment at: llvm/test/tools/llvm-dwarfdump/AMDGPU/known-amdgcn-relocs.yaml:7
+
+# To add more tests you need to modify the Content of the .debug_abbrev and
+# .debug_info sections. To do that create a test.s which matches the assembly
----------------
jhenderson wrote:
> Have you looked into using the yaml2obj support for Dwarf? I haven't used it much, but you should see some examples in the yaml2obj testing on how to use it. It may or may not be sufficient for your testing purposes.
> 
> Alternatively, what does using YAML here give you above using the assembly directly in the test?
I didn't realize it supported DWARF directly :^) thank you for the pointer

I moved everything into the YAML, let me know what you think


================
Comment at: llvm/test/tools/llvm-dwarfdump/AMDGPU/known-amdgcn-relocs.yaml:73
+
+      # Test 1
+      # CHECK: DW_AT_high_pc (0x0000000000000001)
----------------
jhenderson wrote:
> Rather than just labelling these as "Test 1" etc, it might be worth stating why they are interesting cases.
The description would just repeat the contents of the test; there is nothing particularly interesting about any case, I just threw multiple variations at each relocation kind to get some coverage.

I just deleted the redundant "Test #" comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144301