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

[Kazushi Marukawa via Phabricator via llvm-commits]

kaz7 marked 3 inline comments as done.
kaz7 added inline comments.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:33
+      1,    0,                         // Type
+      251,  0,                         // Machine
+      1,    0,   0,   0,               // Version
----------------
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.


================
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:
> kaz7 wrote:
> > 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.
> If you look carefully in the Phabricator UI, I've only selected the comment line, because I'm referring to the comment with the "missing trailing full stop". In other words, the comment should be:
> 
> "A very simple ELF object file which contains no data."
Haha.  Thanks for correction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79545