Support 64-bit file header and section header writing.
This patch is the first partition of D103696
Details
- Reviewers
jhenderson nemanjai sfertile Esme jasonliu daltenty hubert.reinterpretcast Helflym - Group Reviewers
Restricted Project
Diff Detail
Unit Tests
Time | Test | |
---|---|---|
100 ms | x64 windows > Flang.Driver::output-paths.f90 |
Event Timeline
This is important, we can't approve and land a patch without testing, and this patch is currently in an untestable state as we will hit a fatal error before hitting the code changes in writeFileHeader/writeSectionHeaderTable. On the converse side, there is nothing to stop object file writing after emitting the file header and section header tables.
I've left some inline comments on this patch showing where we will have to make adjustments for your changes to be testable. There is one other modification we need to make in PPCXCOFFObjectWriter::getRelocTypeAndSignSize which gets called as we finalize the layout. We need to add support for mapping the FK_Data_8 fixup type with a VK_None modifier (all the other modifiers should be left supported for now). Note that since the last field is the signdness and (1 - the length) and we are relocating 8 byte data now it will differ slightly from the existing relocations. Something like
case FK_Data_8: switch (Modifier) { default: report_fatal_error("Unsupported modifier"); case MCSymbolRefExpr::VK_None: return {XCOFF::RelocationType::R_POS, EncodedSignednessIndicator | 63}; }
should be fine for this patch, and we can handle the other modifiers in the follow up patches.
llvm/include/llvm/BinaryFormat/XCOFF.h | ||
---|---|---|
30 ↗ | (On Diff #353352) | I'm not sure why these changes are showing up in this patch as they are all landed upstream already (they landed in the patch adding support for yaml2obj support for xcoff) |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
368 | This fatal error needs to be removed to be able to reach the point where we emit the file-header and section header table. | |
380 | The implementation of this function will need to be modified to be correct for 64-bit object generation since the name is always stored in the string table (or in a debug section) and not in the symbol table entry itself. | |
586 | This fatal error will also need to be removed to be able to observe your changes to writeFileHeader and writeSectionheaderTable | |
593 | We should insert an early return after this line, with a comment saying this is the temporary end of the 64-bit implementation. |
Thanks you for your reviews,
I tried to make changes you told me to see if I could make it in time. Because tomorow is my last day at work, I don't have enough time to redo all the patchs especially the tests.
When I applied your recommendations, I got the error "The end of the file was unexpectedly encountered". I think it would require more time to do things properly.
Otherwise, there are tests for the file header and the section header in further patches.
Sorry I couldn't get these reviewed and landed in time before your internship finished. Thank you for all the work you did, It is a pretty impressive the amount you got accomplished.
Your right, its because we are writing values into the file-header or section header table that implies there is following info (like a symbol table offset and non-zero count). I've tested setting those fields to 0 (as well as offsets to relocations and section data) and then we can dump the file headers and section header table. For existing lit tests that dump symbol tables or section contents I would expect llvm-readobj to give an error as those parts are missing from the object file.
Each patch that adds/alters functionality has to be accompanied by testing of said functionality. We can add further testing in subsequent patches but only patches that contain no function changes are allowed to be committed without new testing (or updated testing).
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
58 | I don't see any need to update this structure until we start emitting relocations. | |
80–81 | Isn't the 64-bit object format also limited to a 4 byte symbol table index? |
When I did this work, I had no idea that I could remove "64-bit XCOFF object files are not supported yet." in order to create tests.
Unfortunately, I learned this late,
Thank you so much for your clarifications.
llvm/include/llvm/BinaryFormat/XCOFF.h | ||
---|---|---|
30 ↗ | (On Diff #353352) | I think the patch you are talking about was recently merged. |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
80–81 | Yes I made a mistake, I will change it right now. |
I don't see any need to update this structure until we start emitting relocations.