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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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?



================
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))
----------------
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?


================
Comment at: llvm/test/tools/llvm-dwarfdump/AMDGPU/known-amdgcn-relocs.yaml:1
+# Tests AMDGPU relocations. We provide a .debug_info section with multiple
+# DW_AT_high_pc entries (that's one of the attributes for which relocations are
----------------
I don't know whether this is a pattern you'll see widely in llvm-dwarfdump tests, but in newer tests I've been involved with, we've often used `##` for comments, to distinguish them from the actual test logic.


================
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
----------------
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?


================
Comment at: llvm/test/tools/llvm-dwarfdump/AMDGPU/known-amdgcn-relocs.yaml:68
+    Flags:           [ SHF_INFO_LINK ]
+    Link:            .symtab
+    AddressAlign:    0x0000000000000008
----------------
I might be mistaken, but I believe this line is implicit, so can be omitted.


================
Comment at: llvm/test/tools/llvm-dwarfdump/AMDGPU/known-amdgcn-relocs.yaml:73
+
+      # Test 1
+      # CHECK: DW_AT_high_pc (0x0000000000000001)
----------------
Rather than just labelling these as "Test 1" etc, it might be worth stating why they are interesting cases.


================
Comment at: llvm/test/tools/llvm-dwarfdump/AMDGPU/known-amdgcn-relocs.yaml:130-141
+  - Name:            v0
+    Type:            STT_SECTION
+    Section:         .debug_info
+    Value:           0x0
+  - Name:            v1
+    Type:            STT_SECTION
+    Section:         .debug_info
----------------
Nit: your indentation in the last line here is a little off, but in that case, you might as well remove the excessive spacing between key and value, so that there's just one space between the longest key and its value (and all the other values line up with each other).


================
Comment at: llvm/test/tools/llvm-dwarfdump/AMDGPU/known-r600-relocs.yaml:1
+# Tests AMDGPU relocations. We provide a .debug_info section with multiple
+# DW_AT_high_pc entries (that's one of the attributes for which relocations are
----------------
Same comments as above.


================
Comment at: llvm/test/tools/llvm-dwarfdump/AMDGPU/unknown-amdgcn-relocs.yaml:1
+# Tests AMDGPU relocations. We provide a .debug_info section with multiple
+# DW_AT_high_pc entries (that's one of the attributes for which relocations are
----------------
This test seems to be practically identical to the first one. Could you fold them into a separate test file and use yaml2obj's -D functionality to parameterise the YAML, so that you don't have to repeat it?


================
Comment at: llvm/test/tools/llvm-dwarfdump/AMDGPU/unknown-r600-relocs.yaml:1
+# Tests AMDGPU relocations. We provide a .debug_info section with multiple
+# DW_AT_high_pc entries (that's one of the attributes for which relocations are
----------------
Same comments as other unknown test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144301