Page MenuHomePhabricator

[VE] Implements minimum MC layer for VE (3/4)
ClosedPublic

Authored by kaz7 on Wed, May 6, 8:07 PM.

Details

Summary

Define ELF binary code for VE and modify code where should use this new code.

Depends on D79544.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Having been looking at another piece of work in the Object library, rather than introducing lit tests for it to cover the specific behaviour, I think gtest unit tests should be introduced instead to cover the behaviour. For more thoughts on this, see D79612.

llvm/test/tools/llvm-readobj/ELF/machine-enum.test
378–379 ↗(On Diff #263333)

I'd move this FIXME comment into the code, highlighting that it is dead code, due to the shared value, and then delete this case here, as it looks like it's meant to be a test, but isn't, which might confuse people in the future.

kaz7 updated this revision to Diff 263649.Wed, May 13, 2:30 AM
kaz7 marked an inline comment as done.

Remove FIXME part from a test and put it to ELFDumper.cpp.

kaz7 added a comment.Wed, May 13, 2:31 AM

Having been looking at another piece of work in the Object library, rather than introducing lit tests for it to cover the specific behaviour, I think gtest unit tests should be introduced instead to cover the behaviour. For more thoughts on this, see D79612.

You mean, for example, making "unittest/Object/ElfMachineTypeTest.cpp" instead of above "test/tools/llvm-readobj/ELF/machine-enum.test". That's make sense. Should I need to make them in this ticket?

Having been looking at another piece of work in the Object library, rather than introducing lit tests for it to cover the specific behaviour, I think gtest unit tests should be introduced instead to cover the behaviour. For more thoughts on this, see D79612.

You mean, for example, making "unittest/Object/ElfMachineTypeTest.cpp" instead of above "test/tools/llvm-readobj/ELF/machine-enum.test". That's make sense. Should I need to make them in this ticket?

Yes, if reasonable to do, since you are adding the new tests in this patch.

kaz7 updated this revision to Diff 263902.Wed, May 13, 6:06 PM

I add several unit tests. I summarized modified files and tests for
each of them below. Please check them and let me know what you think.
Thanks.

  • llvm/include/llvm/BinaryFormat/ELF.h
    • Add unittest/Object/ELFTest.cpp, but the test becomes something like an apple is an apple test. Maybe, it's Ok.
  • llvm/include/llvm/BinaryFormat/ELFRelocs/VE.def
    • Add unittest/Object/ELFTest.cpp, but the test again becomes an apple is an apple test.
  • llvm/include/llvm/Object/ELFObjectFile.h
    • Add unittest/Object/ELFObjectFileTest.cpp. This is a good example as a unit test.
    • Add test/Object/VE/elf64-arch.yaml and elf64-machine.yaml. We can remove them if we have above unit test.
  • llvm/lib/Object/ELF.cpp
    • Add unittest/Object/ELFTest.cpp. The test is also an apple is an apple test, but it can examine getELFRelocationTypeName function, so not bad.
    • Add test/tools/llvm-readobj/ELF/reloc-types-elf-ve.test. We can remove this if we have above unit test.
  • llvm/lib/ObjectYAML/ELFYAML.cpp
    • No idea how to make a unit test.
    • Add test/tools/obj2yaml/ELF/relocation-type-ve.yaml.
  • llvm/tools/llvm-readobj/ELFDumper.cpp
    • Making a unit test requires refactoring of ELFDumper.cpp. It is out of my purpose.
    • Add test/tools/llvm-readobj/ELF/machine-enum.test.

If these modifications and tests are Ok, I'll remove following
duplicated tests.

  • test/Object/VE/elf64-arch.yaml
  • test/Object/VE/elf64-machine.yaml
  • test/tools/llvm-readobj/ELF/reloc-types-elf-ve.test

Thanks @kaz7. I've run out of time to look at this today, but I'll try to get to it tomorrow, or Monday. I noticed you were looking to add unit tests to llvm-readobj, which I don't think you need to do. In general terms, I would suggest unit tests for things in the lib directory, and lit tests for things in the tools directory. Thus, if you've made a change to both code in the llvm-readobj directory and in the Object directory, you'd have unittests for the Object stuff and lit tests for the llvm-readobj stuff.

kaz7 added a comment.Thu, May 14, 3:57 PM

Thank you @jhenderson too. Thanks for spending your time for this review and letting me know your situation. I misunderstood what you said about a unit test. Anyway, I've not made a unit test for tools this time. :)

Have a time and let me know your thought. Thanks!

kaz7 updated this revision to Diff 264123.Thu, May 14, 4:00 PM

Update to correct filenames and to avoid warning messages.

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

llvm/lib/Object/ELF.cpp
201–202

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?

llvm/test/Object/VE/elf64-arch.yaml
1 ↗(On Diff #264123)

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.

llvm/test/tools/llvm-readobj/ELF/machine-enum.test
1 ↗(On Diff #264123)

I'd rename this file to file-header-machine-types.test to show it is specifically about that bit.

llvm/unittests/Object/ELFObjectFileTest.cpp
24

I'd rename this test to "VEMachineTest".

34

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.

47

Nit: empty data -> no data

Also add the missing trailing full stop.

llvm/unittests/Object/ELFTest.cpp
17–18

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.

85

For readability, add a blank line after each individual TEST.

kaz7 marked 7 inline comments as done.Fri, May 15, 1:12 AM

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

llvm/lib/Object/ELF.cpp
201–202

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.

llvm/test/Object/VE/elf64-arch.yaml
1 ↗(On Diff #264123)

Thanks. I'll remove them.

llvm/test/tools/llvm-readobj/ELF/machine-enum.test
1 ↗(On Diff #264123)

OK. Thank your for suggestion.

llvm/unittests/Object/ELFObjectFileTest.cpp
24

I'll rename that.

34

Thank you for suggestions. I'll take care them Monday.

47

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.

llvm/unittests/Object/ELFTest.cpp
17–18

I agree with you. I'll remove them. Thanks.

kaz7 planned changes to this revision.Fri, May 15, 1:13 AM
jhenderson added inline comments.Fri, May 15, 1:39 AM
llvm/unittests/Object/ELFObjectFileTest.cpp
47

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."

kaz7 marked 3 inline comments as done.Mon, May 18, 12:03 AM
kaz7 added inline comments.
llvm/unittests/Object/ELFObjectFileTest.cpp
34

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.

47

Haha. Thanks for correction.

kaz7 updated this revision to Diff 264547.Mon, May 18, 12:03 AM

Update along to suggestions. Need to finish implementation of
unittests/ObjectYAML/ELFRelocationTypeTest.cpp which takes for a
while to learn how to traverse symbol information in ELFObjectFile.

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

llvm/unittests/Object/ELFObjectFileTest.cpp
34

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.

llvm/unittests/Object/MachineTest.cpp
1 ↗(On Diff #264547)

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.

31 ↗(On Diff #264547)

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.

kaz7 marked 3 inline comments as done.Mon, May 18, 2:57 AM

I managed to implement test code for ELFYAML modifications. Now we can test relocation type info through unit tests. Therefore, I'll remove following lit tests.

  • .../tools/llvm-readobj/ELF/reloc-types-elf-ve.test
  • .../tools/obj2yaml/ELF/relocation-type-ve.yaml
llvm/unittests/Object/ELFObjectFileTest.cpp
34

Thank you for suggestions, but I have not enough time to do so. May I stop here to add test to support other architectures? I'm really exhausted by adding these unit tests.

kaz7 updated this revision to Diff 264569.Mon, May 18, 2:59 AM

Add all necessary unit tests now.

Okay, I've had a bit more time to look at the tests. The logic generally looks fine, but there are a number of mostly stylistic issues that need addressing.

clang-format is complaining a lot on some of your new files. Please remember to run it to format them according to the LLVM coding standards.

llvm/unittests/Object/ELFObjectFileTest.cpp
2

Remove the "Machine field in" bit in the comment. It doesn't add anything, and will be incorrect in the future when we add more tests.

24–25

There is no need to mention the class name in this comment. It doesn't add anything to the comment, because the comment is already associated with the struct by its presence.

Also "to represent an ELF".

26–27

Rather than leave this FIXME here, just do it right the first time. You could use something similar to the fallible constructor pattern as described in the LLVM Programmer's Manual to have this object return a an ELFObjectFile instance. You could even consider instead doing the error handling inside the constructor using ASSERT_THAT_EXPECTED to create an ELFObjectFile instance stored in the struct and used by the test.

28

The name of the struct says ELF64, but you pass the Class into the code, implying it's able to work with ELF32. I don't think you can have both. Better would be to make this class able to work with both ELF32 and ELF64 and generate the data accordingly.

31

This is a struct. No need for public here.

33

I don't think there's any real benefit to pre-sizing this vector, but even if there is, you should use sizeof(Elf64_Ehdr) or equivalent for ELF32 rather than hard-coding in the number.

34–35

You allow passing in of the Encoding value, but are hard-coding here an assumption about it being little endian (or more specifically, the same as the host machine). You need to a) handle hosts being either little or big endian (using sys::isLittleEndian) and b) handle different Encoding values. There are already functions in LLVM for byte swapping. Consider using those instead, possibly in combination with reinterpret_cast<uint8_t *> to convert it to a sequence of bytes.

61

LLVM style doesn't allow names to have underscores in them. I'd also be even more generic with this name and just put something like ELFObjectFileTest. This will allow users to just run it specifically alongside other future ELFObjectFile tests.

llvm/unittests/ObjectYAML/ELFRelocationTypeTest.cpp
1 ↗(On Diff #264569)

Similar to the ELFObjectFile test, this test file should be named after the source file it is testing, and the comment here needs updating.

36 ↗(On Diff #264569)

Don't use auto where the type isn't obvious. Instead, please use the type explicitly.

82 ↗(On Diff #264569)

I think SectionRef and RelocationRef below are intended to be like StringRef in that they don't need to be passed around as const & since they are lightweight.

130 ↗(On Diff #264569)

Perhaps "Found unexpected relocation type: " + R.getType() would be a more useful error message.

kaz7 marked 11 inline comments as done.Wed, May 20, 7:49 PM

OK, I will run clang-format. It modifies ENUM_ENT definitions in ELFDumper.cpp, so that I was not running it in this patch since I feel modifying existing format is not good. Maybe I should think like this patch is a good practice to update existing format. Thanks.

llvm/unittests/Object/ELFObjectFileTest.cpp
26–27

Thanks for the link. I couldn't handle it correctly at first try. I would like to try it if I have time.

On the other hand, we don't need to hold ELFObjectFile in this structure since I'm not using test fixtures yet. I supported not only EM_VE but also X86_64, 385, all ELF64, ELF32, big endian, little endian MIPS cases in this test. I appreciate if I can stop extending and generalizing this test case here at least for this patch.

kaz7 updated this revision to Diff 265409.Wed, May 20, 7:50 PM

Update regression tests.
Also rebase to the latest master.

kaz7 marked an inline comment as done.Thu, May 21, 1:44 AM
kaz7 added inline comments.
llvm/tools/llvm-readobj/ELFDumper.cpp
1474

FYI, I've checked registry status internally. And I heard that it is registered through registry@sco.com.

Sorry for the delay in responding - I ran out of time on Friday, and then it was a public holiday yesterday, so I wasn't working. Mostly, this looks fine. I just think the ELFObjectFIle unit tests still has some issues, which are described inline and below. Once you've resolve those, I think this is good to go.

I'm not a fan of writing out the ELF data byte-by-byte as you have done in the ELFObjectFile test - I think it can be simplified somewhat, and made more flexible/less prone to being broken by endianness issues etc. ELF already has the Ehdr structures, which you can use to populate the data below, rather than individually assigning each byte. Here's what I would recommend to solve these issues (note - code is untested, so don't blindly copy, but should give you a good indication of what I think you should do):

// You might want to cosider storing Encoding in the struct, to make using this function easier.
template <typename T> T swap(T Value, uint8_t Encoding) {
  assert(Encoding == ELF::ELFDATA2MSB || Encoding == ELF::ELFDATA2LSB);
  bool IsLittleEndian == ELF::ELFDATA2LSB;
  if (IsLittleEndianHost != IsLittleEndian)
    return sys::swapByteOrder(Value);
  return Value;
}

DataForTest(uint8_t Class, uint8_t Encoding, uint16_t Machine) {
  if (Class == ELF::ELFCLASS64)
    Data = makeElfData<ELF::Elf64_Ehdr>(Class, Encoding, Machine);
  else {
    assert(Class == ELF::ELFCLASS32);
    Data = makeElfData<ELF::Elf64_Ehdr>(Class, Encoding, Machine);
  }
}

template <typename T>
std::vector<uint8_t> makeElfData(uint8_t Class, uint8_t Encoding, uint16_t Machine) {
  T Ehdr {}; // Zero-initialise the header.
  Ehdr.e_ident[ELF::EI_MAG0] = 0x7f;
  Ehdr.e_ident[ELF::EI_MAG1] = 'E';
  Ehdr.e_ident[ELF::EI_MAG2] = 'L';
  Ehdr.e_ident[ELF::EI_MAG3] = 'F';
  Ehdr.e_ident[ELF::EI_CLASS] = Class;
  Ehdr.e_ident[ELF::EI_DATA] = Encoding;
  Ehdr.e_ident[ELF::EI_VERSION] = 1;
  Ehdr.e_type = swap(ELF::ET_REL, Encoding);
  Ehdr.e_machine = swap(Machine, Encoding);
  Ehdr.e_version = swap(uint16_t(1), Encoding);
  Ehdr.e_ehsize = swap(uint16_t(sizeof(T)), Encoding);
  uint8_t *EhdrBytes = reinterpret_cast<uint8_t *>(&Ehdr);
  std::vector<uint8_t> Bytes;
  std::copy(EhdrBytes, EhdrBytes + sizeof(Ehdr), std::back_inserter(Bytes));
  return Bytes;
}
llvm/tools/llvm-readobj/ELFDumper.cpp
1474

Great, good to hear. When I was referring to clang-formatting things, I was specifically referring to the new files you've added, rather than this enum, which I'm not convinced we should reformat. Could you undo the formatting changes to this bit, please?

llvm/unittests/Object/ELFObjectFileTest.cpp
28

No need to explicitly initialise Data.

31

If I'm reading the code correctly, this will unconditionally swap bytes for big endian inputs, regardless of the host endianness. If the host machine is big endian, you don't want to swap. There are build bots out there which are big endian, so you need to handle that case, or the test will fail on those.

See my out-of-line comment for a possible solution.

37

I don't think assigning each byte individually is a good way to go. See my out-of-line comment for an alternative.

llvm/unittests/Object/ELFTest.cpp
57

No need for an additional blank line here at the end of the file.

kaz7 planned changes to this revision.Tue, May 26, 2:50 AM
kaz7 marked 5 inline comments as done.

No problem. Thank you for your time for reviewing. I also almost ran out my time this month to work on this. ;)

llvm/tools/llvm-readobj/ELFDumper.cpp
1474

Thanks. This clang-format thing is useful, but it becomes little annoying if I modifies code region which doesn't follow clang-format style. ;). Anyway, I'll revert this.

llvm/unittests/Object/ELFObjectFileTest.cpp
31

You are right. I didn't think about that scenario. And thank you for your example.

llvm/unittests/Object/ELFTest.cpp
57

Thank you for mentioning that. I also want to remove blank lines at the EOF and blanks at the EOL. But this time I cannot find blank line at the end of this file. Maybe smoking of glitches on the web? Please let me know if you find a blank like again.

jhenderson added inline comments.Tue, May 26, 3:20 AM
llvm/unittests/Object/ELFTest.cpp
57

Looks like it. It's not the first time I've seen that happened.

kaz7 updated this revision to Diff 266164.Tue, May 26, 4:30 AM

Update following jhenderson's suggestions. Also rebase to the latest.

jhenderson accepted this revision.Wed, May 27, 1:52 AM

Great work, LGTM. Thanks for persevering! The testing framework you've started should also prove very useful in the future, I think.

llvm/tools/llvm-readobj/ELFDumper.cpp
1474

Yeah, there might be a way to resolve that, but I'm not sure exactly how. I certainly think this format makes sense for the current context.

This revision is now accepted and ready to land.Wed, May 27, 1:52 AM
This revision was automatically updated to reflect the committed changes.
kaz7 added a comment.Thu, May 28, 8:03 AM

Thank you. I appreciate your kind suggestions and examples too.