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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:942
+
+  auto countRelocations = [&](SectionEntry *Sec, uint64_t RelCount) {
+    // Handles relocation field overflows in an XCOFF32 file. An XCOFF64 file
----------------
jhenderson wrote:
> 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.
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.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:990
+    countRelocations(&DwarfSection, DwarfSection.DwarfSect->Relocations.size());
 
+  // Calculate the file offset to the section data.
----------------
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;
```


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1009
+  // paddings bytes for FileOffsetToData calculation.
+  if (!DwarfSections.empty())
+    RawPointer +=
----------------
since you have a !DwarfSections.empty() here, I suggest to put the codes from line  1010 ~1020 into the  same curly braces.


================
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);
----------------
Esme wrote:
> 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.
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.) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137819