[PATCH] D137819: [XCOFF] support the overflow section.

[Esme Yi via Phabricator via llvm-commits]

Esme added a comment.

In D137819#3925955 <https://reviews.llvm.org/D137819#3925955>, @DiggerLin wrote:

> 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.



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:819
+    // the number of line-number entries actually required.
     writeWord(IsDwarf ? 0 : Sec->Address);
+    writeWord((IsDwarf || IsOvrflo) ? 0 : Sec->Address);
----------------
DiggerLin wrote:
> 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
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.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:834
       W.write<uint16_t>(Sec->RelocationCount);
-      W.write<uint16_t>(0); // NumberOfLineNumbers. Not supported yet.
+      W.write<uint16_t>(IsOvrflo
+                            ? Sec->RelocationCount
----------------
DiggerLin wrote:
> 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.
Oh I see, I should check (IsOvrflo || Sec->RelocationCount == XCOFF::RelocOverflow).


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:949
+      SecEntry.Address = RelCount;
+      SecEntry.Index = 3;
+
----------------
DiggerLin wrote:
> why set index to 3 here?
I set it  as 3 it during debug, but forgot to delete it. Thank you for finding it.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:980
+  // Calculate the file offset to the section data.
+  uint64_t CurrAddress = 0;
+  for (auto *Sec : Sections) {
----------------
DiggerLin wrote:
> 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;
> }
> 
> ```
> 
> 
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)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137819