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

[Kazushi Marukawa via Phabricator via llvm-commits]

kaz7 marked 7 inline comments as done.
kaz7 added a comment.

Thanks for inspecting tests.  I'll update them Monday again.



================
Comment at: llvm/lib/Object/ELF.cpp:201-202
     break;
+  case ELF::EM_VE:
+    break;
   default:
----------------
jhenderson wrote:
> 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?
I'll remove EM_VE here, but leave getELFRelativeRelocationTypeTest. .  Please add tests for other architectures from this patch in a separate patch at your side.  I'd like to reduce the size of this patch.


================
Comment at: llvm/test/Object/VE/elf64-arch.yaml:1
+## Test that llvm-readobj shows proper arch names for VE target.
+
----------------
jhenderson wrote:
> 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.
Thanks.  I'll remove them.


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


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


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:33
+      1,    0,                         // Type
+      251,  0,                         // Machine
+      1,    0,   0,   0,               // Version
----------------
jhenderson wrote:
> 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.
Thank you for suggestions.  I'll take care them Monday.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:46
+  };
+  // A very simple ELF object file which contains empty data
+  auto ExpectedFile = create<ELF64LE>(Data);
----------------
jhenderson wrote:
> Nit: empty data -> no data
> 
> Also add the missing trailing full stop.
Thanks for the correction.  I'm not sure about "trailing full stop" since I've never tried to generate ELF data previously.  I guess you are saying that above blob is not completed and need some trailing data.   I appreciate if you advise me with some concrete examples or references.


================
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);
----------------
jhenderson wrote:
> 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.
I agree with you.  I'll remove them.  Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79545