Page MenuHomePhabricator

[AIX][XCOFF] Add support for 64-bit file header and section header writing
AcceptedPublic

Authored by MaryamBen on Jun 21 2021, 6:44 AM.

Details

Summary

Support 64-bit file header and section header writing.
This patch is the first partition of D103696

Diff Detail

Unit TestsFailed

TimeTest
170 msx64 windows > Clang.Driver::cl-include.c
Script: -- : 'RUN: at line 4'; c:\ws\w32-1\llvm-project\premerge-checks\build\bin\clang.exe --driver-mode=cl -### -- C:\ws\w32-1\llvm-project\premerge-checks\clang\test\Driver\cl-include.c 2>&1 | c:\ws\w32-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w32-1\llvm-project\premerge-checks\clang\test\Driver\cl-include.c --check-prefix=BUILTIN

Event Timeline

MaryamBen requested review of this revision.Jun 21 2021, 6:44 AM
MaryamBen created this revision.
MaryamBen retitled this revision from [AIX][XCOFF64] Add support for 64-bit file header and section header writing to [AIX][XCOFF] Add support for 64-bit file header and section header writing.Jun 21 2021, 7:17 AM

As a non-XCOFF developer, this mostly looks fine to me, but needs a test case.

Helflym accepted this revision.Aug 6 2021, 1:06 AM
Helflym added a subscriber: Helflym.

Ok for the XCOFF part

This revision is now accepted and ready to land.Aug 6 2021, 1:06 AM

As a non-XCOFF developer, this mostly looks fine to me, but needs a test case.

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

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
347

This fatal error needs to be removed to be able to reach the point where we emit the file-header and section header table.

359

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.

562

This fatal error will also need to be removed to be able to observe your changes to writeFileHeader and writeSectionheaderTable

569

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.

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.

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.

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.

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.

Otherwise, there are tests for the file header and the section header in further patches.

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.

78

Isn't the 64-bit object format also limited to a 4 byte symbol table index?

MaryamBen updated this revision to Diff 367735.Aug 20 2021, 1:53 AM
MaryamBen marked 2 inline comments as done.

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

I think the patch you are talking about was recently merged.
When I created the pacth (2 months ago) , these changes weren't applied.

llvm/lib/MC/XCOFFObjectWriter.cpp
78

Yes I made a mistake, I will change it right now.