This is an archive of the discontinued LLVM Phabricator instance.

[XCOFF] support the overflow section (only relocation overflow is handled).
ClosedPublic

Authored by Esme on Nov 11 2022, 1:12 AM.

Details

Summary

Handles relocation or line-number field overflows in an XCOFF32 file. (XCOFF64 files may not have overflow section headers.) If a section has more than 65,534 relocation entries or line number entries, both of these fields are set to a value of 65535. In this case, an overflow section header with the s_flags field equal to STYP_OVRFLO is used to contain the relocation and line-number count information.
Since line number is not supported, this patch only handles the relocation overflow.

Diff Detail

Event Timeline

Esme created this revision.Nov 11 2022, 1:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2022, 1:12 AM
Esme requested review of this revision.Nov 11 2022, 1:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2022, 1:12 AM

change the description to 'support the relocation overflow section."

llvm/lib/MC/XCOFFObjectWriter.cpp
931–932

the information of SectionEntry is not enough to identify the the overflow is relocation or line no overflow. I think we need to derived class from SectionEntry and have flag in the derived class to identify the overflow is relocation or line no overflow.

and if the flag is relocation overflow, the Sec->Address will be write in the s_paddr, otherwise Sec->Address will be writen in the s_vaddr

942–943

the IsOvrflo is used to identify overflow section. but for the section which has overflow relocation or line no overflow, the both field of s_nlnno and s_nlnno should be set to same (both 65535) , it looks you not implement it.

1128

why set index to 3 here?

1136

I can not find "the s_relptr must have the same values as in the corresponding primary section header." in the xcoff document.

s_relptr Recognized for the .text , .data , .tdata, and STYP_DWARF sections only.
so the field "s_relptr" in the overflow section should be set to zero?

DiggerLin added inline comments.Nov 15 2022, 10:11 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
1123

nit : suggest moving the line after the SecEntry.Index=3. put assign the member of SecEntry together.

1136

sorry I found it . please ignore above comment.

1390

since you change the caculation logic of the RawPointer and EndOfAllHeadersOffset in the patch. I suggest to delete the code from line 1185~1193 and

change

uint64_t RawPointer = EndOfAllHeadersOffset;

to

uint64_t RawPointer =
     (is64Bit() ? (XCOFF::FileHeaderSize64 +
                   SectionCount * XCOFF::SectionHeaderSize64)
                : (XCOFF::FileHeaderSize32 +
                   SectionCount * XCOFF::SectionHeaderSize32)) +
     auxiliaryHeaderSize();

at the begine of function void XCOFFObjectWriter::finalizeSectionInfo()

and we can delete the variable "EndOfAllHeadersOffset" from the patch.

DiggerLin added inline comments.Nov 15 2022, 10:11 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
1126

we do not need to calculate the CurrAddress if there is not Dwarf section. we can delete the CurrAddress and code of calculation of CurrAddress , and get the last element of Sections when calculating the padding before the dwarf section.

for example :

if (!DwarfSections.empty()) {
   CsectSectionEntry * LastCsectSec = Sections.back();
   uint64_t LastCsectSecAddr = LastCsectSec ->Address + LastCsectSet ->Size;
  RawPointer +=
        alignTo(LastCsectSecAddr,
                (*DwarfSections.begin()).DwarfSect->MCSec->getAlignment()) -
        LastCsectSecAddr;
}
Esme updated this revision to Diff 476749.Nov 20 2022, 8:09 AM
Esme marked 4 inline comments as done.

change the description to 'support the relocation overflow section."

As the doc says, the overflow section is required when either of the relocation entries or the line number entries exceeds 65,534. As long as one of the counts exceeds 65,534, the other one will also be set to 65535 in the primary section header, and then the overflow section will record actual counts for both relocation entries and line number entries, where the s_paddr for relocation entries and s_vaddr for line number entries.
Therefore I reckon there's no need to identify whether it's a relocation overflow section or line number overflow section.

llvm/lib/MC/XCOFFObjectWriter.cpp
931–932

As my previous comments, there's no need to identify whether it's a relocation overflow section or line number overflow section.
Since the line number is not supported, set the Virtual Address, which specifies the number of line-number entries actually required, to 0.

942–943

Oh I see, I should check (IsOvrflo || Sec->RelocationCount == XCOFF::RelocOverflow).

1126

We skip over some sections in the loop:

if (Sec->Index == SectionEntry::UninitializedIndex || Sec->IsVirtual)
  continue;

So LastCsectSecAddr may not equal to CurrAddress if the last section satisfies (Sec->Index == SectionEntry::UninitializedIndex || Sec->IsVirtual).

1128

I set it as 3 it during debug, but forgot to delete it. Thank you for finding it.

Esme updated this revision to Diff 476750.Nov 20 2022, 8:13 AM

The functionality of this patch is best left for @DiggerLin to review, so I've kept my comments to stylistic ones.

llvm/lib/MC/XCOFFObjectWriter.cpp
286

Not sure why you're abbreviating here. Just use OverflowSections.

413

Again, OverflowSec would be preferable.

931–932
932–936

Either "for the overflow section" or "for overflow sections".

934
945
1117

General consensus is that lambdas should be named like variables, because they are function objects, not functions. I.e. CountRelocations, not countRelocations.

That being said, this lambda is to long. Pull it out into a helper function instead.

1120

Nit: prefer static_cast to C-style cast.

1126

Prefer static_cast.

1127

Again, this lambda is too long - pull it into a helper function.

1129

Prefer static_cast.

1132

https://llvm.org/docs/CodingStandards.html#prefer-preincrement

You should probably also do it as part of the assignment, i.e. SecEntry.Index = ++SectionCount;

1134

Noting here and in other places that this should really return an Error rather than report_fatal_error. That's another piece of work though.

1141

Simply "some padding" or "some padding bytes", but not "some paddings".

Also I'd change the last bit to "and to use this padding in FileOffsetToData calculation."

change the description to 'support the relocation overflow section."

As the doc says, the overflow section is required when either of the relocation entries or the line number entries exceeds 65,534. As long as one of the counts exceeds 65,534, the other one will also be set to 65535 in the primary section header, and then the overflow section will record actual counts for both relocation entries and line number entries, where the s_paddr for relocation entries and s_vaddr for line number entries.
Therefore I reckon there's no need to identify whether it's a relocation overflow section or line number overflow section.

if one day, you want to add the functionality patch of overflow section for line number , what title  you want to use for that new patch?
DiggerLin added inline comments.Nov 28 2022, 10:20 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
931–932

assume you need to implement line number overflow in next patch, where you want to put the actually line number in SectionEntry of overflow section?(you put the relocation number in the Sec->Address in current patch.)

1117

the "countRelocations " can not self-explain the functionality of the lambda. the Relcount is already calculate outside the countRelocations, the lambda function just check whether need to generate a overflow section and set the relocation count to section header.

1125–1126

move the codes line 935 ~940 to here( uint64_t CurrAddress = 0;), and you can delete the code line 960~961

// Add the overflow section header size to RawPointer.
      RawPointer += XCOFF::SectionHeaderSize32;
1143

since you have a !DwarfSections.empty() here, I suggest to put the codes from line 1010 ~1020 into the same curly braces.

pscoro added a subscriber: pscoro.Dec 1 2022, 12:09 PM
pscoro added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
286

Seems likely because of how XCOFF documentation has abbreviated their flags: STYP_OVRFLO is the XCOFF flag. I agree with using full word for variable name because the full name of the section is still "Overflow section"

1335

Concerned about whether my exception section fintegrated-as support commit might conflict with how this logic was rearranged. My commit added code after this line that I assume this review wants moved elsewhere. Can we rebase against a more recent commit that includes https://reviews.llvm.org/D134195 and test?

Esme updated this revision to Diff 479925.Dec 4 2022, 7:51 AM
Esme marked 15 inline comments as done.
Esme retitled this revision from [XCOFF] support the overflow section. to [XCOFF] support the overflow section (only relocation overflow are handled)..

Addressed comments.

Esme edited the summary of this revision. (Show Details)Dec 4 2022, 7:52 AM
Esme added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
931–932
struct SectionEntry {
  char Name[XCOFF::NameSize];
  // The physical/virtual address of the section. For an object file
  // these values are equivalent.
  uint64_t Address;
  uint64_t Size;
  uint64_t FileOffsetToData;
  uint64_t FileOffsetToRelocations;
  uint32_t RelocationCount;
  int32_t Flags;

  int16_t Index;
...
}

To implement the line number, we have to add some members to SectionEntry, like FileOffsetToLineNum and LineNumCount, and I think we can distinguish the Address here into PhysicalAddress and VirtualAddress, which will be used to record the actual line number when the section is an overflow section.

1125–1126

Oh yes, thanks.

1134

Thanks, I'll post another patch for this work after this patch is committed.

1335

Thanks for reminding.
It seems that D134195 wasn't committed after format-clean, so I format them and then rebase, which brings formatting changes to this patch. If necessary, we can submit a separate NFC patch for formatting.

Esme added a comment.Dec 4 2022, 7:54 AM

change the description to 'support the relocation overflow section."

As the doc says, the overflow section is required when either of the relocation entries or the line number entries exceeds 65,534. As long as one of the counts exceeds 65,534, the other one will also be set to 65535 in the primary section header, and then the overflow section will record actual counts for both relocation entries and line number entries, where the s_paddr for relocation entries and s_vaddr for line number entries.
Therefore I reckon there's no need to identify whether it's a relocation overflow section or line number overflow section.

if one day, you want to add the functionality patch of overflow section for line number , what title  you want to use for that new patch?

Updated the title and description. Thanks!

Esme retitled this revision from [XCOFF] support the overflow section (only relocation overflow are handled). to [XCOFF] support the overflow section (only relocation overflow is handled)..Dec 4 2022, 7:55 AM
Esme updated this revision to Diff 479978.Dec 4 2022, 9:12 PM

Rebase on latest commit.

I'm okay with an NFC reformatting patch if it's needed, rather than trying to bundle together the changes in with functional changes.

llvm/lib/MC/XCOFFObjectWriter.cpp
945

Ping?

1086

Noting another incident of OvrflowSec, which should be OverflowSec.

Esme updated this revision to Diff 480013.Dec 5 2022, 1:14 AM
Esme marked 3 inline comments as done.

Addressed comments.

Esme added a comment.Dec 5 2022, 1:17 AM

I'm okay with an NFC reformatting patch if it's needed, rather than trying to bundle together the changes in with functional changes.

Agree, rG664cbfaf07e0 is the NFC reformatting patch.

DiggerLin added inline comments.Dec 5 2022, 10:19 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
931–932

I think we can distinguish the Address here into PhysicalAddress and VirtualAddress, which will be used to record the actual line number when the section is an overflow section.

you want to add a new flag to distinguish the Address here into PhysicalAddress and VirtualAddress later when you implement the line number overflow patch? that is what I ask for. please add the flag now or add some comment on it.

llvm/test/CodeGen/PowerPC/aix-xcoff-huge-relocs.ll
8

since 64bit do not have overflow section. I think we also need 64bits test case.

Esme updated this revision to Diff 480325.Dec 5 2022, 8:47 PM
Esme added inline comments.
llvm/test/CodeGen/PowerPC/aix-xcoff-huge-relocs.ll
8

We've already test XCOFF64. (See line 20~23)

jhenderson added inline comments.Dec 5 2022, 11:55 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
111
113
115
947

No need for "for" here. Also duplicate "the".

This comment seems to basically be saying the same thing as the new one in the header. I don't think we need both as it's just repeating information. @DiggerLin, what do you think?

DiggerLin added inline comments.Dec 6 2022, 8:49 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
947

agree with James.

Esme updated this revision to Diff 480776.Dec 6 2022, 10:43 PM
Esme marked 3 inline comments as done.

Update the comments.

No more comments from me, thanks!

DiggerLin added inline comments.Dec 14 2022, 12:12 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
1082

we do not need introduce a new variable RelocationSizeInSec

We can update

RawPointer += OverflowSec.Address * XCOFF::RelocationSerializationSize32;

and

RawPointer += Sec->RelocationCount *
                          (is64Bit() ? XCOFF::RelocationSerializationSize64: XCOFF::RelocationSerializationSize32);

directly.

Esme added inline comments.Dec 14 2022, 7:05 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
1082

Nice point, but this variable is also used to check whether the primary section has a corresponding overflow section, so I think it is useful to introduce this variable.
assert(RelocationSizeInSec && "Overflow section header doesn't exist.");

Looks great! I also did some functionality test related to -g/-ffunction-sections, all look good.

llvm/lib/MC/XCOFFObjectWriter.cpp
1066

Can we use push_back(std::move(SecEntry))?

1087

Can we add a field in SectionEntry struct to indicate the related overflow section index, so that we don't need the loop here? For example a new field about index to vector OverflowSec?

1157

We don't need the CurrAddress here, we can just use the Sections.back()->Address + Sections.back()->Size.

DiggerLin added inline comments.Dec 15 2022, 7:35 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
1087

all the data in the SectionEntry should be common to all Section.

maybe we can delete the loop by setting the

Sec->RelocationCount as real relocation number.

we do not need to do

// The field in the primary section header is always 65535
 // (XCOFF::RelocOverflow).
 Sec->RelocationCount = XCOFF::RelocOverflow;

in function
XCOFFObjectWriter::finalizeRelocationInfo

and in function XCOFFObjectWriter::writeSectionHeader
when found the
Sec->RelocationCount >= XCOFF::RelocOverflow
we write as XCOFF::RelocOverflow ?

Esme updated this revision to Diff 484507.Dec 21 2022, 2:38 AM
Esme marked an inline comment as done.

Addressed comments.
Didn't find a better way instead of looping through to find the corresponding overflow section.

llvm/lib/MC/XCOFFObjectWriter.cpp
1087

Thanks @DiggerLin
With the approach you mentioned, we will miss to set value to FileOffsetToRelocations for overflow section. It's necessary to find out the corresponding overflow section.

// This field must have the same values as in the corresponding
// primary section header.
OverflowSec.FileOffsetToRelocations = Sec->FileOffsetToRelocations;
1087

Thanks @shchenz
Most sections will not have a corresponding overflow section, so I also think that adding such a member is not common.

1157

Digger also had such comment before (see comments below line 1124), we can't do that because we skip over some sections in the loop:

if (Sec->Index == SectionEntry::UninitializedIndex || Sec->IsVirtual)
      continue;
shchenz added inline comments.Dec 22 2022, 7:37 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
1087

OK.

1157

Have you checked the replacement of {RawPointer(at line 1150) - RawPointer(at line 1130)} for CurrAddress?

Esme added inline comments.Jan 4 2023, 8:30 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
1157

Yes, I've checked that and they're not equal.

shchenz accepted this revision as: shchenz.Jan 5 2023, 12:01 AM

LGTM. Thanks for working on this.

This revision is now accepted and ready to land.Jan 5 2023, 12:01 AM