Define ELF binary code for VE and modify code where should use this new code.
Depends on D79544.
Differential D79545
[VE] Implements minimum MC layer for VE (3/4) kaz7 on May 6 2020, 8:07 PM. Authored by
Details
Diff Detail
Event Timeline
Comment Actions Thank you for your comments and examples for regression tests. I'll add them.
Comment Actions Add some tests locally and found a problem need other's help. Please let me know how to fix EM_ECOG1X problem and how to check Tripe::ve. Thanks!
Comment Actions Thank you for kind suggestions.
Comment Actions I update following jhenderson's suggestions and also add a new test to
Comment Actions 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.
Comment Actions 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? Comment Actions I add several unit tests. I summarized modified files and tests for
If these modifications and tests are Ok, I'll remove following
Comment Actions 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. Comment Actions 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! Comment Actions Tests mostly look fine, thanks, but see inline comments.
Comment Actions Thanks for inspecting tests. I'll update them Monday again.
Comment Actions Update along to suggestions. Need to finish implementation of Comment Actions I've run out of time, so will come back to looking at the rest of the updates tomorrow.
Comment Actions 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.
Comment Actions 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.
Comment Actions 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.
Comment Actions 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; }
Comment Actions No problem. Thank you for your time for reviewing. I also almost ran out my time this month to work on this. ;)
Comment Actions Great work, LGTM. Thanks for persevering! The testing framework you've started should also prove very useful in the future, I think.
|
Why are some commented out?