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

[Esme Yi via Phabricator via llvm-commits]

Esme added inline comments.


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


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1000
+    if (RawPointer > MaxRawDataSize)
+      report_fatal_error("Section raw data overflowed this object file.");
+
----------------
jhenderson wrote:
> 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.
Thanks, I'll post another patch for this work after this patch is committed.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1144
-    assert(RawPointer <= MaxRawDataSize &&
-           "Section raw data overflowed this object file.");
-  }
----------------
pscoro wrote:
> 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? 
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.


================
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:
> 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.) 
```
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137819