[PATCH] D101304: AMDGPU/llvm-readobj: Add missing tests for note parsing/displaying

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-amd-invalid-v2.s:1
+# RUN: yaml2obj %s -o %t.o
+# RUN: llvm-readobj --notes %t.o | FileCheck %s --check-prefix=LLVM
----------------
Please add a brief top-level comment to this test explaining the test's purpose.

It would also be helpful to have some inline comments next to each test case (possibly by the YAML) explaining why each case is invalid for future reference.

Same comments apply to each of the new tests.

Also, these should probably have a test file name ending in `.test`, since they aren't written in assembly.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-amd-invalid-v2.s:121
+        Type: NT_AMD_HSA_CODE_OBJECT_VERSION
+        Desc: '02000000'
+  - Name:     .note.nt_amd_hsa_hsail
----------------
It might be a good idea to have one that's too big as well as too small, to show that the size has to match exactly.

Same goes for the other cases where the desc size is incorrect.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-amd-invalid-v3.s:30
+    Type:     SHT_NOTE
+    Content:     07000000D400000020000000414D44475055000081ae616d646873612e6b65726e656c73918ab92e67726f75705f7365676d656e745f66697865645f73697a6502b62e6b65726e6172675f7365676d656e745f616c69676e04b52e6b65726e6172675f7365676d656e745f73697a6501b82e6d61785f666c61745f776f726b67726f75705f73697a6508a52e6e616d65a3666f6fbb2e707269766174655f7365676d656e745f66697865645f73697a6503ab2e736770725f636f756e7406a72e73796d626f6ca3666f6fab2e766770725f636f756e7407af2e7761766566726f6e745f73697a6505
----------------
Is this the minimum the data can be in order to trigger the code path you are testing here?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-amd-valid-v2.s:2-3
+# RUN: yaml2obj %s -o %t.o
+# RUN: llvm-readobj --notes %t.o | FileCheck %s --check-prefix=LLVM
+# RUN: llvm-readelf --notes %t.o | FileCheck %s --check-prefix=GNU
+
----------------
Here and elsewhere, you might want something like `--match-full-lines` for the FileCheck invocation. That will ensure no garbage is printed after the expected string on any given line of output, which is especially important for where strings are being printed (in case something is wrong regarding null termination in the code).


================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-amd-valid-v2.s:165
+        Type: NT_AMD_HSA_METADATA
+        Desc: '2d2d2d0a56657273696f6e3a2020202020202020205b20312c2030205d0a4b65726e656c733a0a20202d204e616d653a202020202020202020202020656c665f6e6f7465730a2020202053796d626f6c4e616d653a20202020202027656c665f6e6f746573406b64270a20202020436f646550726f70733a0a2020202020204b65726e6172675365676d656e7453697a653a20300a20202020202047726f75705365676d656e74466978656453697a653a20300a202020202020507269766174655365676d656e74466978656453697a653a20300a2020202020204b65726e6172675365676d656e74416c69676e3a20340a2020202020205761766566726f6e7453697a653a20202036340a2020202020204e756d53475052733a202020202020202039360a2020202020204d6178466c6174576f726b47726f757053697a653a20313032340a2e2e2e0a'
+  - Name:     .note.nt_amd_hsa_isa_name 
----------------
If I'm reading this correctly, this is just a string, in which case it can be any string, e.g. "foo" rather than some very large binary blob. I recommend simplifying drastically - it doesn't need to be a real-world case for testing purposes. Also, I believe you need a case for this note type where `Desc.size() == 0`. That may be tested elsewhere, but it would make sense to explicitly check it in llvm-readobj too.

Same goes for the `NT_AMD_HSA_ISA_NAME` case.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-amd-valid-v3.s:225
+        Type: NT_AMDGPU_METADATA
+        Desc: '83AE616D646873612E6B65726E656C73938DA52E617267739385AE2E616464726573735F7370616365A6676C6F62616CA52E6E616D65A172A72E6F666673657400A52E73697A6508AB2E76616C75655F6B696E64AD676C6F62616C5F62756666657285AE2E616464726573735F7370616365A6676C6F62616CA52E6E616D65A161A72E6F666673657408A52E73697A6508AB2E76616C75655F6B696E64AD676C6F62616C5F62756666657285AE2E616464726573735F7370616365A6676C6F62616CA52E6E616D65A162A72E6F666673657410A52E73697A6508AB2E76616C75655F6B696E64AD676C6F62616C5F627566666572B92E67726F75705F7365676D656E745F66697865645F73697A6500B62E6B65726E6172675F7365676D656E745F616C69676E08B52E6B65726E6172675F7365676D656E745F73697A6518B82E6D61785F666C61745F776F726B67726F75705F73697A65CD0400A52E6E616D65A466616464BB2E707269766174655F7365676D656E745F66697865645F73697A6500AB2E736770725F636F756E7408B12E736770725F7370696C6C5F636F756E7400A72E73796D626F6CA7666164642E6B64AB2E766770725F636F756E7403B12E766770725F7370696C6C5F636F756E7400AF2E7761766566726F6E745F73697A65408DA52E617267739385AE2E616464726573735F7370616365A6676C6F62616CA52E6E616D65A172A72E6F666673657400A52E73697A6508AB2E76616C75655F6B696E64AD676C6F62616C5F62756666657285AE2E616464726573735F7370616365A6676C6F62616CA52E6E616D65A161A72E6F666673657408A52E73697A6508AB2E76616C75655F6B696E64AD676C6F62616C5F62756666657285AE2E616464726573735F7370616365A6676C6F62616CA52E6E616D65A162A72E6F666673657410A52E73697A6508AB2E76616C75655F6B696E64AD676C6F62616C5F627566666572B92E67726F75705F7365676D656E745F66697865645F73697A6500B62E6B65726E6172675F7365676D656E745F616C69676E08B52E6B65726E6172675F7365676D656E745F73697A6518B82E6D61785F666C61745F776F726B67726F75705F73697A65CD0400A52E6E616D65A466737562BB2E707269766174655F7365676D656E745F66697865645F73697A6500AB2E736770725F636F756E7408B12E736770725F7370696C6C5F636F756E7400A72E73796D626F6CA7667375622E6B64AB2E766770725F636F756E7403B12E766770725F7370696C6C5F636F756E7400AF2E7761766566726F6E745F73697A65408DA52E617267739484A52E6E616D65A169A72E6F666673657400A52E73697A6504AB2E76616C75655F6B696E64A862795F76616C756585AE2E616464726573735F7370616365A6676C6F62616CA52E6E616D65A172A72E6F666673657408A52E73697A6508AB2E76616C75655F6B696E64AD676C6F62616C5F62756666657285AE2E616464726573735F7370616365A6676C6F62616CA52E6E616D65A161A72E6F666673657410A52E73697A6508AB2E76616C75655F6B696E64AD676C6F62616C5F62756666657285AE2E616464726573735F7370616365A6676C6F62616CA52E6E616D65A162A72E6F666673657418A52E73697A6508AB2E76616C75655F6B696E64AD676C6F62616C5F627566666572B92E67726F75705F7365676D656E745F66697865645F73697A6500B62E6B65726E6172675F7365676D656E745F616C69676E08B52E6B65726E6172675F7365676D656E745F73697A6520B82E6D61785F666C61745F776F726B67726F75705F73697A65CD0400A52E6E616D65A5656D707479BB2E707269766174655F7365676D656E745F66697865645F73697A6500AB2E736770725F636F756E7400B12E736770725F7370696C6C5F636F756E7400A72E73796D626F6CA8656D7074792E6B64AB2E766770725F636F756E7400B12E766770725F7370696C6C5F636F756E7400AF2E7761766566726F6E745F73697A6540AD616D646873612E746172676574B9616D6467636E2D616D642D616D646873612D2D676678383033AE616D646873612E76657273696F6E920101'
----------------
Such a large (and opaque) blob might mean it's easier to write this test in assembly and use llvm-mc to generate the output. The same goes for the invalid version of this case.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5058
           << ", HSAIL Minor: " << Properties->HSAILMinorVersion
-          << ", Profile: " << Properties->Profile
-          << ", Machine Model: " << Properties->MachineModel
-          << ", Default Float Round: " << Properties->DefaultFloatRound << "]";
+          << ", Profile: "
+          << uint32_t(Properties->Profile)
----------------
Nit: run clang-format.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5114
     raw_string_ostream StrOS(MetadataString);
-    for (size_t I = 0, E = Desc.size() / sizeof(PALMetadata); I < E; ++E) {
+    for (size_t I = 0, E = Desc.size() / sizeof(PALMetadata); I < E; ++I) {
       StrOS << "[" << Isa[I].Key << ": " << Isa[I].Value << "]";
----------------
I missed this before, but what happens if `Desc` isn't a multiple of `sizeof(PALMetadata)`? It looks to me like you'd read off the end of the note data, which should trigger some sort of error (but currently isn't checked). Please fix and add a test case.


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

https://reviews.llvm.org/D101304