[PATCH] D137819: [XCOFF] support the overflow section (only relocation overflow is handled).

[ChenZheng via Phabricator via llvm-commits]

shchenz added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1085
+    for (auto &OverflowSec : OverflowSections) {
+      if (OverflowSec.RelocationCount == static_cast<uint32_t>(Sec->Index)) {
+        RelocationSizeInSec =
----------------
Esme wrote:
> Esme wrote:
> > DiggerLin wrote:
> > > shchenz wrote:
> > > > 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`?
> > > 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 ?
> > 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;
> > ```
> Thanks @shchenz 
> Most sections will not have a corresponding overflow section, so I also think that adding such a member is not common.
OK.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1155
+    RawPointer +=
+        alignTo(CurrAddress,
+                (*DwarfSections.begin()).DwarfSect->MCSec->getAlign()) -
----------------
Esme wrote:
> shchenz wrote:
> > We don't need the `CurrAddress` here, we can just use the `Sections.back()->Address + Sections.back()->Size`.
> 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;
> ```
Have you checked the replacement of {`RawPointer`(at line 1150) - `RawPointer`(at line 1130)} for `CurrAddress`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137819/new/

https://reviews.llvm.org/D137819