[PATCH] D80713: [AMDGPU] Support disassembly for AMDGPU kernel descriptors

[James Henderson via Phabricator via llvm-commits]

jhenderson reopened this revision.
jhenderson added a comment.

Hi @rochauha,

I'm not sure this should have been pushed - nobody has reviewed your latest update, and I've had concerns about the testing prior to that, as stated. There is also a "Needs Revision" marker still outstanding, and I usually take that to mean that this shouldn't be pushed until the relevant reviewer is satisfied, regardless of what others have said (note that Phabricator highlighted that this patch landded in a "Needs Review" state).

Please could you revert, as there are potential assertion failures in my reading of this code, at least in the event you hit malformed input, in addition to the other issues raised.



================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1220-1225
+  // For some malformed KD cases, the Cursor tends to hold Error::success().
+  // We check that here to prevent runtime crash in this case.
+  if (!C) {
+    auto Err = C.takeError();
+    assert(!Err);
+  }
----------------
This is not how to handle malformed input. This will result in an assertion in debug builds, which is equivalent to a crash, without any useful context to draw on, because you've not reported the error. More below.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1408
+    FourByteBuffer = DE.getU32(Cursor);
+    checkError(Cursor);
+    KdStream << Indent << ".amdhsa_group_segment_fixed_size " << FourByteBuffer
----------------
Rather than all these `checkError` calls, I'd expect to see a check of the `Cursor` followed by a return of `MCDisassembler::Fail` to indicate there was a problem.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1542-1548
+  while (C && C.tell() < Bytes.size()) {
+    MCDisassembler::DecodeStatus Status =
+        decodeKernelDescriptorDirective(C, Bytes, KdStream);
+
+    if (Status == MCDisassembler::Fail)
+      return MCDisassembler::Fail;
+  }
----------------
If this loop terminates due to `C` being invalid, you don't want to fall out the bottom and return `Success`, I think. You'd want to check `C` after loop termination and return `Fail`. Alternatively, if you return `Fail` from the `decodeKernelDescriptorDirective` you can do `cantFail(C.takeError());` after the loop.

Ideally, we'd actually report the error from `C` in the event of failure, but currently there's no way of communicating that back up to the caller.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/AMDGPU/kd-failure.s:1
+; RUN: llvm-mc %s -mattr=+code-object-v3 --triple=amdgcn-amd-amdhsa -mcpu=gfx908 -filetype=obj -o %t.o
+
----------------
Please add a comment to the top of this test explaining what this test is actually testing.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/AMDGPU/kd-failure.s:3
+
+; RUN: printf ".type  my_kernel.kd, @object \nmy_kernel.kd:\n.size my_kernel.kd, 64\n" > %t1.sym_info
+; RUN: llvm-objdump --arch-name=amdgcn --mcpu=gfx908 --disassemble-symbols=my_kernel.kd %t.o \
----------------
Rather than writing text to another input file at run time in this way, you can use the new split-file tool to have all the input inline below, and split it up using the tool into multiple files.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/AMDGPU/kd-failure.s:11
+
+// Test failure by setting one of the reserved bytes to non-zero value.
+
----------------
Please don't mix comment markers within the same file. You use '//' here, but ';' everywhere else.

Additionally, new LLVM binutils tests tend to use double comment markers to indicate true comments as opposed to RUN/CHECK lines (i.e. ';;' in this context).


================
Comment at: llvm/test/tools/llvm-objdump/ELF/AMDGPU/kd-failure.s:34-35
+  .short 0x0000
+
+
----------------
Please remove the additional blank lines at the end of file.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/AMDGPU/kd-sgpr.s:1
+; RUN: llvm-mc %s -mattr=+code-object-v3 --triple=amdgcn-amd-amdhsa -mcpu=gfx908 -filetype=obj -o %t
+; RUN: llvm-objdump --arch-name=amdgcn --mcpu=gfx908 --disassemble-symbols=my_kernel_1.kd %t | tail -n +8 > %t1.s
----------------
Please follow the comments from `kd-failure.s` in all the tests (where applicable) too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80713