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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've run out of time, so will come back to looking at the rest of the updates tomorrow.



================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:33
+      1,    0,                         // Type
+      251,  0,                         // Machine
+      1,    0,   0,   0,               // Version
----------------
kaz7 wrote:
> kaz7 wrote:
> > 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.
> There is a problem to use TEST_P.  TEST_P is for parameterize tests in my understanding.  For example, it is useful to test something identical conditions for different parameters.  However, we need different conditions for each parameter, e.g. name "elf64-ve" for EM_VE and name "elf64-x86-64" for EM_X86_64.
> 
> I tried TYPED_TEST_P, TEST_F, etc.  All of them doesn't match our purpose, I think.  This time, I only made a common class to obtain a buffer to create different ELFObjectFile.
> 
> I will also try to implement a yaml test.  There is a problem to implement a method to traverse generated ELFObjectFile, though.  I'll try it by reading tools/llvm-readobj/ELFDumper.cpp, but it takes for a while for me.
You can use std::tuple to create a parameterised test that has multiple properties. See for example `MaxOpsPerInstFixture` in `DWARFDebugLineTest.cpp`. In this case, you could have `EM_VE` and `"elf64-ve"` in one tuple, and `EM_X86_64` and `"elf64-x86-64"` in another.


================
Comment at: llvm/unittests/Object/MachineTest.cpp:1
+//===- MachineTest.cpp - Tests for Machine field in ELFObjectFile ---------===//
+//
----------------
When I said rename the TEST, I meant the TEST `BasicTestForVE` and not the test file name. The test file name should remain ELFObjectFileTest.cpp, to reflect that it is testing that file.


================
Comment at: llvm/unittests/Object/MachineTest.cpp:31
+public:
+  ELF64_DataForTest(unsigned Class, unsigned Encoding, unsigned Machine)
+      : Data(56) {
----------------
Use fixed width types throughout (`uint8_t`, `uint32_t` etc) that match the size that is allowed in the ELF spec. In this case, I think `uint8_t` is the right value in most, maybe all cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79545