[PATCH] D79545: [VE] Implements minimum MC layer for VE (3/4)

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Tests mostly look fine, thanks, but see inline comments.



================
Comment at: llvm/lib/Object/ELF.cpp:201-202
     break;
+  case ELF::EM_VE:
+    break;
   default:
----------------
Given you're no longer doing anything special here, perhaps it's okay to just let the `EM_VE` use `default` instead, by just deleting the case here. There are many other machines that are already in this position after all. If you go with that, I'd get rid of the corresponding unit test change from this patch. If you want to add unit tests for this function going forwards for it, it's perfectly fine to do so, but I'd add them in a separate patch and cover all the cases then.

What do you think?


================
Comment at: llvm/test/Object/VE/elf64-arch.yaml:1
+## Test that llvm-readobj shows proper arch names for VE target.
+
----------------
This and the other Object/VE tests can be deleted. I'm happy with the unit tests, and don't see a need for extra testing on top.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/machine-enum.test:1
+## Show that all machine codes are correctly printed.
+
----------------
I'd rename this file to file-header-machine-types.test to show it is specifically about that bit.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:23
+
+TEST(ELFObjectFileTest, BasicTestsForVE) {
+  std::vector<uint8_t> Data{
----------------
I'd rename this test to "VEMachineTest".


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:33
+      1,    0,                         // Type
+      251,  0,                         // Machine
+      1,    0,   0,   0,               // Version
----------------
Two competing ideas:

1) It might be nice if this blob could be shared between the different machine types in the future, so that most of the testing can be avoided. There's a way to do this using TEST_P instead of TEST, which allows you to specify parameters for the test. In this case, it would be `EM_VE` and `"elf64-ve"`. Take a look at some examples, e.g. in the DWARFDebugLineTest.cpp unit test file.
2) A way to reduce this down to YAML is by following a similar process to that used in the `MinidumpYAMLTest.cpp` unit test file (in the ObjectYAML directory). I think that would you to make this shorter and more focused on what is interesting. You'd need to use ELF-style YAML of course, like you already do in your lit tests.

I think both have merits. There may even be a way of combining the two, by somehow providing the YAML string for the machine at test run time via a parameter. I'd be happy with any of these approaches.

One suggestion: if you choose to stick with the byte array you already have, I'd replace `251` with EM_VE explicitly.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:46
+  };
+  // A very simple ELF object file which contains empty data
+  auto ExpectedFile = create<ELF64LE>(Data);
----------------
Nit: empty data -> no data

Also add the missing trailing full stop.


================
Comment at: llvm/unittests/Object/ELFTest.cpp:16-17
+
+TEST(ELFTest, checkMachineArchitectureId) { EXPECT_EQ(251, EM_VE); }
+TEST(ELFTest, getELFRelocationTypeIdForVE) {
+  EXPECT_EQ(0, R_VE_NONE);
----------------
These two TESTs are actually testing enum values from BinaryFormat/ELF.h and VE.def, if I'm not mistaken. I don't think there's much value in them - you're just as likely to make a mistake here in the test as you are in the enum, but also nobody's ever going to deliberately change this. Up to you though.


================
Comment at: llvm/unittests/Object/ELFTest.cpp:84
+  EXPECT_EQ("R_VE_CALL_LO32", getELFRelocationTypeName(EM_VE, R_VE_CALL_LO32));
+}
+TEST(ELFTest, getELFRelativeRelocationType) {
----------------
For readability, add a blank line after each individual TEST.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79545