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

[Esme Yi via Phabricator via llvm-commits]

Esme 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 =
----------------
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;
```


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1085
+    for (auto &OverflowSec : OverflowSections) {
+      if (OverflowSec.RelocationCount == static_cast<uint32_t>(Sec->Index)) {
+        RelocationSizeInSec =
----------------
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.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1155
+    RawPointer +=
+        alignTo(CurrAddress,
+                (*DwarfSections.begin()).DwarfSect->MCSec->getAlign()) -
----------------
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;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137819